2020-09-30 13:04:11

by Colin King

Subject: [PATCH][next][resend] lib/mpi: fix off-by-one check on index "no"

From: Colin Ian King <[email protected]>

There is an off-by-one range check on the upper limit of
index "no". Fix this by changing the > comparison to >=

Addresses-Coverity: ("Out-of-bounds read")
Fixes: a8ea8bdd9df9 ("lib/mpi: Extend the MPI library")
Signed-off-by: Colin Ian King <[email protected]>
---

resend to Cc linux-crypto

---
lib/mpi/mpiutil.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/mpi/mpiutil.c b/lib/mpi/mpiutil.c
index 3c63710c20c6..632d0a4bf93f 100644
--- a/lib/mpi/mpiutil.c
+++ b/lib/mpi/mpiutil.c
@@ -69,7 +69,7 @@ postcore_initcall(mpi_init);
*/
MPI mpi_const(enum gcry_mpi_constants no)
{
- if ((int)no < 0 || no > MPI_NUMBER_OF_CONSTANTS)
+ if ((int)no < 0 || no >= MPI_NUMBER_OF_CONSTANTS)
pr_err("MPI: invalid mpi_const selector %d\n", no);
if (!constants[no])
pr_err("MPI: MPI subsystem not initialized\n");
--
2.27.0

2020-09-30 14:37:04

by Ondrej Mosnáček

Subject: Re: [PATCH][next][resend] lib/mpi: fix off-by-one check on index "no"

st 30. 9. 2020 o 15:04 Colin King <[email protected]> napísal(a):
>
> From: Colin Ian King <[email protected]>
>
> There is an off-by-one range check on the upper limit of
> index "no". Fix this by changing the > comparison to >=

Note that this doesn't completely fix the bug though... (see below)

>
> Addresses-Coverity: ("Out-of-bounds read")
> Fixes: a8ea8bdd9df9 ("lib/mpi: Extend the MPI library")
> Signed-off-by: Colin Ian King <[email protected]>
> ---
>
> resend to Cc linux-crypto
>
> ---
> lib/mpi/mpiutil.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/mpi/mpiutil.c b/lib/mpi/mpiutil.c
> index 3c63710c20c6..632d0a4bf93f 100644
> --- a/lib/mpi/mpiutil.c
> +++ b/lib/mpi/mpiutil.c
> @@ -69,7 +69,7 @@ postcore_initcall(mpi_init);
> */
> MPI mpi_const(enum gcry_mpi_constants no)
> {
> - if ((int)no < 0 || no > MPI_NUMBER_OF_CONSTANTS)
> + if ((int)no < 0 || no >= MPI_NUMBER_OF_CONSTANTS)
> pr_err("MPI: invalid mpi_const selector %d\n", no);

What the code does is it just logs an error if the value is out of
range, but then it happily continues dereferencing the array anyway...
In the original libgcrypt code [1] (which BTW needs this patch, too),
there is log_bug() instead of pr_err(), which doesn't just log the
error, but also abort()'s the program. BUG() would be the correct
kernel equivalent for log_bug(). It seems the whole kernel's MPI
library clone should be re-audited for other instances of pr_*()'s
that should in fact be BUG()'s (or even better, WARN_ONCE()'s with
proper error handling, but that might diverge the code from libgcrypt
too much...).

[1] https://github.com/gpg/libgcrypt/blob/9cd92ebae21900e54cc3d8b607c8ed1afbf2eb9b/mpi/mpiutil.c#L773

> if (!constants[no])
> pr_err("MPI: MPI subsystem not initialized\n");
> --
> 2.27.0
>

2020-09-30 16:05:43

by Ard Biesheuvel

Subject: Re: [PATCH][next][resend] lib/mpi: fix off-by-one check on index "no"

On Wed, 30 Sep 2020 at 16:36, Ondrej Mosnáček <[email protected]> wrote:
>
> st 30. 9. 2020 o 15:04 Colin King <[email protected]> napísal(a):
> >
> > From: Colin Ian King <[email protected]>
> >
> > There is an off-by-one range check on the upper limit of
> > index "no". Fix this by changing the > comparison to >=
>
> Note that this doesn't completely fix the bug though... (see below)
>

And by the same reasoning, it does not address the coverity issue
either, since the out of bounds read is still executed.


> >
> > Addresses-Coverity: ("Out-of-bounds read")
> > Fixes: a8ea8bdd9df9 ("lib/mpi: Extend the MPI library")
> > Signed-off-by: Colin Ian King <[email protected]>
> > ---
> >
> > resend to Cc linux-crypto
> >
> > ---
> > lib/mpi/mpiutil.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/mpi/mpiutil.c b/lib/mpi/mpiutil.c
> > index 3c63710c20c6..632d0a4bf93f 100644
> > --- a/lib/mpi/mpiutil.c
> > +++ b/lib/mpi/mpiutil.c
> > @@ -69,7 +69,7 @@ postcore_initcall(mpi_init);
> > */
> > MPI mpi_const(enum gcry_mpi_constants no)
> > {
> > - if ((int)no < 0 || no > MPI_NUMBER_OF_CONSTANTS)
> > + if ((int)no < 0 || no >= MPI_NUMBER_OF_CONSTANTS)
> > pr_err("MPI: invalid mpi_const selector %d\n", no);
>
> What the code does is it just logs an error if the value is out of
> range, but then it happily continues dereferencing the array anyway...
> In the original libgcrypt code [1] (which BTW needs this patch, too),
> there is log_bug() instead of pr_err(), which doesn't just log the
> error, but also abort()'s the program. BUG() would be the correct
> kernel equivalent for log_bug(). It seems the whole kernel's MPI
> library clone should be re-audited for other instances of pr_*()'s
> that should in fact be BUG()'s (or even better, WARN_ONCE()'s with
> proper error handling, but that might diverge the code from libgcrypt
> too much...).
>
> [1] https://github.com/gpg/libgcrypt/blob/9cd92ebae21900e54cc3d8b607c8ed1afbf2eb9b/mpi/mpiutil.c#L773
>
> > if (!constants[no])
> > pr_err("MPI: MPI subsystem not initialized\n");
> > --
> > 2.27.0
> >