* [PATCH][next][resend] lib/mpi: fix off-by-one check on index "no"
@ 2020-09-30 13:01 ` Colin King
0 siblings, 0 replies; 6+ messages in thread
From: Colin King @ 2020-09-30 13:01 UTC (permalink / raw)
To: Andrew Morton, Johannes Weiner, Herbert Xu, Tianjia Zhang, Waiman Long
Cc: kernel-janitors, linux-crypto, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
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 <colin.king@canonical.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);
if (!constants[no])
pr_err("MPI: MPI subsystem not initialized\n");
--
2.27.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH][next][resend] lib/mpi: fix off-by-one check on index "no"
@ 2020-09-30 13:01 ` Colin King
0 siblings, 0 replies; 6+ messages in thread
From: Colin King @ 2020-09-30 13:01 UTC (permalink / raw)
To: Andrew Morton, Johannes Weiner, Herbert Xu, Tianjia Zhang, Waiman Long
Cc: kernel-janitors, linux-crypto, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
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 <colin.king@canonical.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);
if (!constants[no])
pr_err("MPI: MPI subsystem not initialized\n");
--
2.27.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH][next][resend] lib/mpi: fix off-by-one check on index "no"
2020-09-30 13:01 ` Colin King
@ 2020-09-30 14:35 ` Ondrej Mosnáček
-1 siblings, 0 replies; 6+ messages in thread
From: Ondrej Mosnáček @ 2020-09-30 14:35 UTC (permalink / raw)
To: Colin King
Cc: Andrew Morton, Johannes Weiner, Herbert Xu, Tianjia Zhang,
Waiman Long, kernel-janitors, linux-crypto,
Linux Kernel Mailing List
st 30. 9. 2020 o 15:04 Colin King <colin.king@canonical.com> napísal(a):
>
> From: Colin Ian King <colin.king@canonical.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)
>
> Addresses-Coverity: ("Out-of-bounds read")
> Fixes: a8ea8bdd9df9 ("lib/mpi: Extend the MPI library")
> Signed-off-by: Colin Ian King <colin.king@canonical.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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][next][resend] lib/mpi: fix off-by-one check on index "no"
@ 2020-09-30 14:35 ` Ondrej Mosnáček
0 siblings, 0 replies; 6+ messages in thread
From: Ondrej Mosnáček @ 2020-09-30 14:35 UTC (permalink / raw)
To: Colin King
Cc: Andrew Morton, Johannes Weiner, Herbert Xu, Tianjia Zhang,
Waiman Long, kernel-janitors, linux-crypto,
Linux Kernel Mailing List
st 30. 9. 2020 o 15:04 Colin King <colin.king@canonical.com> napÃsal(a):
>
> From: Colin Ian King <colin.king@canonical.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)
>
> Addresses-Coverity: ("Out-of-bounds read")
> Fixes: a8ea8bdd9df9 ("lib/mpi: Extend the MPI library")
> Signed-off-by: Colin Ian King <colin.king@canonical.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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][next][resend] lib/mpi: fix off-by-one check on index "no"
2020-09-30 14:35 ` Ondrej Mosnáček
@ 2020-09-30 16:03 ` Ard Biesheuvel
-1 siblings, 0 replies; 6+ messages in thread
From: Ard Biesheuvel @ 2020-09-30 16:03 UTC (permalink / raw)
To: Ondrej Mosnáček
Cc: Colin King, Andrew Morton, Johannes Weiner, Herbert Xu,
Tianjia Zhang, Waiman Long, kernel-janitors,
Linux Crypto Mailing List, Linux Kernel Mailing List
On Wed, 30 Sep 2020 at 16:36, Ondrej Mosnáček <omosnacek@gmail.com> wrote:
>
> st 30. 9. 2020 o 15:04 Colin King <colin.king@canonical.com> napísal(a):
> >
> > From: Colin Ian King <colin.king@canonical.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@canonical.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
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][next][resend] lib/mpi: fix off-by-one check on index "no"
@ 2020-09-30 16:03 ` Ard Biesheuvel
0 siblings, 0 replies; 6+ messages in thread
From: Ard Biesheuvel @ 2020-09-30 16:03 UTC (permalink / raw)
To: Ondrej Mosnáček
Cc: Colin King, Andrew Morton, Johannes Weiner, Herbert Xu,
Tianjia Zhang, Waiman Long, kernel-janitors,
Linux Crypto Mailing List, Linux Kernel Mailing List
On Wed, 30 Sep 2020 at 16:36, Ondrej Mosnáček <omosnacek@gmail.com> wrote:
>
> st 30. 9. 2020 o 15:04 Colin King <colin.king@canonical.com> napísal(a):
> >
> > From: Colin Ian King <colin.king@canonical.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@canonical.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
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-09-30 16:03 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-30 13:01 [PATCH][next][resend] lib/mpi: fix off-by-one check on index "no" Colin King
2020-09-30 13:01 ` Colin King
2020-09-30 14:35 ` Ondrej Mosnáček
2020-09-30 14:35 ` Ondrej Mosnáček
2020-09-30 16:03 ` Ard Biesheuvel
2020-09-30 16:03 ` Ard Biesheuvel
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.