1. Patchew
  2. linux
  3. View series

[PATCH] lib/mpi: Fix buffer overrun when SG is too long

Herbert Xu posted 1 patch 2 years, 9 months ago
There is a newer version of this series
[PATCH] lib/mpi: Fix buffer overrun when SG is too long
Posted by Herbert Xu 2 years, 9 months ago 
On Mon, Dec 19, 2022 at 09:49:29AM +0100, Roberto Sassu wrote:
>
> do you have any news on this bug?

Thanks for the reminder.  Could you please try this patch?

---8<---
The helper mpi_read_raw_from_sgl ignores the second parameter
nbytes when reading the SG list and may overrun its own buffer
because it only allocates enough memory according to 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>

diff --git a/lib/mpi/mpicoder.c b/lib/mpi/mpicoder.c
index 39c4c6731094..6bffc68c1a5a 100644
--- a/lib/mpi/mpicoder.c
+++ b/lib/mpi/mpicoder.c
@@ -494,17 +494,15 @@ MPI mpi_read_raw_from_sgl(struct scatterlist *sgl, unsigned int nbytes)
 	val->sign = 0;
 	val->nlimbs = nlimbs;
 
-	if (nbytes == 0)
-		return val;
-
 	j = nlimbs - 1;
 	a = 0;
 	z = BYTES_PER_MPI_LIMB - nbytes % BYTES_PER_MPI_LIMB;
 	z %= BYTES_PER_MPI_LIMB;
 
-	while (sg_miter_next(&miter)) {
+	while (nbytes && 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;
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] lib/mpi: Fix buffer overrun when SG is too long
Posted by Roberto Sassu 2 years, 9 months ago 
On 12/20/2022 8:24 AM, Herbert Xu wrote:
> On Mon, Dec 19, 2022 at 09:49:29AM +0100, Roberto Sassu wrote:
>>
>> do you have any news on this bug?
> 
> Thanks for the reminder.  Could you please try this patch?

Tried, could not boot the UML kernel.

After looking, it seems we have to call sg_miter_stop(). Or 
alternatively, we could let sg_miter_next() be called but not writing 
anything inside the loop.

With either of those fixes, the tests pass (using one scatterlist).

Roberto

> ---8<---
> The helper mpi_read_raw_from_sgl ignores the second parameter
> nbytes when reading the SG list and may overrun its own buffer
> because it only allocates enough memory according to 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>
> 
> diff --git a/lib/mpi/mpicoder.c b/lib/mpi/mpicoder.c
> index 39c4c6731094..6bffc68c1a5a 100644
> --- a/lib/mpi/mpicoder.c
> +++ b/lib/mpi/mpicoder.c
> @@ -494,17 +494,15 @@ MPI mpi_read_raw_from_sgl(struct scatterlist *sgl, unsigned int nbytes)
>   	val->sign = 0;
>   	val->nlimbs = nlimbs;
>   
> -	if (nbytes == 0)
> -		return val;
> -
>   	j = nlimbs - 1;
>   	a = 0;
>   	z = BYTES_PER_MPI_LIMB - nbytes % BYTES_PER_MPI_LIMB;
>   	z %= BYTES_PER_MPI_LIMB;
>   
> -	while (sg_miter_next(&miter)) {
> +	while (nbytes && 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;
Re: [PATCH] lib/mpi: Fix buffer overrun when SG is too long
Posted by Eric Biggers 2 years, 9 months ago 
On Tue, Dec 20, 2022 at 11:36:50AM +0100, Roberto Sassu wrote:
> On 12/20/2022 8:24 AM, Herbert Xu wrote:
> > On Mon, Dec 19, 2022 at 09:49:29AM +0100, Roberto Sassu wrote:
> > > 
> > > do you have any news on this bug?
> > 
> > Thanks for the reminder.  Could you please try this patch?
> 
> Tried, could not boot the UML kernel.
> 
> After looking, it seems we have to call sg_miter_stop(). Or alternatively,
> we could let sg_miter_next() be called but not writing anything inside the
> loop.
> 
> With either of those fixes, the tests pass (using one scatterlist).
> 
> Roberto
> 
> > ---8<---
> > The helper mpi_read_raw_from_sgl ignores the second parameter
> > nbytes when reading the SG list and may overrun its own buffer
> > because it only allocates enough memory according to 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>
> > 
> > diff --git a/lib/mpi/mpicoder.c b/lib/mpi/mpicoder.c
> > index 39c4c6731094..6bffc68c1a5a 100644
> > --- a/lib/mpi/mpicoder.c
> > +++ b/lib/mpi/mpicoder.c
> > @@ -494,17 +494,15 @@ MPI mpi_read_raw_from_sgl(struct scatterlist *sgl, unsigned int nbytes)
> >   	val->sign = 0;
> >   	val->nlimbs = nlimbs;
> > -	if (nbytes == 0)
> > -		return val;
> > -
> >   	j = nlimbs - 1;
> >   	a = 0;
> >   	z = BYTES_PER_MPI_LIMB - nbytes % BYTES_PER_MPI_LIMB;
> >   	z %= BYTES_PER_MPI_LIMB;
> > -	while (sg_miter_next(&miter)) {
> > +	while (nbytes && 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;

I think it should look like:

	while (nbytes) {
		sg_miter_next(&miter);
		...
	}
	sg_miter_stop(&miter);

- Eric
[v2 PATCH] lib/mpi: Fix buffer overrun when SG is too long
Posted by Herbert Xu 2 years, 9 months ago 
On Tue, Dec 20, 2022 at 08:30:16PM +0000, Eric Biggers wrote:
>
> > Tried, could not boot the UML kernel.
> > 
> > After looking, it seems we have to call sg_miter_stop(). Or alternatively,
> > we could let sg_miter_next() be called but not writing anything inside the
> > loop.
> > 
> > With either of those fixes, the tests pass (using one scatterlist).

Thanks for the quick feedback Roberto!

> I think it should look like:
> 
> 	while (nbytes) {
> 		sg_miter_next(&miter);
> 		...
> 	}
> 	sg_miter_stop(&miter);

You're right Eric.  However, we could also do it by simply not
checking nbytes since we already set nents according to nbytes
at the top of the function.

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

diff --git a/lib/mpi/mpicoder.c b/lib/mpi/mpicoder.c
index 39c4c6731094..157ef532a6a2 100644
--- a/lib/mpi/mpicoder.c
+++ b/lib/mpi/mpicoder.c
@@ -504,7 +501,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;
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [v2 PATCH] lib/mpi: Fix buffer overrun when SG is too long
Posted by Eric Biggers 2 years, 8 months ago 
On Wed, Dec 21, 2022 at 02:53:58PM +0800, Herbert Xu wrote:
> On Tue, Dec 20, 2022 at 08:30:16PM +0000, Eric Biggers wrote:
> >
> > > Tried, could not boot the UML kernel.
> > > 
> > > After looking, it seems we have to call sg_miter_stop(). Or alternatively,
> > > we could let sg_miter_next() be called but not writing anything inside the
> > > loop.
> > > 
> > > With either of those fixes, the tests pass (using one scatterlist).
> 
> Thanks for the quick feedback Roberto!
> 
> > I think it should look like:
> > 
> > 	while (nbytes) {
> > 		sg_miter_next(&miter);
> > 		...
> > 	}
> > 	sg_miter_stop(&miter);
> 
> You're right Eric.  However, we could also do it by simply not
> checking nbytes since we already set nents according to nbytes
> at the top of the function.
> 
> ---8<---
> 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>
> 
> diff --git a/lib/mpi/mpicoder.c b/lib/mpi/mpicoder.c
> index 39c4c6731094..157ef532a6a2 100644
> --- a/lib/mpi/mpicoder.c
> +++ b/lib/mpi/mpicoder.c
> @@ -504,7 +501,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;

That's fine, I guess.  One quirk of the above approach is that if the last
needed element of the scatterlist has a lot of extra pages, this will iterate
through all those extra pages, processing 0 bytes from each.  It could just stop
when done.  I suppose it's not worth worrying about that case, though.

- Eric
Re: [v2 PATCH] lib/mpi: Fix buffer overrun when SG is too long
Posted by Herbert Xu 2 years, 8 months ago 
On Wed, Dec 21, 2022 at 12:53:29PM -0800, Eric Biggers wrote:
>
> That's fine, I guess.  One quirk of the above approach is that if the last
> needed element of the scatterlist has a lot of extra pages, this will iterate
> through all those extra pages, processing 0 bytes from each.  It could just stop
> when done.  I suppose it's not worth worrying about that case, though.

Ideally this should be handled in the sg_miter interface, IOW,
it should allow us to cap the SG list at a certain number of bytes
as opposed to a certain number of entries.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.