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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAMj1kXF2AzFkcyWzUHagq4xD_4BHpiscrPurndmJmJyp2KgXJg@mail.gmail.com>
Date:   Wed, 30 Sep 2020 18:03:33 +0200
From:   Ard Biesheuvel <ardb@...nel.org>
To:     Ondrej Mosnáček <omosnacek@...il.com>
Cc:     Colin King <colin.king@...onical.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Johannes Weiner <hannes@...xchg.org>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Tianjia Zhang <tianjia.zhang@...ux.alibaba.com>,
        Waiman Long <longman@...hat.com>,
        kernel-janitors@...r.kernel.org,
        Linux Crypto Mailing List <linux-crypto@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
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 <omosnacek@...il.com> wrote:
>
> st 30. 9. 2020 o 15:04 Colin King <colin.king@...onical.com> napísal(a):
> >
> > From: Colin Ian King <colin.king@...onical.com>
> >
> > 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@...onical.com>
> > ---
> >
> > 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
> >