| Message ID | 20240716082825.65219-1-r.smirnov@omp.ru (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: mpi: add NULL checks to mpi_normalize(). | expand |
On Tue, Jul 16, 2024 at 11:28:25AM +0300, Roman Smirnov wrote: > If a->d is NULL, the NULL pointer will be dereferenced. It > is necessary to prevent this case. There is at least one call > stack that can lead to it: > > mpi_ec_curve_point() > ec_pow2() > ec_mulm() > ec_mod() > mpi_mod() > mpi_fdiv_r() > mpi_tdiv_r() > mpi_tdiv_qr() > mpi_resize() > kcalloc() > > mpi_resize can return -ENOMEM, but this case is not handled in any way. > > Next, dereferencing takes place: > > mpi_ec_curve_point() > mpi_cmp() > do_mpi_cmp() > mpi_normalize() > > Found by Linux Verification Center (linuxtesting.org) with Svace. > > Signed-off-by: Roman Smirnov <r.smirnov@omp.ru> > --- > lib/crypto/mpi/mpi-bit.c | 3 +++ > 1 file changed, 3 insertions(+) I've just posted a patch to remove mpi_ec_curve_point and mpi_tdiv_qr. Are there any other code paths with the same problem? Thanks,
On Fri, 2024-08-02 at 20:46 +0800, Herbert Xu wrote: > On Tue, Jul 16, 2024 at 11:28:25AM +0300, Roman Smirnov wrote: > > If a->d is NULL, the NULL pointer will be dereferenced. It > > is necessary to prevent this case. There is at least one call > > stack that can lead to it: > > > > mpi_ec_curve_point() > > ec_pow2() > > ec_mulm() > > ec_mod() > > mpi_mod() > > mpi_fdiv_r() > > mpi_tdiv_r() > > mpi_tdiv_qr() > > mpi_resize() > > kcalloc() > > > > mpi_resize can return -ENOMEM, but this case is not handled in any way. > > > > Next, dereferencing takes place: > > > > mpi_ec_curve_point() > > mpi_cmp() > > do_mpi_cmp() > > mpi_normalize() > > > > Found by Linux Verification Center (linuxtesting.org) with Svace. > > > > Signed-off-by: Roman Smirnov <r.smirnov@omp.ru> > > --- > > lib/crypto/mpi/mpi-bit.c | 3 +++ > > 1 file changed, 3 insertions(+) > > I've just posted a patch to remove mpi_ec_curve_point and mpi_tdiv_qr. > Are there any other code paths with the same problem? Svace found a similar case but it is no longer relevant: NULL constant: mpi_ec_mul_point() ec_mulm(z3, point->z, z2, ctx) ec_mod() mpi_mod() mpi_fdiv_r() mpi_tdiv_r() mpi_tdiv_qr() mpi_resize() kcalloc() Dereference: mpi_ec_mul_point() ec_invm(z3, z3, ctx) mpi_invm() mpi_cmp_ui() mpi_normalize() > > Thanks,
On Wed, Aug 07, 2024 at 02:20:36PM +0000, Roman Smirnov wrote: > > Svace found a similar case but it is no longer relevant: > > NULL constant: > mpi_ec_mul_point() > ec_mulm(z3, point->z, z2, ctx) > ec_mod() > mpi_mod() > mpi_fdiv_r() > mpi_tdiv_r() > mpi_tdiv_qr() > mpi_resize() > kcalloc() In general, I think it's better to deal with the error at the point of allocation. So whoever calls mpi_resize should check for errors and return an error if appropriate. We can deal with this if this function is ever reintroduced. Thanks,
diff --git a/lib/crypto/mpi/mpi-bit.c b/lib/crypto/mpi/mpi-bit.c index 070ba784c9f1..d7420bdb4ff2 100644 --- a/lib/crypto/mpi/mpi-bit.c +++ b/lib/crypto/mpi/mpi-bit.c @@ -29,6 +29,9 @@ */ void mpi_normalize(MPI a) { + if (!a || !a->d) + return; + for (; a->nlimbs && !a->d[a->nlimbs - 1]; a->nlimbs--) ; }
If a->d is NULL, the NULL pointer will be dereferenced. It is necessary to prevent this case. There is at least one call stack that can lead to it: mpi_ec_curve_point() ec_pow2() ec_mulm() ec_mod() mpi_mod() mpi_fdiv_r() mpi_tdiv_r() mpi_tdiv_qr() mpi_resize() kcalloc() mpi_resize can return -ENOMEM, but this case is not handled in any way. Next, dereferencing takes place: mpi_ec_curve_point() mpi_cmp() do_mpi_cmp() mpi_normalize() Found by Linux Verification Center (linuxtesting.org) with Svace. Signed-off-by: Roman Smirnov <r.smirnov@omp.ru> --- lib/crypto/mpi/mpi-bit.c | 3 +++ 1 file changed, 3 insertions(+)