Message-ID: <274c48cb-7590-682b-f4de-f5c4ce2d2144@arista.com>
Date:   Wed, 6 Feb 2019 13:26:55 -0800
From:   Julien Gomes <julien@...sta.com>
To:     Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
Cc:     netdev@...r.kernel.org, linux-sctp@...r.kernel.org,
        linux-kernel@...r.kernel.org, davem@...emloft.net,
        nhorman@...driver.com, vyasevich@...il.com, lucien.xin@...il.com
Subject: Re: [PATCH net] sctp: make sctp_setsockopt_events() less strict about
 the option length



On 2/6/19 1:07 PM, Marcelo Ricardo Leitner wrote:
> On Wed, Feb 06, 2019 at 12:48:38PM -0800, Julien Gomes wrote:
>>
>>
>> On 2/6/19 12:37 PM, Marcelo Ricardo Leitner wrote:
>>> On Wed, Feb 06, 2019 at 12:14:30PM -0800, Julien Gomes wrote:
>>>> Make sctp_setsockopt_events() able to accept sctp_event_subscribe
>>>> structures longer than the current definitions.
>>>>
>>>> This should prevent unjustified setsockopt() failures due to struct
>>>> sctp_event_subscribe extensions (as in 4.11 and 4.12) when using
>>>> binaries that should be compatible, but were built with later kernel
>>>> uapi headers.
>>>
>>> Not sure if we support backwards compatibility like this?
>>>
>>> My issue with this change is that by doing this, application will have
>>> no clue if the new bits were ignored or not and it may think that an
>>> event is enabled while it is not.
>>>
>>> A workaround would be to do a getsockopt and check the size that was
>>> returned. But then, it might as well use the right struct here in the
>>> first place.
>>>
>>> I'm seeing current implementation as an implicitly versioned argument:
>>> it will always accept setsockopt calls with an old struct (v4.11 or
>>> v4.12), but if the user tries to use v3 on a v1-only system, it will
>>> be rejected. Pretty much like using a newer setsockopt on an old
>>> system.
>>
>> With the current implementation, given sources that say are supposed to
>> run on a 4.9 kernel (no use of any newer field added in 4.11 or 4.12),
>> we can't rebuild the exact same sources on a 4.19 kernel and still run
>> them on 4.9 without messing with structures re-definition.
> 
> Maybe what we want(ed) here then is explicit versioning, to have the 3
> definitions available. Then the application is able to use, say struct
> sctp_event_subscribe, and be happy with it, while there is struct
> sctp_event_subscribe_v2 and struct sctp_event_subscribe_v3 there too.
> 
> But it's too late for that now because that would break applications
> already using the new fields in sctp_event_subscribe.

Right.

> 
>>
>> I understand your point, but this still looks like a sort of uapi
>> breakage to me.
> 
> Not disagreeing. I really just don't know how supported that is.
> Willing to know so I can pay more attention to this on future changes.
> 
> Btw, is this the only occurrence?

Can't really say, this is one I witnessed, I haven't really looked for
others.

> 
>>
>>
>> I also had another way to work-around this in mind, by copying optlen
>> bytes and checking that any additional field (not included in the
>> "current" kernel structure definition) is not set, returning EINVAL in
>> such case to keep a similar to current behavior.
>> The issue with this is that I didn't find a suitable (ie not totally
>> arbitrary such as "twice the existing structure size") upper limit to
>> optlen.
> 
> Seems interesting. Why would it need that upper limit to optlen?
> 
> Say struct v1 had 4 bytes, v3 now had 12. The user supplies 12 bytes
> to the kernel that only knows about 4 bytes. It can check that (12-4)
> bytes in the end, make sure no bit is on and use only the first 4.
> 
> The fact that it was 12 or 200 shouldn't matter, should it? As long as
> the (200-4) bytes are 0'ed, only the first 4 will be used and it
> should be ok, otherwise EINVAL. No need to know how big the current
> current actually is because it wouldn't be validating that here: just
> that it can safely use the first 4 bytes.

The upper limit concern is more regarding the call to copy_from_user
with an unrestricted/unchecked value.
I am not sure of how much of a risk/how exploitable this could be,
that's why I cautiously wanted to limit it in the first place just in case.

> 
>>
>>>
>>>>
>>>> Signed-off-by: Julien Gomes <julien@...sta.com>
>>>> ---
>>>>  net/sctp/socket.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>>>> index 9644bdc8e85c..f9717e2789da 100644
>>>> --- a/net/sctp/socket.c
>>>> +++ b/net/sctp/socket.c
>>>> @@ -2311,7 +2311,7 @@ static int sctp_setsockopt_events(struct sock *sk, char __user *optval,
>>>>  	int i;
>>>>  
>>>>  	if (optlen > sizeof(struct sctp_event_subscribe))
>>>> -		return -EINVAL;
>>>> +		optlen = sizeof(struct sctp_event_subscribe);
>>>>  
>>>>  	if (copy_from_user(&subscribe, optval, optlen))
>>>>  		return -EFAULT;
>>>> -- 
>>>> 2.20.1
>>>>
>>

-- 
Julien Gomes

Powered by blists - more mailing lists