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

[Ondrej Mosnáček]

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)

>
> 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
>