Re: [PATCH] lib/crypto/mpi: Fix heap OOB write in mpi_powm() when res aliases input

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] lib/crypto/mpi: use-after-free and leak in mpi_powm() when assign_rp is set
--

commit 85051cb2fd0529447a03f7f3bbc383a2addcb07d
Author: Maholi Tumanggor <maholitumanggor@gmail.com>

lib/crypto/mpi: Fix heap OOB write in mpi_powm() when res aliases input

This commit avoids writing the exponentiation result into an undersized
res->d buffer in mpi_powm() when assign_rp is set due to res->d aliasing
an input operand.

> diff --git a/lib/crypto/mpi/mpi-pow.c b/lib/crypto/mpi/mpi-pow.c
> index 9e695a3bda3a0..06038ac8f92ac 100644
> --- a/lib/crypto/mpi/mpi-pow.c
> +++ b/lib/crypto/mpi/mpi-pow.c
[ ... ]
> @@ -256,14 +256,16 @@ int mpi_powm(MPI res, MPI base, MPI exp, MPI mod)
>  		 * might be, see above).
>  		 */
>  		if (mod_shift_cnt) {
> -			carry_limb =
> -			    mpihelp_lshift(res->d, rp, rsize, mod_shift_cnt);
> +			mpi_ptr_t dst = assign_rp ? rp : res->d;
> +
> +			carry_limb = mpihelp_lshift(dst, rp, rsize, mod_shift_cnt);
> +			if (!assign_rp)
>  			rp = res->d;
>  			if (carry_limb) {
> -				rp[rsize] = carry_limb;
> -				rsize++;
> +			rp[rsize] = carry_limb;
> +			rsize++;
>  			}
> -		} else {
> +		} else if (!assign_rp) {
>  			MPN_COPY(res->d, rp, rsize);
>  			rp = res->d;
>  		}

[Severity: Critical]
When assign_rp is set in lib/crypto/mpi/mpi-pow.c:mpi_powm() because
res->alloced < size and rp aliases bp, ep, or mp, a new limb buffer is
allocated directly into rp without saving a marker pointer:

	if (res->alloced < size) {
		if (rp == ep || rp == mp || rp == bp) {
			rp = mpi_alloc_limb_space(size);
			if (!rp)
				goto enomem;
			assign_rp = 1;

Later in the exponentiation loop, rp and xp are swapped after every
squaring step and after every multiplication step:

		xp = xp_marker = mpi_alloc_limb_space(2 * (msize + 1));
		...
		for (;;) {
			while (c) {
				...
				swap(rp, xp);
				rsize = xsize;

				if ((mpi_limb_signed_t) e < 0) {
					...
					swap(rp, xp);
					rsize = xsize;
				}

If the exponent causes an odd number of swap(rp, xp) operations (for
example when exp has an odd number of zero bits after the most significant
bit, such as exp = 2 or exp = 65537), the loop finishes with rp pointing
to xp_marker and the block-local xp pointing to the buffer allocated for
assign_rp.

With this change, when assign_rp is 1, rp is left pointing to xp_marker
while xp goes out of scope. At the end of mpi_powm():

	if (assign_rp)
		mpi_assign_limb_space(res, rp, size);
	...
	if (xp_marker)
		mpi_free_limb_space(xp_marker);

Wouldn't mpi_assign_limb_space() assign xp_marker to res->d and
mpi_free_limb_space(xp_marker) immediately zero and free it, leaving
res->d pointing to freed memory while leaking the limb buffer originally
allocated for rp?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102064225.2271-1-maholitumanggor@gmail.com?part=1