[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

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

[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

[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

[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