From: Julien Gomes <julien@arista.com>
To: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Cc: netdev@vger.kernel.org, linux-sctp@vger.kernel.org,
linux-kernel@vger.kernel.org, davem@davemloft.net,
nhorman@tuxdriver.com, vyasevich@gmail.com, lucien.xin@gmail.com
Subject: Re: [PATCH net] sctp: make sctp_setsockopt_events() less strict about the option length
Date: Wed, 6 Feb 2019 13:26:55 -0800 [thread overview]
Message-ID: <274c48cb-7590-682b-f4de-f5c4ce2d2144@arista.com> (raw)
In-Reply-To: <20190206210723.GD13621@localhost.localdomain>
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@arista.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
next prev parent reply other threads:[~2019-02-06 21:27 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-06 20:14 [PATCH net] sctp: make sctp_setsockopt_events() less strict about the option length Julien Gomes
2019-02-06 20:37 ` Marcelo Ricardo Leitner
2019-02-06 20:48 ` Julien Gomes
2019-02-06 21:07 ` Marcelo Ricardo Leitner
2019-02-06 21:23 ` Neil Horman
2019-02-06 21:48 ` Julien Gomes
2019-02-07 14:44 ` Neil Horman
2019-02-06 21:26 ` Julien Gomes [this message]
2019-02-06 21:39 ` Neil Horman
2019-02-06 21:48 ` Julien Gomes
2019-02-06 21:53 ` Julien Gomes
2019-02-07 14:48 ` Neil Horman
2019-02-07 17:33 ` David Laight
2019-02-07 17:47 ` 'Marcelo Ricardo Leitner'
2019-02-08 9:53 ` David Laight
2019-02-08 12:36 ` Neil Horman
2019-02-06 21:08 ` Neil Horman
2019-02-06 21:18 ` Marcelo Ricardo Leitner
2019-02-09 23:12 ` David Miller
2019-02-10 12:46 ` Marcelo Ricardo Leitner
2019-02-10 20:15 ` Marcelo Ricardo Leitner
2019-02-13 16:17 ` David Laight
2019-02-13 17:23 ` 'Marcelo Ricardo Leitner'
2019-02-11 15:04 ` Neil Horman
2019-02-11 17:05 ` Marcelo Ricardo Leitner
2019-02-06 20:49 ` Neil Horman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=274c48cb-7590-682b-f4de-f5c4ce2d2144@arista.com \
--to=julien@arista.com \
--cc=davem@davemloft.net \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-sctp@vger.kernel.org \
--cc=lucien.xin@gmail.com \
--cc=marcelo.leitner@gmail.com \
--cc=netdev@vger.kernel.org \
--cc=nhorman@tuxdriver.com \
--cc=vyasevich@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).