Prevent unsigned integer overflow in multiplication

[25077667]

The current implementation involves multiplying values that could potentially result in an overflow when converted to size_t, specifically when dealing with unsigned int. To address this issue as identified by CodeQL, we propose a solution.

By incorporating uint64_t{1} into the calculation, we intentionally promote the multiplication type to uint64_t before the result is computed. This strategic promotion helps mitigate the risk of overflow.

It's important to note that we've opted for this approach instead of using static_cast<uint64_t>() due to concerns about code readability and consistency. Using static_cast<uint64_t>() on both the left and right operands would lead to a lengthier expression, potentially impacting clarity. By introducing uint64_t{1}, we achieve our goal of preventing overflow while maintaining a more elegant code structure. This approach ensures that the promotion is uniform and balanced across both sides of the expression.

[randombit]

@25077667 thanks for your contribution. I don't think this is quite how we'd want to address it since for systems that use 32 bit size_t we'll cause other warnings due to integer truncation.

@reneme it seems all of the implicated expressions use params.n which returns a uint32_t. What do you think about either changing this return type, or alternately adding another getter on the params which returns n upcasted to a size_t?

[reneme]

Albeit technically correct, I'm sure those CodeQL findings are false positives for the specified parameter sets of SPHINCS+. Nevertheless: Maybe we should promote all algorithm parameters (and their getter methods) in sp_parameters.h to size_t. That would probably be the most concise option.

[randombit]

I'm sure those CodeQL findings are false positives for the specified parameter sets of SPHINCS+

Yes, I already checked them initially when CodeQL was turned on and agree they are not really relevant/possible.

But I do think we should address them some way or another; quoting goals.rst

Within reason, all warnings from compilers and static analyzers should be addressed, even if they seem like false positives, because that maximizes the signal value of new warnings from the tool.

Maybe we should promote all algorithm parameters (and their getter methods) in sp_parameters.h to size_t.

If that's an option that seems best to me. I had thought these were uint32_t for some structure packing reason (eg we somewhere do store_le(something_derived_from_params) and needed that to be precisely 32 bits. If that's not the case, having them be size_t is certainly preferable.