diff mbox series

[v5,1/2] lib/mpi: Fix buffer overrun when SG is too long

Message ID 20221227142740.2807136-2-roberto.sassu@huaweicloud.com
State New
Headers
Series KEYS: asymmetric: Copy sig and digest in public_key_verify_signature() |

Commit Message

Roberto Sassu Dec. 27, 2022, 2:27 p.m. UTC 
  From: Herbert Xu <herbert@gondor.apana.org.au>

The helper mpi_read_raw_from_sgl sets the number of entries in
the SG list according to nbytes.  However, if the last entry
in the SG list contains more data than nbytes, then it may overrun
the buffer because it only allocates enough memory for nbytes.

Fixes: 2d4d1eea540b ("lib/mpi: Add mpi sgl helpers")
Reported-by: Roberto Sassu <roberto.sassu@huaweicloud.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---
 lib/mpi/mpicoder.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Eric Biggers Dec. 29, 2022, 10:38 p.m. UTC | #1 
On Tue, Dec 27, 2022 at 03:27:39PM +0100, Roberto Sassu wrote:
> From: Herbert Xu <herbert@gondor.apana.org.au>
> 
> The helper mpi_read_raw_from_sgl sets the number of entries in
> the SG list according to nbytes.  However, if the last entry
> in the SG list contains more data than nbytes, then it may overrun
> the buffer because it only allocates enough memory for nbytes.
> 
> Fixes: 2d4d1eea540b ("lib/mpi: Add mpi sgl helpers")
> Reported-by: Roberto Sassu <roberto.sassu@huaweicloud.com>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> ---
>  lib/mpi/mpicoder.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Eric Biggers <ebiggers@google.com>

- Eric
David Laight Dec. 30, 2022, 1:35 p.m. UTC | #2 
From: Roberto Sassu
> Sent: 27 December 2022 14:28
> 
> From: Herbert Xu <herbert@gondor.apana.org.au>
> 
> The helper mpi_read_raw_from_sgl sets the number of entries in
> the SG list according to nbytes.  However, if the last entry
> in the SG list contains more data than nbytes, then it may overrun
> the buffer because it only allocates enough memory for nbytes.
> 
> Fixes: 2d4d1eea540b ("lib/mpi: Add mpi sgl helpers")
> Reported-by: Roberto Sassu <roberto.sassu@huaweicloud.com>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> ---
>  lib/mpi/mpicoder.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/mpi/mpicoder.c b/lib/mpi/mpicoder.c
> index 39c4c6731094..3cb6bd148fa9 100644
> --- a/lib/mpi/mpicoder.c
> +++ b/lib/mpi/mpicoder.c
> @@ -504,7 +504,8 @@ MPI mpi_read_raw_from_sgl(struct scatterlist *sgl, unsigned int nbytes)
> 
>  	while (sg_miter_next(&miter)) {
>  		buff = miter.addr;
> -		len = miter.length;
> +		len = min_t(unsigned, miter.length, nbytes);

Technically that min_t() is incorrect.
miter.length is size_t (unsigned long on 64bit) and nbytes unsigned int.
Any cast needs to force the smaller type to the larger one.
(Clearly here the domain of the values is probably than 4G - but that isn't
the point. There must be some places where the sg length needs to
be size_t because 32 bits isn't enough.)

In reality min() is being completely over-zealous in its checking and
should allow comparisons where the signed-ness of the two values matches.
Search for the patch I posted before xmas.

	David


> +		nbytes -= len;
> 
>  		for (x = 0; x < len; x++) {
>  			a <<= 8;
> --
> 2.25.1

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Herbert Xu Dec. 30, 2022, 3:39 p.m. UTC | #3 
On Fri, Dec 30, 2022 at 01:35:07PM +0000, David Laight wrote:
>
> miter.length is size_t (unsigned long on 64bit) and nbytes unsigned int.

miter.length is bounded by sg->length which is unsigned int.

Cheers,
David Laight Dec. 31, 2022, 1:23 p.m. UTC | #4 
From: Herbert Xu
> Sent: 30 December 2022 15:40
> 
> On Fri, Dec 30, 2022 at 01:35:07PM +0000, David Laight wrote:
> >
> > miter.length is size_t (unsigned long on 64bit) and nbytes unsigned int.
> 
> miter.length is bounded by sg->length which is unsigned int.

I did say 'technically' :-)

Should there be a sg_miter_stop() before the return at the bottom?
Care seems to have been taken to add one before an earlier error return.
(The logic in that function is very strange...)

Indeed other parts of the file are equally strange.
The big multi-line if-else in twocompl() is just:
	p[i] = (p[1] ^ 0xff) + 1;
or even:
	p[i] = -p[i];
That function could also return the 'zero status' to correct
for -0 (rather than the extra check earlier in the caller).

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Herbert Xu Jan. 6, 2023, 3:18 p.m. UTC | #5 
On Tue, Dec 27, 2022 at 03:27:39PM +0100, Roberto Sassu wrote:
> From: Herbert Xu <herbert@gondor.apana.org.au>
> 
> The helper mpi_read_raw_from_sgl sets the number of entries in
> the SG list according to nbytes.  However, if the last entry
> in the SG list contains more data than nbytes, then it may overrun
> the buffer because it only allocates enough memory for nbytes.
> 
> Fixes: 2d4d1eea540b ("lib/mpi: Add mpi sgl helpers")
> Reported-by: Roberto Sassu <roberto.sassu@huaweicloud.com>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> ---
>  lib/mpi/mpicoder.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Patch applied.  Thanks.
Roberto Sassu Jan. 16, 2023, 8:57 a.m. UTC | #6 
On Fri, 2023-01-06 at 23:18 +0800, Herbert Xu wrote:
> On Tue, Dec 27, 2022 at 03:27:39PM +0100, Roberto Sassu wrote:
> > From: Herbert Xu <herbert@gondor.apana.org.au>
> > 
> > The helper mpi_read_raw_from_sgl sets the number of entries in
> > the SG list according to nbytes.  However, if the last entry
> > in the SG list contains more data than nbytes, then it may overrun
> > the buffer because it only allocates enough memory for nbytes.
> > 
> > Fixes: 2d4d1eea540b ("lib/mpi: Add mpi sgl helpers")
> > Reported-by: Roberto Sassu <roberto.sassu@huaweicloud.com>
> > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> > ---
> >  lib/mpi/mpicoder.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> Patch applied.  Thanks.

Hi Herbert

will you take also the second patch?

Thanks

Roberto
Herbert Xu Jan. 16, 2023, 9:06 a.m. UTC | #7 
On Mon, Jan 16, 2023 at 09:57:57AM +0100, Roberto Sassu wrote:
>
> Hi Herbert
> 
> will you take also the second patch?

That's part of David Howells' tree so hopefully he will pick
it up soon.

Thanks,
Roberto Sassu Jan. 20, 2023, 10:23 a.m. UTC | #8 
On 1/16/2023 10:06 AM, Herbert Xu wrote:
> On Mon, Jan 16, 2023 at 09:57:57AM +0100, Roberto Sassu wrote:
>>
>> Hi Herbert
>>
>> will you take also the second patch?
> 
> That's part of David Howells' tree so hopefully he will pick
> it up soon.

Hi David

could you please take the second patch?

Thanks

Roberto
diff mbox series

Patch

diff --git a/lib/mpi/mpicoder.c b/lib/mpi/mpicoder.c
index 39c4c6731094..3cb6bd148fa9 100644
--- a/lib/mpi/mpicoder.c
+++ b/lib/mpi/mpicoder.c
@@ -504,7 +504,8 @@  MPI mpi_read_raw_from_sgl(struct scatterlist *sgl, unsigned int nbytes)
 
 	while (sg_miter_next(&miter)) {
 		buff = miter.addr;
-		len = miter.length;
+		len = min_t(unsigned, miter.length, nbytes);
+		nbytes -= len;
 
 		for (x = 0; x < len; x++) {
 			a <<= 8;