Re: [PATCH net] sctp: make sctp_setsockopt_events() less strict about the option length

[Julien Gomes]

On 2/6/19 1:48 PM, Julien Gomes wrote:
> 
> 
> On 2/6/19 1:39 PM, Neil Horman wrote:
>> On Wed, Feb 06, 2019 at 01:26:55PM -0800, Julien Gomes wrote:
>>>
>>>
>>> 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.
>> Copy_from_user should be safe to copy an arbitrary amount, the only restriction
>> is that optlen can't exceed the size of the buffer receiving the data in the
>> kernel.  From that standpoint your patch is safe.  However,  that exposes the
>> problem of checking any tail data on the userspace buffer.  That is to say, if
>> you want to ensure that any extra data that gets sent from userspace isn't
>> 'set', you would have to copy that extra data in consumable chunks and check
>> them individaully, and that screams DOS to me (i.e. imagine a user passing in a
>> 4GB buffer, and having to wait for the kernel to copy each X sized chunk,
>> looking for non-zero values).
> 
> There probably is a decent compromise to find between "not accepting a
> single additional byte" and accepting several GB.
> For example how likely is it that the growth of this structure make it
> go over a page? I would hope not at all.
> 
> By choosing a large but decent high limit, I think we can find a
> future-compatible compromise that doesn't rely on a preliminary
> getsockopt() just for structure trucation decision...

And I was just reminded about huge pages.
But still, my point of finding a compromise still stands.

> 
>>
>>> 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@xxxxxxxxxx>
>>>>>>> ---
>>>>>>>  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
>>>
>