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

[Ard Biesheuvel]

On Wed, 30 Sep 2020 at 16:36, Ondrej Mosnáček <omosnacek@xxxxxxxxx> wrote:
>
> st 30. 9. 2020 o 15:04 Colin King <colin.king@xxxxxxxxxxxxx> napísal(a):
> >
> > From: Colin Ian King <colin.king@xxxxxxxxxxxxx>
> >
> > 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 <colin.king@xxxxxxxxxxxxx>
> > ---
> >
> > 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
> >