| Message ID | d870c278-3f0e-4386-a58d-c9e2c97a7c6c@moroto.mountain (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: rsa - add a check for allocation failure | expand |
On Mon, Oct 30, 2023 at 12:02:59PM +0300, Dan Carpenter wrote: > Static checkers insist that the mpi_alloc() allocation can fail so add > a check to prevent a NULL dereference. Small allocations like this > can't actually fail in current kernels, but adding a check is very > simple and makes the static checkers happy. > > Fixes: 6637e11e4ad2 ("crypto: rsa - allow only odd e and restrict value in FIPS mode") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > crypto/rsa.c | 2 ++ > 1 file changed, 2 insertions(+) Nack. Please fix the static checker instead. Thanks,
On Mon, Oct 30, 2023 at 05:11:33PM +0800, Herbert Xu wrote: > On Mon, Oct 30, 2023 at 12:02:59PM +0300, Dan Carpenter wrote: > > Static checkers insist that the mpi_alloc() allocation can fail so add > > a check to prevent a NULL dereference. Small allocations like this > > can't actually fail in current kernels, but adding a check is very > > simple and makes the static checkers happy. > > > > Fixes: 6637e11e4ad2 ("crypto: rsa - allow only odd e and restrict value in FIPS mode") > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > > --- > > crypto/rsa.c | 2 ++ > > 1 file changed, 2 insertions(+) > > Nack. Please fix the static checker instead. The checker is not wrong. Kernel policy is that every allocation has to be checked for failure. In *current* kernels it won't but we have discussed changing this so the policy makes sense. I have tested this some years back and I don't think it's so infeasible as people think to allow these allocations to fail. regards, dan carpenter
On Mon, Oct 30, 2023 at 12:15:19PM +0300, Dan Carpenter wrote: > On Mon, Oct 30, 2023 at 05:11:33PM +0800, Herbert Xu wrote: > > On Mon, Oct 30, 2023 at 12:02:59PM +0300, Dan Carpenter wrote: > > > Static checkers insist that the mpi_alloc() allocation can fail so add > > > a check to prevent a NULL dereference. Small allocations like this > > > can't actually fail in current kernels, but adding a check is very > > > simple and makes the static checkers happy. > > > > > > Fixes: 6637e11e4ad2 ("crypto: rsa - allow only odd e and restrict value in FIPS mode") > > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > > > --- > > > crypto/rsa.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > Nack. Please fix the static checker instead. > > The checker is not wrong. Kernel policy is that every allocation has > to be checked for failure. In *current* kernels it won't but we have > discussed changing this so the policy makes sense. One way to fix the code would be to take gfp parameter and let people pass a GFP_NOFAIL. Unless there is a GFP_NOFAIL then the policy is that the allocation can fail. Or you could take the brtfs v1 approach and follow every kmalloc() with a: p = kmalloc(); BUG_on(!p); regards, dan carpenter
On Mon, Oct 30, 2023 at 12:15:19PM +0300, Dan Carpenter wrote: > > The checker is not wrong. Kernel policy is that every allocation has > to be checked for failure. In *current* kernels it won't but we have > discussed changing this so the policy makes sense. Nevermind, I misread it as kmalloc(0). mpi_alloc(0) can indeed fail because it's not allocating nothing. I'll take this patch. Thanks,
On Mon, Oct 30, 2023 at 12:02:59PM +0300, Dan Carpenter wrote: > Static checkers insist that the mpi_alloc() allocation can fail so add > a check to prevent a NULL dereference. Small allocations like this > can't actually fail in current kernels, but adding a check is very > simple and makes the static checkers happy. > > Fixes: 6637e11e4ad2 ("crypto: rsa - allow only odd e and restrict value in FIPS mode") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > crypto/rsa.c | 2 ++ > 1 file changed, 2 insertions(+) Patch applied. Thanks.
diff --git a/crypto/rsa.c b/crypto/rsa.c index c79613cdce6e..b9cd11fb7d36 100644 --- a/crypto/rsa.c +++ b/crypto/rsa.c @@ -220,6 +220,8 @@ static int rsa_check_exponent_fips(MPI e) } e_max = mpi_alloc(0); + if (!e_max) + return -ENOMEM; mpi_set_bit(e_max, 256); if (mpi_cmp(e, e_max) >= 0) {
Static checkers insist that the mpi_alloc() allocation can fail so add a check to prevent a NULL dereference. Small allocations like this can't actually fail in current kernels, but adding a check is very simple and makes the static checkers happy. Fixes: 6637e11e4ad2 ("crypto: rsa - allow only odd e and restrict value in FIPS mode") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- crypto/rsa.c | 2 ++ 1 file changed, 2 insertions(+)