[PATCH] net/sctp: Validate parameter size for SCTP_GET_ASSOC_STATS control message

[PATCH] net/sctp: Validate parameter size for SCTP_GET_ASSOC_STATS control message

[Guenter Roeck]

Building sctp may fail with:

In function ‘copy_from_user’,
    inlined from ‘sctp_getsockopt_assoc_stats’ at
    net/sctp/socket.c:5656:20:
arch/x86/include/asm/uaccess_32.h:211:26: error: call to
    ‘copy_from_user_overflow’ declared with attribute error: copy_from_user()
    buffer size is not provably correct

if built with W=1 due to a missing parameter size validation.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 net/sctp/socket.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index cedd9bf..0a5f2bf 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -5652,6 +5652,8 @@ static int sctp_getsockopt_assoc_stats(struct sock *sk, int len,
 	/* User must provide at least the assoc id */
 	if (len < sizeof(sctp_assoc_t))
 		return -EINVAL;
+	if (len > sizeof(struct sctp_assoc_stats))
+		len = sizeof(struct sctp_assoc_stats);
 
 	if (copy_from_user(&sas, optval, len))
 		return -EFAULT;
-- 
1.7.9.7

[PATCH] net/sctp: Validate parameter size for SCTP_GET_ASSOC_STATS control message

[Guenter Roeck]

Building sctp may fail with:

In function ‘copy_from_user’,
    inlined from ‘sctp_getsockopt_assoc_stats’ at
    net/sctp/socket.c:5656:20:
arch/x86/include/asm/uaccess_32.h:211:26: error: call to
    ‘copy_from_user_overflow’ declared with attribute error: copy_from_user()
    buffer size is not provably correct

if built with W=1 due to a missing parameter size validation.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 net/sctp/socket.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index cedd9bf..0a5f2bf 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -5652,6 +5652,8 @@ static int sctp_getsockopt_assoc_stats(struct sock *sk, int len,
 	/* User must provide at least the assoc id */
 	if (len < sizeof(sctp_assoc_t))
 		return -EINVAL;
+	if (len > sizeof(struct sctp_assoc_stats))
+		len = sizeof(struct sctp_assoc_stats);
 
 	if (copy_from_user(&sas, optval, len))
 		return -EFAULT;
-- 
1.7.9.7

Re: [PATCH] net/sctp: Validate parameter size for SCTP_GET_ASSOC_STATS control message

[Neil Horman]

On Wed, Feb 27, 2013 at 11:43:51AM -0800, Guenter Roeck wrote:
> Building sctp may fail with:
> 
> In function ‘copy_from_user’,
>     inlined from ‘sctp_getsockopt_assoc_stats’ at
>     net/sctp/socket.c:5656:20:
> arch/x86/include/asm/uaccess_32.h:211:26: error: call to
>     ‘copy_from_user_overflow’ declared with attribute error: copy_from_user()
>     buffer size is not provably correct
> 
> if built with W=1 due to a missing parameter size validation.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  net/sctp/socket.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index cedd9bf..0a5f2bf 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -5652,6 +5652,8 @@ static int sctp_getsockopt_assoc_stats(struct sock *sk, int len,
>  	/* User must provide at least the assoc id */
>  	if (len < sizeof(sctp_assoc_t))
>  		return -EINVAL;
> +	if (len > sizeof(struct sctp_assoc_stats))
> +		len = sizeof(struct sctp_assoc_stats);
>  
>  	if (copy_from_user(&sas, optval, len))
>  		return -EFAULT;
> -- 
> 1.7.9.7
> 
> 

Theres more than that going on here.  This will fix the warning, but the
function is written such that, if you pass in a size that is greater than the
size of a struct sctp_association, but less than a struct sctp_assoc_stats.  I'm
not sure that a partial stat struct is really that useful to people.  What if
you were to check for max(struct sctp_association, struct sctp_assoc_stats) as
your minimum length check, then just did a copy_from_user of that length.  It
would save you having to compute two lengths separately, since you could then
just do a copy_to_user(...,sizeof(struct sctp_assoc_stats), at the bottom of
that function.

Thanks!
Neil

Re: [PATCH] net/sctp: Validate parameter size for SCTP_GET_ASSOC_STATS control message

[Neil Horman]

On Wed, Feb 27, 2013 at 11:43:51AM -0800, Guenter Roeck wrote:
> Building sctp may fail with:
> 
> In function ‘copy_from_user’,
>     inlined from ‘sctp_getsockopt_assoc_stats’ at
>     net/sctp/socket.c:5656:20:
> arch/x86/include/asm/uaccess_32.h:211:26: error: call to
>     ‘copy_from_user_overflow’ declared with attribute error: copy_from_user()
>     buffer size is not provably correct
> 
> if built with W=1 due to a missing parameter size validation.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  net/sctp/socket.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index cedd9bf..0a5f2bf 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -5652,6 +5652,8 @@ static int sctp_getsockopt_assoc_stats(struct sock *sk, int len,
>  	/* User must provide at least the assoc id */
>  	if (len < sizeof(sctp_assoc_t))
>  		return -EINVAL;
> +	if (len > sizeof(struct sctp_assoc_stats))
> +		len = sizeof(struct sctp_assoc_stats);
>  
>  	if (copy_from_user(&sas, optval, len))
>  		return -EFAULT;
> -- 
> 1.7.9.7
> 
> 

Theres more than that going on here.  This will fix the warning, but the
function is written such that, if you pass in a size that is greater than the
size of a struct sctp_association, but less than a struct sctp_assoc_stats.  I'm
not sure that a partial stat struct is really that useful to people.  What if
you were to check for max(struct sctp_association, struct sctp_assoc_stats) as
your minimum length check, then just did a copy_from_user of that length.  It
would save you having to compute two lengths separately, since you could then
just do a copy_to_user(...,sizeof(struct sctp_assoc_stats), at the bottom of
that function.

Thanks!
Neil

Re: [PATCH] net/sctp: Validate parameter size for SCTP_GET_ASSOC_STATS control message

[David Miller]

From: Neil Horman <nhorman@tuxdriver.com>
Date: Wed, 27 Feb 2013 15:09:31 -0500

> On Wed, Feb 27, 2013 at 11:43:51AM -0800, Guenter Roeck wrote:
>> Building sctp may fail with:
>> 
>> In function ‘copy_from_user’,
>>     inlined from ‘sctp_getsockopt_assoc_stats’ at
>>     net/sctp/socket.c:5656:20:
>> arch/x86/include/asm/uaccess_32.h:211:26: error: call to
>>     ‘copy_from_user_overflow’ declared with attribute error: copy_from_user()
>>     buffer size is not provably correct
>> 
>> if built with W=1 due to a missing parameter size validation.
>> 
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>>  net/sctp/socket.c |    2 ++
>>  1 file changed, 2 insertions(+)
>> 
>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>> index cedd9bf..0a5f2bf 100644
>> --- a/net/sctp/socket.c
>> +++ b/net/sctp/socket.c
>> @@ -5652,6 +5652,8 @@ static int sctp_getsockopt_assoc_stats(struct sock *sk, int len,
>>  	/* User must provide at least the assoc id */
>>  	if (len < sizeof(sctp_assoc_t))
>>  		return -EINVAL;
>> +	if (len > sizeof(struct sctp_assoc_stats))
>> +		len = sizeof(struct sctp_assoc_stats);
>>  
>>  	if (copy_from_user(&sas, optval, len))
>>  		return -EFAULT;
>> -- 
>> 1.7.9.7
>> 
>> 
> 
> Theres more than that going on here.  This will fix the warning, but the
> function is written such that, if you pass in a size that is greater than the
> size of a struct sctp_association, but less than a struct sctp_assoc_stats.  I'm
> not sure that a partial stat struct is really that useful to people.  What if
> you were to check for max(struct sctp_association, struct sctp_assoc_stats) as
> your minimum length check, then just did a copy_from_user of that length.  It
> would save you having to compute two lengths separately, since you could then
> just do a copy_to_user(...,sizeof(struct sctp_assoc_stats), at the bottom of
> that function.

Genreally, getsockopt() implementations happily give partial return values
when the user gives a too small length.

Re: [PATCH] net/sctp: Validate parameter size for SCTP_GET_ASSOC_STATS control message

[David Miller]

RnJvbTogTmVpbCBIb3JtYW4gPG5ob3JtYW5AdHV4ZHJpdmVyLmNvbT4NCkRhdGU6IFdlZCwgMjcg
RmViIDIwMTMgMTU6MDk6MzEgLTA1MDANCg0KPiBPbiBXZWQsIEZlYiAyNywgMjAxMyBhdCAxMTo0
Mzo1MUFNIC0wODAwLCBHdWVudGVyIFJvZWNrIHdyb3RlOg0KPj4gQnVpbGRpbmcgc2N0cCBtYXkg
ZmFpbCB3aXRoOg0KPj4gDQo+PiBJbiBmdW5jdGlvbiChY29weV9mcm9tX3VzZXKiLA0KPj4gICAg
IGlubGluZWQgZnJvbSChc2N0cF9nZXRzb2Nrb3B0X2Fzc29jX3N0YXRzoiBhdA0KPj4gICAgIG5l
dC9zY3RwL3NvY2tldC5jOjU2NTY6MjA6DQo+PiBhcmNoL3g4Ni9pbmNsdWRlL2FzbS91YWNjZXNz
XzMyLmg6MjExOjI2OiBlcnJvcjogY2FsbCB0bw0KPj4gICAgIKFjb3B5X2Zyb21fdXNlcl9vdmVy
Zmxvd6IgZGVjbGFyZWQgd2l0aCBhdHRyaWJ1dGUgZXJyb3I6IGNvcHlfZnJvbV91c2VyKCkNCj4+
ICAgICBidWZmZXIgc2l6ZSBpcyBub3QgcHJvdmFibHkgY29ycmVjdA0KPj4gDQo+PiBpZiBidWls
dCB3aXRoIFc9MSBkdWUgdG8gYSBtaXNzaW5nIHBhcmFtZXRlciBzaXplIHZhbGlkYXRpb24uDQo+
PiANCj4+IFNpZ25lZC1vZmYtYnk6IEd1ZW50ZXIgUm9lY2sgPGxpbnV4QHJvZWNrLXVzLm5ldD4N
Cj4+IC0tLQ0KPj4gIG5ldC9zY3RwL3NvY2tldC5jIHwgICAgMiArKw0KPj4gIDEgZmlsZSBjaGFu
Z2VkLCAyIGluc2VydGlvbnMoKykNCj4+IA0KPj4gZGlmZiAtLWdpdCBhL25ldC9zY3RwL3NvY2tl
dC5jIGIvbmV0L3NjdHAvc29ja2V0LmMNCj4+IGluZGV4IGNlZGQ5YmYuLjBhNWYyYmYgMTAwNjQ0
DQo+PiAtLS0gYS9uZXQvc2N0cC9zb2NrZXQuYw0KPj4gKysrIGIvbmV0L3NjdHAvc29ja2V0LmMN
Cj4+IEBAIC01NjUyLDYgKzU2NTIsOCBAQCBzdGF0aWMgaW50IHNjdHBfZ2V0c29ja29wdF9hc3Nv
Y19zdGF0cyhzdHJ1Y3Qgc29jayAqc2ssIGludCBsZW4sDQo+PiAgCS8qIFVzZXIgbXVzdCBwcm92
aWRlIGF0IGxlYXN0IHRoZSBhc3NvYyBpZCAqLw0KPj4gIAlpZiAobGVuIDwgc2l6ZW9mKHNjdHBf
YXNzb2NfdCkpDQo+PiAgCQlyZXR1cm4gLUVJTlZBTDsNCj4+ICsJaWYgKGxlbiA+IHNpemVvZihz
dHJ1Y3Qgc2N0cF9hc3NvY19zdGF0cykpDQo+PiArCQlsZW4gPSBzaXplb2Yoc3RydWN0IHNjdHBf
YXNzb2Nfc3RhdHMpOw0KPj4gIA0KPj4gIAlpZiAoY29weV9mcm9tX3VzZXIoJnNhcywgb3B0dmFs
LCBsZW4pKQ0KPj4gIAkJcmV0dXJuIC1FRkFVTFQ7DQo+PiAtLSANCj4+IDEuNy45LjcNCj4+IA0K
Pj4gDQo+IA0KPiBUaGVyZXMgbW9yZSB0aGFuIHRoYXQgZ29pbmcgb24gaGVyZS4gIFRoaXMgd2ls
bCBmaXggdGhlIHdhcm5pbmcsIGJ1dCB0aGUNCj4gZnVuY3Rpb24gaXMgd3JpdHRlbiBzdWNoIHRo
YXQsIGlmIHlvdSBwYXNzIGluIGEgc2l6ZSB0aGF0IGlzIGdyZWF0ZXIgdGhhbiB0aGUNCj4gc2l6
ZSBvZiBhIHN0cnVjdCBzY3RwX2Fzc29jaWF0aW9uLCBidXQgbGVzcyB0aGFuIGEgc3RydWN0IHNj
dHBfYXNzb2Nfc3RhdHMuICBJJ20NCj4gbm90IHN1cmUgdGhhdCBhIHBhcnRpYWwgc3RhdCBzdHJ1
Y3QgaXMgcmVhbGx5IHRoYXQgdXNlZnVsIHRvIHBlb3BsZS4gIFdoYXQgaWYNCj4geW91IHdlcmUg
dG8gY2hlY2sgZm9yIG1heChzdHJ1Y3Qgc2N0cF9hc3NvY2lhdGlvbiwgc3RydWN0IHNjdHBfYXNz
b2Nfc3RhdHMpIGFzDQo+IHlvdXIgbWluaW11bSBsZW5ndGggY2hlY2ssIHRoZW4ganVzdCBkaWQg
YSBjb3B5X2Zyb21fdXNlciBvZiB0aGF0IGxlbmd0aC4gIEl0DQo+IHdvdWxkIHNhdmUgeW91IGhh
dmluZyB0byBjb21wdXRlIHR3byBsZW5ndGhzIHNlcGFyYXRlbHksIHNpbmNlIHlvdSBjb3VsZCB0
aGVuDQo+IGp1c3QgZG8gYSBjb3B5X3RvX3VzZXIoLi4uLHNpemVvZihzdHJ1Y3Qgc2N0cF9hc3Nv
Y19zdGF0cyksIGF0IHRoZSBib3R0b20gb2YNCj4gdGhhdCBmdW5jdGlvbi4NCg0KR2VucmVhbGx5
LCBnZXRzb2Nrb3B0KCkgaW1wbGVtZW50YXRpb25zIGhhcHBpbHkgZ2l2ZSBwYXJ0aWFsIHJldHVy
biB2YWx1ZXMNCndoZW4gdGhlIHVzZXIgZ2l2ZXMgYSB0b28gc21hbGwgbGVuZ3RoLg0K

Re: [PATCH] net/sctp: Validate parameter size for SCTP_GET_ASSOC_STATS control message

[Guenter Roeck]

On Wed, Feb 27, 2013 at 03:09:31PM -0500, Neil Horman wrote:
> On Wed, Feb 27, 2013 at 11:43:51AM -0800, Guenter Roeck wrote:
> > Building sctp may fail with:
> > 
> > In function ‘copy_from_user’,
> >     inlined from ‘sctp_getsockopt_assoc_stats’ at
> >     net/sctp/socket.c:5656:20:
> > arch/x86/include/asm/uaccess_32.h:211:26: error: call to
> >     ‘copy_from_user_overflow’ declared with attribute error: copy_from_user()
> >     buffer size is not provably correct
> > 
> > if built with W=1 due to a missing parameter size validation.
> > 
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > ---
> >  net/sctp/socket.c |    2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > index cedd9bf..0a5f2bf 100644
> > --- a/net/sctp/socket.c
> > +++ b/net/sctp/socket.c
> > @@ -5652,6 +5652,8 @@ static int sctp_getsockopt_assoc_stats(struct sock *sk, int len,
> >  	/* User must provide at least the assoc id */
> >  	if (len < sizeof(sctp_assoc_t))
> >  		return -EINVAL;
> > +	if (len > sizeof(struct sctp_assoc_stats))
> > +		len = sizeof(struct sctp_assoc_stats);
> >  
> >  	if (copy_from_user(&sas, optval, len))
> >  		return -EFAULT;
> > -- 
> > 1.7.9.7
> > 
> > 
> 
> Theres more than that going on here.  This will fix the warning, but the
> function is written such that, if you pass in a size that is greater than the
> size of a struct sctp_association, but less than a struct sctp_assoc_stats.  I'm
> not sure that a partial stat struct is really that useful to people.  What if
> you were to check for max(struct sctp_association, struct sctp_assoc_stats) as
> your minimum length check, then just did a copy_from_user of that length.  It
> would save you having to compute two lengths separately, since you could then
> just do a copy_to_user(...,sizeof(struct sctp_assoc_stats), at the bottom of
> that function.
> 
Yes, but that would require input from someone who knows the code. All I am trying
to accomplish is to ensure that copy_from_user does not overwrite the stack.

Thanks,
Guenter

Re: [PATCH] net/sctp: Validate parameter size for SCTP_GET_ASSOC_STATS control message

[Guenter Roeck]

On Wed, Feb 27, 2013 at 03:09:31PM -0500, Neil Horman wrote:
> On Wed, Feb 27, 2013 at 11:43:51AM -0800, Guenter Roeck wrote:
> > Building sctp may fail with:
> > 
> > In function ‘copy_from_user’,
> >     inlined from ‘sctp_getsockopt_assoc_stats’ at
> >     net/sctp/socket.c:5656:20:
> > arch/x86/include/asm/uaccess_32.h:211:26: error: call to
> >     ‘copy_from_user_overflow’ declared with attribute error: copy_from_user()
> >     buffer size is not provably correct
> > 
> > if built with W=1 due to a missing parameter size validation.
> > 
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > ---
> >  net/sctp/socket.c |    2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > index cedd9bf..0a5f2bf 100644
> > --- a/net/sctp/socket.c
> > +++ b/net/sctp/socket.c
> > @@ -5652,6 +5652,8 @@ static int sctp_getsockopt_assoc_stats(struct sock *sk, int len,
> >  	/* User must provide at least the assoc id */
> >  	if (len < sizeof(sctp_assoc_t))
> >  		return -EINVAL;
> > +	if (len > sizeof(struct sctp_assoc_stats))
> > +		len = sizeof(struct sctp_assoc_stats);
> >  
> >  	if (copy_from_user(&sas, optval, len))
> >  		return -EFAULT;
> > -- 
> > 1.7.9.7
> > 
> > 
> 
> Theres more than that going on here.  This will fix the warning, but the
> function is written such that, if you pass in a size that is greater than the
> size of a struct sctp_association, but less than a struct sctp_assoc_stats.  I'm
> not sure that a partial stat struct is really that useful to people.  What if
> you were to check for max(struct sctp_association, struct sctp_assoc_stats) as
> your minimum length check, then just did a copy_from_user of that length.  It
> would save you having to compute two lengths separately, since you could then
> just do a copy_to_user(...,sizeof(struct sctp_assoc_stats), at the bottom of
> that function.
> 
Yes, but that would require input from someone who knows the code. All I am trying
to accomplish is to ensure that copy_from_user does not overwrite the stack.

Thanks,
Guenter

Re: [PATCH] net/sctp: Validate parameter size for SCTP_GET_ASSOC_STATS control message

[David Miller]

From: Guenter Roeck <linux@roeck-us.net>
Date: Wed, 27 Feb 2013 11:43:51 -0800

> Building sctp may fail with:
> 
> In function ‘copy_from_user’,
>     inlined from ‘sctp_getsockopt_assoc_stats’ at
>     net/sctp/socket.c:5656:20:
> arch/x86/include/asm/uaccess_32.h:211:26: error: call to
>     ‘copy_from_user_overflow’ declared with attribute error: copy_from_user()
>     buffer size is not provably correct
> 
> if built with W=1 due to a missing parameter size validation.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

This change is correct, but please fix this by simply moving the:

	/* Allow the struct to grow and fill in as much as possible */
	len = min_t(size_t, len, sizeof(sas));

line higher up in the function.

And I also prefer this because:

	something testing sizeof(foo);
	if (copy_from_user(..., ..., sizeof(foo)))

must easier to audit and validate, especially in patch form.

Otherwise I have to bring the code into an editor and read the whole
function just to make sure you got the type correct.

Thanks.

Re: [PATCH] net/sctp: Validate parameter size for SCTP_GET_ASSOC_STATS control message

[David Miller]

RnJvbTogR3VlbnRlciBSb2VjayA8bGludXhAcm9lY2stdXMubmV0Pg0KRGF0ZTogV2VkLCAyNyBG
ZWIgMjAxMyAxMTo0Mzo1MSAtMDgwMA0KDQo+IEJ1aWxkaW5nIHNjdHAgbWF5IGZhaWwgd2l0aDoN
Cj4gDQo+IEluIGZ1bmN0aW9uIKFjb3B5X2Zyb21fdXNlcqIsDQo+ICAgICBpbmxpbmVkIGZyb20g
oXNjdHBfZ2V0c29ja29wdF9hc3NvY19zdGF0c6IgYXQNCj4gICAgIG5ldC9zY3RwL3NvY2tldC5j
OjU2NTY6MjA6DQo+IGFyY2gveDg2L2luY2x1ZGUvYXNtL3VhY2Nlc3NfMzIuaDoyMTE6MjY6IGVy
cm9yOiBjYWxsIHRvDQo+ICAgICChY29weV9mcm9tX3VzZXJfb3ZlcmZsb3eiIGRlY2xhcmVkIHdp
dGggYXR0cmlidXRlIGVycm9yOiBjb3B5X2Zyb21fdXNlcigpDQo+ICAgICBidWZmZXIgc2l6ZSBp
cyBub3QgcHJvdmFibHkgY29ycmVjdA0KPiANCj4gaWYgYnVpbHQgd2l0aCBXPTEgZHVlIHRvIGEg
bWlzc2luZyBwYXJhbWV0ZXIgc2l6ZSB2YWxpZGF0aW9uLg0KPiANCj4gU2lnbmVkLW9mZi1ieTog
R3VlbnRlciBSb2VjayA8bGludXhAcm9lY2stdXMubmV0Pg0KDQpUaGlzIGNoYW5nZSBpcyBjb3Jy
ZWN0LCBidXQgcGxlYXNlIGZpeCB0aGlzIGJ5IHNpbXBseSBtb3ZpbmcgdGhlOg0KDQoJLyogQWxs
b3cgdGhlIHN0cnVjdCB0byBncm93IGFuZCBmaWxsIGluIGFzIG11Y2ggYXMgcG9zc2libGUgKi8N
CglsZW4gPSBtaW5fdChzaXplX3QsIGxlbiwgc2l6ZW9mKHNhcykpOw0KDQpsaW5lIGhpZ2hlciB1
cCBpbiB0aGUgZnVuY3Rpb24uDQoNCkFuZCBJIGFsc28gcHJlZmVyIHRoaXMgYmVjYXVzZToNCg0K
CXNvbWV0aGluZyB0ZXN0aW5nIHNpemVvZihmb28pOw0KCWlmIChjb3B5X2Zyb21fdXNlciguLi4s
IC4uLiwgc2l6ZW9mKGZvbykpKQ0KDQptdXN0IGVhc2llciB0byBhdWRpdCBhbmQgdmFsaWRhdGUs
IGVzcGVjaWFsbHkgaW4gcGF0Y2ggZm9ybS4NCg0KT3RoZXJ3aXNlIEkgaGF2ZSB0byBicmluZyB0
aGUgY29kZSBpbnRvIGFuIGVkaXRvciBhbmQgcmVhZCB0aGUgd2hvbGUNCmZ1bmN0aW9uIGp1c3Qg
dG8gbWFrZSBzdXJlIHlvdSBnb3QgdGhlIHR5cGUgY29ycmVjdC4NCg0KVGhhbmtzLg0K

Re: [PATCH] net/sctp: Validate parameter size for SCTP_GET_ASSOC_STATS control message

[Vlad Yasevich]

On 02/27/2013 03:22 PM, Guenter Roeck wrote:
> On Wed, Feb 27, 2013 at 03:09:31PM -0500, Neil Horman wrote:
>> On Wed, Feb 27, 2013 at 11:43:51AM -0800, Guenter Roeck wrote:
>>> Building sctp may fail with:
>>>
>>> In function ‘copy_from_user’,
>>>      inlined from ‘sctp_getsockopt_assoc_stats’ at
>>>      net/sctp/socket.c:5656:20:
>>> arch/x86/include/asm/uaccess_32.h:211:26: error: call to
>>>      ‘copy_from_user_overflow’ declared with attribute error: copy_from_user()
>>>      buffer size is not provably correct
>>>
>>> if built with W=1 due to a missing parameter size validation.
>>>
>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>> ---
>>>   net/sctp/socket.c |    2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>>> index cedd9bf..0a5f2bf 100644
>>> --- a/net/sctp/socket.c
>>> +++ b/net/sctp/socket.c
>>> @@ -5652,6 +5652,8 @@ static int sctp_getsockopt_assoc_stats(struct sock *sk, int len,
>>>   	/* User must provide at least the assoc id */
>>>   	if (len < sizeof(sctp_assoc_t))
>>>   		return -EINVAL;
>>> +	if (len > sizeof(struct sctp_assoc_stats))
>>> +		len = sizeof(struct sctp_assoc_stats);
>>>
>>>   	if (copy_from_user(&sas, optval, len))
>>>   		return -EFAULT;
>>> --
>>> 1.7.9.7
>>>
>>>
>>
>> Theres more than that going on here.  This will fix the warning, but the
>> function is written such that, if you pass in a size that is greater than the
>> size of a struct sctp_association, but less than a struct sctp_assoc_stats.  I'm
>> not sure that a partial stat struct is really that useful to people.  What if
>> you were to check for max(struct sctp_association, struct sctp_assoc_stats) as
>> your minimum length check, then just did a copy_from_user of that length.  It
>> would save you having to compute two lengths separately, since you could then
>> just do a copy_to_user(...,sizeof(struct sctp_assoc_stats), at the bottom of
>> that function.
>>
> Yes, but that would require input from someone who knows the code. All I am trying
> to accomplish is to ensure that copy_from_user does not overwrite the stack.
>

The function already has the following in it:

	/* Allow the struct to grow and fill in as much as possible */
         len = min_t(size_t, len, sizeof(sas));


It would be better to move that up to before the copy_from_user.
The alternative would be to do:
	if (copy_from_user(&sas, optval, sizeof(sctp_assoc_t))

since all we are after here is an association id and nothing else.

-vlad

> Thanks,
> Guenter
>

Re: [PATCH] net/sctp: Validate parameter size for SCTP_GET_ASSOC_STATS control message

[Vlad Yasevich]

On 02/27/2013 03:22 PM, Guenter Roeck wrote:
> On Wed, Feb 27, 2013 at 03:09:31PM -0500, Neil Horman wrote:
>> On Wed, Feb 27, 2013 at 11:43:51AM -0800, Guenter Roeck wrote:
>>> Building sctp may fail with:
>>>
>>> In function ‘copy_from_user’,
>>>      inlined from ‘sctp_getsockopt_assoc_stats’ at
>>>      net/sctp/socket.c:5656:20:
>>> arch/x86/include/asm/uaccess_32.h:211:26: error: call to
>>>      ‘copy_from_user_overflow’ declared with attribute error: copy_from_user()
>>>      buffer size is not provably correct
>>>
>>> if built with W=1 due to a missing parameter size validation.
>>>
>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>> ---
>>>   net/sctp/socket.c |    2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>>> index cedd9bf..0a5f2bf 100644
>>> --- a/net/sctp/socket.c
>>> +++ b/net/sctp/socket.c
>>> @@ -5652,6 +5652,8 @@ static int sctp_getsockopt_assoc_stats(struct sock *sk, int len,
>>>   	/* User must provide at least the assoc id */
>>>   	if (len < sizeof(sctp_assoc_t))
>>>   		return -EINVAL;
>>> +	if (len > sizeof(struct sctp_assoc_stats))
>>> +		len = sizeof(struct sctp_assoc_stats);
>>>
>>>   	if (copy_from_user(&sas, optval, len))
>>>   		return -EFAULT;
>>> --
>>> 1.7.9.7
>>>
>>>
>>
>> Theres more than that going on here.  This will fix the warning, but the
>> function is written such that, if you pass in a size that is greater than the
>> size of a struct sctp_association, but less than a struct sctp_assoc_stats.  I'm
>> not sure that a partial stat struct is really that useful to people.  What if
>> you were to check for max(struct sctp_association, struct sctp_assoc_stats) as
>> your minimum length check, then just did a copy_from_user of that length.  It
>> would save you having to compute two lengths separately, since you could then
>> just do a copy_to_user(...,sizeof(struct sctp_assoc_stats), at the bottom of
>> that function.
>>
> Yes, but that would require input from someone who knows the code. All I am trying
> to accomplish is to ensure that copy_from_user does not overwrite the stack.
>

The function already has the following in it:

	/* Allow the struct to grow and fill in as much as possible */
         len = min_t(size_t, len, sizeof(sas));


It would be better to move that up to before the copy_from_user.
The alternative would be to do:
	if (copy_from_user(&sas, optval, sizeof(sctp_assoc_t))

since all we are after here is an association id and nothing else.

-vlad

> Thanks,
> Guenter
>

Re: [PATCH] net/sctp: Validate parameter size for SCTP_GET_ASSOC_STATS control message

[Neil Horman]

On Wed, Feb 27, 2013 at 12:22:55PM -0800, Guenter Roeck wrote:
> On Wed, Feb 27, 2013 at 03:09:31PM -0500, Neil Horman wrote:
> > On Wed, Feb 27, 2013 at 11:43:51AM -0800, Guenter Roeck wrote:
> > > Building sctp may fail with:
> > > 
> > > In function ‘copy_from_user’,
> > >     inlined from ‘sctp_getsockopt_assoc_stats’ at
> > >     net/sctp/socket.c:5656:20:
> > > arch/x86/include/asm/uaccess_32.h:211:26: error: call to
> > >     ‘copy_from_user_overflow’ declared with attribute error: copy_from_user()
> > >     buffer size is not provably correct
> > > 
> > > if built with W=1 due to a missing parameter size validation.
> > > 
> > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > > ---
> > >  net/sctp/socket.c |    2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > > index cedd9bf..0a5f2bf 100644
> > > --- a/net/sctp/socket.c
> > > +++ b/net/sctp/socket.c
> > > @@ -5652,6 +5652,8 @@ static int sctp_getsockopt_assoc_stats(struct sock *sk, int len,
> > >  	/* User must provide at least the assoc id */
> > >  	if (len < sizeof(sctp_assoc_t))
> > >  		return -EINVAL;
> > > +	if (len > sizeof(struct sctp_assoc_stats))
> > > +		len = sizeof(struct sctp_assoc_stats);
> > >  
> > >  	if (copy_from_user(&sas, optval, len))
> > >  		return -EFAULT;
> > > -- 
> > > 1.7.9.7
> > > 
> > > 
> > 
> > Theres more than that going on here.  This will fix the warning, but the
> > function is written such that, if you pass in a size that is greater than the
> > size of a struct sctp_association, but less than a struct sctp_assoc_stats.  I'm
> > not sure that a partial stat struct is really that useful to people.  What if
> > you were to check for max(struct sctp_association, struct sctp_assoc_stats) as
> > your minimum length check, then just did a copy_from_user of that length.  It
> > would save you having to compute two lengths separately, since you could then
> > just do a copy_to_user(...,sizeof(struct sctp_assoc_stats), at the bottom of
> > that function.
> > 
> Yes, but that would require input from someone who knows the code. All I am trying
> to accomplish is to ensure that copy_from_user does not overwrite the stack.
> 
Not really, its pretty straightforward.  Although, its kind of moot, after Daves
note I looked again, and if the size of the stats structure is less than the
association structure, everything works fine, but if the association is smaller
than the association, we'll get an EFAULT on the copy to user.  So it all works
out

Acked-by: Neil Horman <nhorman@tuxdriver.com>


Dave
> Thanks,
> Guenter
> 

Re: [PATCH] net/sctp: Validate parameter size for SCTP_GET_ASSOC_STATS control message

[Neil Horman]

On Wed, Feb 27, 2013 at 12:22:55PM -0800, Guenter Roeck wrote:
> On Wed, Feb 27, 2013 at 03:09:31PM -0500, Neil Horman wrote:
> > On Wed, Feb 27, 2013 at 11:43:51AM -0800, Guenter Roeck wrote:
> > > Building sctp may fail with:
> > > 
> > > In function ‘copy_from_user’,
> > >     inlined from ‘sctp_getsockopt_assoc_stats’ at
> > >     net/sctp/socket.c:5656:20:
> > > arch/x86/include/asm/uaccess_32.h:211:26: error: call to
> > >     ‘copy_from_user_overflow’ declared with attribute error: copy_from_user()
> > >     buffer size is not provably correct
> > > 
> > > if built with W=1 due to a missing parameter size validation.
> > > 
> > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > > ---
> > >  net/sctp/socket.c |    2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > > index cedd9bf..0a5f2bf 100644
> > > --- a/net/sctp/socket.c
> > > +++ b/net/sctp/socket.c
> > > @@ -5652,6 +5652,8 @@ static int sctp_getsockopt_assoc_stats(struct sock *sk, int len,
> > >  	/* User must provide at least the assoc id */
> > >  	if (len < sizeof(sctp_assoc_t))
> > >  		return -EINVAL;
> > > +	if (len > sizeof(struct sctp_assoc_stats))
> > > +		len = sizeof(struct sctp_assoc_stats);
> > >  
> > >  	if (copy_from_user(&sas, optval, len))
> > >  		return -EFAULT;
> > > -- 
> > > 1.7.9.7
> > > 
> > > 
> > 
> > Theres more than that going on here.  This will fix the warning, but the
> > function is written such that, if you pass in a size that is greater than the
> > size of a struct sctp_association, but less than a struct sctp_assoc_stats.  I'm
> > not sure that a partial stat struct is really that useful to people.  What if
> > you were to check for max(struct sctp_association, struct sctp_assoc_stats) as
> > your minimum length check, then just did a copy_from_user of that length.  It
> > would save you having to compute two lengths separately, since you could then
> > just do a copy_to_user(...,sizeof(struct sctp_assoc_stats), at the bottom of
> > that function.
> > 
> Yes, but that would require input from someone who knows the code. All I am trying
> to accomplish is to ensure that copy_from_user does not overwrite the stack.
> 
Not really, its pretty straightforward.  Although, its kind of moot, after Daves
note I looked again, and if the size of the stats structure is less than the
association structure, everything works fine, but if the association is smaller
than the association, we'll get an EFAULT on the copy to user.  So it all works
out

Acked-by: Neil Horman <nhorman@tuxdriver.com>


Dave
> Thanks,
> Guenter
> 

Re: [PATCH] net/sctp: Validate parameter size for SCTP_GET_ASSOC_STATS control message

[Vlad Yasevich]

On 02/27/2013 03:33 PM, David Miller wrote:
> From: Guenter Roeck <linux@roeck-us.net>
> Date: Wed, 27 Feb 2013 11:43:51 -0800
>
>> Building sctp may fail with:
>>
>> In function ‘copy_from_user’,
>>      inlined from ‘sctp_getsockopt_assoc_stats’ at
>>      net/sctp/socket.c:5656:20:
>> arch/x86/include/asm/uaccess_32.h:211:26: error: call to
>>      ‘copy_from_user_overflow’ declared with attribute error: copy_from_user()
>>      buffer size is not provably correct
>>
>> if built with W=1 due to a missing parameter size validation.
>>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>
> This change is correct, but please fix this by simply moving the:
>
> 	/* Allow the struct to grow and fill in as much as possible */
> 	len = min_t(size_t, len, sizeof(sas));
>
> line higher up in the function.
>
> And I also prefer this because:
>
> 	something testing sizeof(foo);
> 	if (copy_from_user(..., ..., sizeof(foo)))
>
> must easier to audit and validate, especially in patch form.
>
> Otherwise I have to bring the code into an editor and read the whole
> function just to make sure you got the type correct.

Right.  In this particular case, we are after a very small portion of 
user data (just the association id) and copying the entire structure is
rather pointless.

A nicer solution here would be to do this:
	 if (copy_from_user(&sas, optval, sizeof(sctp_assoc_t))

We are already guaranteed that much space and we make sure that we don't 
overwrite the kernel stack.

-vlad

Re: [PATCH] net/sctp: Validate parameter size for SCTP_GET_ASSOC_STATS control message

[Vlad Yasevich]

On 02/27/2013 03:33 PM, David Miller wrote:
> From: Guenter Roeck <linux@roeck-us.net>
> Date: Wed, 27 Feb 2013 11:43:51 -0800
>
>> Building sctp may fail with:
>>
>> In function ¡copy_from_user¢,
>>      inlined from ¡sctp_getsockopt_assoc_stats¢ at
>>      net/sctp/socket.c:5656:20:
>> arch/x86/include/asm/uaccess_32.h:211:26: error: call to
>>      ¡copy_from_user_overflow¢ declared with attribute error: copy_from_user()
>>      buffer size is not provably correct
>>
>> if built with W=1 due to a missing parameter size validation.
>>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>
> This change is correct, but please fix this by simply moving the:
>
> 	/* Allow the struct to grow and fill in as much as possible */
> 	len = min_t(size_t, len, sizeof(sas));
>
> line higher up in the function.
>
> And I also prefer this because:
>
> 	something testing sizeof(foo);
> 	if (copy_from_user(..., ..., sizeof(foo)))
>
> must easier to audit and validate, especially in patch form.
>
> Otherwise I have to bring the code into an editor and read the whole
> function just to make sure you got the type correct.

Right.  In this particular case, we are after a very small portion of 
user data (just the association id) and copying the entire structure is
rather pointless.

A nicer solution here would be to do this:
	 if (copy_from_user(&sas, optval, sizeof(sctp_assoc_t))

We are already guaranteed that much space and we make sure that we don't 
overwrite the kernel stack.

-vlad