[PATCH 2.6.32-4.0] sg_start_req(): make sure that there's not too many elements in iovec

[PATCH 2.6.32-4.0] sg_start_req(): make sure that there's not too many elements in iovec

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1463 bytes --]

From: Al Viro <viro@zeniv.linux.org.uk>

commit 451a2886b6bf90e2fb378f7c46c655450fb96e81 upstream.

unfortunately, allowing an arbitrary 16bit value means a possibility of
overflow in the calculation of total number of pages in bio_map_user_iov() -
we rely on there being no more than PAGE_SIZE members of sum in the
first loop there.  If that sum wraps around, we end up allocating
too small array of pointers to pages and it's easy to overflow it in
the second loop.

X-Coverup: TINC (and there's no lumber cartel either)
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
[bwh: s/MAX_UIOVEC/UIO_MAXIOV/. This was fixed upstream by commit
 fdc81f45e9f5 ("sg_start_req(): use import_iovec()"), but we don't have
 that function.]
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
It looks like this bug was introduced in 2.6.28 by commit 10db10d144c0
("sg: convert the indirect IO path to use the block layer"), so the fix
is needed for all stable branches.

Ben.

 drivers/scsi/sg.c | 3 +++
 1 file changed, 3 insertions(+)

--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -1687,6 +1687,9 @@ static int sg_start_req(Sg_request *srp,
 			md->from_user = 0;
 	}
 
+	if (unlikely(iov_count > UIO_MAXIOV))
+		return -EINVAL;
+
 	if (iov_count) {
 		int len, size = sizeof(struct sg_iovec) * iov_count;
 		struct iovec *iov;
-- 
Ben Hutchings
One of the nice things about standards is that there are so many of them.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH 2.6.32-4.0] sg_start_req(): make sure that there's not too many elements in iovec

[Willy Tarreau]

On Sat, Aug 01, 2015 at 06:25:59PM +0100, Ben Hutchings wrote:
> From: Al Viro <viro@zeniv.linux.org.uk>
> 
> commit 451a2886b6bf90e2fb378f7c46c655450fb96e81 upstream.
(...)
> It looks like this bug was introduced in 2.6.28 by commit 10db10d144c0
> ("sg: convert the indirect IO path to use the block layer"), so the fix
> is needed for all stable branches.

Patch queued for 2.6.32, thanks Ben.

willy

Re: [PATCH 2.6.32-4.0] sg_start_req(): make sure that there's not too many elements in iovec

[Jiri Slaby]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On 08/01/2015, 07:25 PM, Ben Hutchings wrote:
> From: Al Viro <viro@zeniv.linux.org.uk>
> 
> commit 451a2886b6bf90e2fb378f7c46c655450fb96e81 upstream.
> 
> unfortunately, allowing an arbitrary 16bit value means a
> possibility of overflow in the calculation of total number of pages
> in bio_map_user_iov() - we rely on there being no more than
> PAGE_SIZE members of sum in the first loop there.  If that sum
> wraps around, we end up allocating too small array of pointers to
> pages and it's easy to overflow it in the second loop.
> 
> X-Coverup: TINC (and there's no lumber cartel either) 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> [bwh:
> s/MAX_UIOVEC/UIO_MAXIOV/. This was fixed upstream by commit 
> fdc81f45e9f5 ("sg_start_req(): use import_iovec()"), but we don't
> have that function.] Signed-off-by: Ben Hutchings
> <ben@decadent.org.uk> --- It looks like this bug was introduced in
> 2.6.28 by commit 10db10d144c0 ("sg: convert the indirect IO path to
> use the block layer"), so the fix is needed for all stable
> branches.

Thanks, now applied to 3.12.

- -- 
js
suse labs
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJVvzroAAoJEL0lsQQGtHBJYwcP/0/o5VbbC8kuAsnOVWBfhZ6k
6MmD4ubR6A39OR12Uj4AsoFR/0FsNCmoU5F74JCTJZENkmcn9VTg6VPmMmDh6QtR
6iGnIMD3zhZK1Z4Cj+6vmbnFHUMmsvwcIr+l3BV5GFT/cpxslgOzCwERmIpT0KuC
C1akB2hnC0LfQumy2g8PLYZB1XHYGJ4JPBRFpTGvVau/BzU8yutXY4XAzKa3WWmY
oCAKR4A6LV5uBgy7WFJbBFxaPRp8tpS8+be1TQvV1A+DzwStv/ckvfWW4TOBFIec
qFxxPsGjiVB+Et44tBIhqSLmcRCxYTGUKueeIhN32K+KVm3R9ag/XH+EVHAeMACw
2QZBRB20gGDMimhEFx50qcfJAuLQyug2pCMwGvWJ/3IRc0ovm3rm1jnFyXBd9f3f
Nz3TS5dp/wJ6uAQ5DQXypXB2pSWG6D6Pu5dS5ixCoCHCeip54NlYNt2LU/2dh6A/
j4iphf+DeXl/v+5ej9lLEgz/FsU742jXSBnZ4k9ubH2ktzLGYNI90Y3s+1RuuHGy
d9yyIeq8KGdxMJj7kw6N1x0mOSunAS7xjzPzzP5ZkwKnqIUTQlePbOnl3HFQBs3V
FhElv7BMK0jfkpi1tYxSKOwCqJlSPHAFjOigw/uKRfFTETFBEE02ejmJhF7iu7mT
a2iFj8Ur6z/i7WoK1HGQ
=Pg4U
-----END PGP SIGNATURE-----

Re: [PATCH 2.6.32-4.0] sg_start_req(): make sure that there's not too many elements in iovec

[Luis Henriques]

On Sat, Aug 01, 2015 at 06:25:59PM +0100, Ben Hutchings wrote:
> From: Al Viro <viro@zeniv.linux.org.uk>
> 
> commit 451a2886b6bf90e2fb378f7c46c655450fb96e81 upstream.
> 
> unfortunately, allowing an arbitrary 16bit value means a possibility of
> overflow in the calculation of total number of pages in bio_map_user_iov() -
> we rely on there being no more than PAGE_SIZE members of sum in the
> first loop there.  If that sum wraps around, we end up allocating
> too small array of pointers to pages and it's easy to overflow it in
> the second loop.
> 
> X-Coverup: TINC (and there's no lumber cartel either)
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> [bwh: s/MAX_UIOVEC/UIO_MAXIOV/. This was fixed upstream by commit
>  fdc81f45e9f5 ("sg_start_req(): use import_iovec()"), but we don't have
>  that function.]
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> ---
> It looks like this bug was introduced in 2.6.28 by commit 10db10d144c0
> ("sg: convert the indirect IO path to use the block layer"), so the fix
> is needed for all stable branches.
> 
> Ben.

Thanks Ben, queuing it for the 3.16 kernel.

Cheers,
--
Lu�s

> 
>  drivers/scsi/sg.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -1687,6 +1687,9 @@ static int sg_start_req(Sg_request *srp,
>  			md->from_user = 0;
>  	}
>  
> +	if (unlikely(iov_count > UIO_MAXIOV))
> +		return -EINVAL;
> +
>  	if (iov_count) {
>  		int len, size = sizeof(struct sg_iovec) * iov_count;
>  		struct iovec *iov;
> -- 
> Ben Hutchings
> One of the nice things about standards is that there are so many of them.
>