* Re: [PATCH net] sctp: make sctp_setsockopt_events() less strict about the option length
@ 2019-02-06 21:07 ` Marcelo Ricardo Leitner
0 siblings, 0 replies; 51+ messages in thread
From: Marcelo Ricardo Leitner @ 2019-02-06 21:07 UTC (permalink / raw)
To: Julien Gomes
Cc: netdev, linux-sctp, linux-kernel, davem, nhorman, vyasevich, lucien.xin
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.
>
> 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?
>
>
> 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.
>
> >
> >>
> >> 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
> >>
>
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH net] sctp: make sctp_setsockopt_events() less strict about the option length
2019-02-06 21:07 ` Marcelo Ricardo Leitner
@ 2019-02-06 21:23 ` Neil Horman
-1 siblings, 0 replies; 51+ messages in thread
From: Neil Horman @ 2019-02-06 21:23 UTC (permalink / raw)
To: Marcelo Ricardo Leitner
Cc: Julien Gomes, netdev, linux-sctp, linux-kernel, davem, vyasevich,
lucien.xin
On Wed, Feb 06, 2019 at 07:07:23PM -0200, 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.
>
Yeah, I'm not supportive of codifying that knoweldge in the kernel. If we were
to support bi-directional versioning, I would encode it into lksctp-tools rather
than the kernel.
> >
> > 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?
>
No, I think there are a few others (maybe paddrparams?)
> >
> >
> > 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?
>
I think the thought was to differentiate between someone passing a legit larger
structure from a newer UAPI, from someone just passing in a massive
inappropriately sized buffer (even if the return on both is the same).
> 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.
>
I'm less than excited about making the kernel check an unbounded user space
buffer, thats seems like a potential DOS attack from an unpriviledged user to
me. I'm also still hung up on the notion that, despite how we do this, this
patch is going into the latest kernel, so it will only work on a kernel that
already understands the most recent set of subscriptions. It would work if we,
again someday in the future extended this struct, someone built against that
newer UAPI, and then tried to run it on a kernel that had this patch.
FWIW, there is an existing implied method to determine the available
subscription events. sctp_getsockopt_events does clamp the size of the output
buffer, and returns that information in the optlen field via put_user. An
application that was build against UAPIs from 4.19 could pass in the 4.19
sctp_event_subscribe struct to sctp_getsockopt_events, and read the output
length, whcih would inform the application of the events that the kernel is
capable of reporting, and limit itself to only using those events. Its not a
perfect solution, but its direct, understandable and portable.
Neil
> >
> > >
> > >>
> > >> 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
> > >>
> >
>
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH net] sctp: make sctp_setsockopt_events() less strict about the option length
@ 2019-02-06 21:23 ` Neil Horman
0 siblings, 0 replies; 51+ messages in thread
From: Neil Horman @ 2019-02-06 21:23 UTC (permalink / raw)
To: Marcelo Ricardo Leitner
Cc: Julien Gomes, netdev, linux-sctp, linux-kernel, davem, vyasevich,
lucien.xin
On Wed, Feb 06, 2019 at 07:07:23PM -0200, 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.
>
Yeah, I'm not supportive of codifying that knoweldge in the kernel. If we were
to support bi-directional versioning, I would encode it into lksctp-tools rather
than the kernel.
> >
> > 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?
>
No, I think there are a few others (maybe paddrparams?)
> >
> >
> > 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?
>
I think the thought was to differentiate between someone passing a legit larger
structure from a newer UAPI, from someone just passing in a massive
inappropriately sized buffer (even if the return on both is the same).
> 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.
>
I'm less than excited about making the kernel check an unbounded user space
buffer, thats seems like a potential DOS attack from an unpriviledged user to
me. I'm also still hung up on the notion that, despite how we do this, this
patch is going into the latest kernel, so it will only work on a kernel that
already understands the most recent set of subscriptions. It would work if we,
again someday in the future extended this struct, someone built against that
newer UAPI, and then tried to run it on a kernel that had this patch.
FWIW, there is an existing implied method to determine the available
subscription events. sctp_getsockopt_events does clamp the size of the output
buffer, and returns that information in the optlen field via put_user. An
application that was build against UAPIs from 4.19 could pass in the 4.19
sctp_event_subscribe struct to sctp_getsockopt_events, and read the output
length, whcih would inform the application of the events that the kernel is
capable of reporting, and limit itself to only using those events. Its not a
perfect solution, but its direct, understandable and portable.
Neil
> >
> > >
> > >>
> > >> 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
> > >>
> >
>
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH net] sctp: make sctp_setsockopt_events() less strict about the option length
2019-02-06 21:23 ` Neil Horman
@ 2019-02-06 21:48 ` Julien Gomes
-1 siblings, 0 replies; 51+ messages in thread
From: Julien Gomes @ 2019-02-06 21:48 UTC (permalink / raw)
To: Neil Horman, Marcelo Ricardo Leitner
Cc: netdev, linux-sctp, linux-kernel, davem, vyasevich, lucien.xin
On 2/6/19 1:23 PM, Neil Horman wrote:
> On Wed, Feb 06, 2019 at 07:07:23PM -0200, 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.
>>
> Yeah, I'm not supportive of codifying that knoweldge in the kernel. If we were
> to support bi-directional versioning, I would encode it into lksctp-tools rather
> than the kernel.
I'm not sure that forcing a library on users is a good reason to break UAPI.
>
>>>
>>> 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?
>>
> No, I think there are a few others (maybe paddrparams?)
>
>>>
>>>
>>> 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?
>>
> I think the thought was to differentiate between someone passing a legit larger
> structure from a newer UAPI, from someone just passing in a massive
> inappropriately sized buffer (even if the return on both is the same).
>
>> 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.
>>
> I'm less than excited about making the kernel check an unbounded user space
> buffer, thats seems like a potential DOS attack from an unpriviledged user to
> me. I'm also still hung up on the notion that, despite how we do this, this
> patch is going into the latest kernel, so it will only work on a kernel that
> already understands the most recent set of subscriptions. It would work if we,
> again someday in the future extended this struct, someone built against that
> newer UAPI, and then tried to run it on a kernel that had this patch.
The patch is going into the latest, but can also be backported on future
stables.
I don't think "not fixing it because it's not fixed yet" is a good
reason to keep things the way they are. But maybe that's just me.
Given that the structure has already been extended several times, there
is pretty much nothing to keep this from happening again and again.
>
> FWIW, there is an existing implied method to determine the available
> subscription events. sctp_getsockopt_events does clamp the size of the output
> buffer, and returns that information in the optlen field via put_user. An
> application that was build against UAPIs from 4.19 could pass in the 4.19
> sctp_event_subscribe struct to sctp_getsockopt_events, and read the output
> length, whcih would inform the application of the events that the kernel is
> capable of reporting, and limit itself to only using those events. Its not a
> perfect solution, but its direct, understandable and portable.
>
> Neil
>
>>>
>>>>
>>>>>
>>>>> 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
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH net] sctp: make sctp_setsockopt_events() less strict about the option length
@ 2019-02-06 21:48 ` Julien Gomes
0 siblings, 0 replies; 51+ messages in thread
From: Julien Gomes @ 2019-02-06 21:48 UTC (permalink / raw)
To: Neil Horman, Marcelo Ricardo Leitner
Cc: netdev, linux-sctp, linux-kernel, davem, vyasevich, lucien.xin
On 2/6/19 1:23 PM, Neil Horman wrote:
> On Wed, Feb 06, 2019 at 07:07:23PM -0200, 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.
>>
> Yeah, I'm not supportive of codifying that knoweldge in the kernel. If we were
> to support bi-directional versioning, I would encode it into lksctp-tools rather
> than the kernel.
I'm not sure that forcing a library on users is a good reason to break UAPI.
>
>>>
>>> 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?
>>
> No, I think there are a few others (maybe paddrparams?)
>
>>>
>>>
>>> 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?
>>
> I think the thought was to differentiate between someone passing a legit larger
> structure from a newer UAPI, from someone just passing in a massive
> inappropriately sized buffer (even if the return on both is the same).
>
>> 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.
>>
> I'm less than excited about making the kernel check an unbounded user space
> buffer, thats seems like a potential DOS attack from an unpriviledged user to
> me. I'm also still hung up on the notion that, despite how we do this, this
> patch is going into the latest kernel, so it will only work on a kernel that
> already understands the most recent set of subscriptions. It would work if we,
> again someday in the future extended this struct, someone built against that
> newer UAPI, and then tried to run it on a kernel that had this patch.
The patch is going into the latest, but can also be backported on future
stables.
I don't think "not fixing it because it's not fixed yet" is a good
reason to keep things the way they are. But maybe that's just me.
Given that the structure has already been extended several times, there
is pretty much nothing to keep this from happening again and again.
>
> FWIW, there is an existing implied method to determine the available
> subscription events. sctp_getsockopt_events does clamp the size of the output
> buffer, and returns that information in the optlen field via put_user. An
> application that was build against UAPIs from 4.19 could pass in the 4.19
> sctp_event_subscribe struct to sctp_getsockopt_events, and read the output
> length, whcih would inform the application of the events that the kernel is
> capable of reporting, and limit itself to only using those events. Its not a
> perfect solution, but its direct, understandable and portable.
>
> Neil
>
>>>
>>>>
>>>>>
>>>>> 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
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH net] sctp: make sctp_setsockopt_events() less strict about the option length
2019-02-06 21:48 ` Julien Gomes
@ 2019-02-07 14:44 ` Neil Horman
-1 siblings, 0 replies; 51+ messages in thread
From: Neil Horman @ 2019-02-07 14:44 UTC (permalink / raw)
To: Julien Gomes
Cc: Marcelo Ricardo Leitner, netdev, linux-sctp, linux-kernel, davem,
vyasevich, lucien.xin
On Wed, Feb 06, 2019 at 01:48:42PM -0800, Julien Gomes wrote:
>
>
> On 2/6/19 1:23 PM, Neil Horman wrote:
> > On Wed, Feb 06, 2019 at 07:07:23PM -0200, 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.
> >>
> > Yeah, I'm not supportive of codifying that knoweldge in the kernel. If we were
> > to support bi-directional versioning, I would encode it into lksctp-tools rather
> > than the kernel.
>
> I'm not sure that forcing a library on users is a good reason to break UAPI.
>
Thats a misleading statement. We've never supported running newer applications
on older kernels, and no one is forcing anyone to use the lksctp-tools library,
I was only suggesting that, if we were to support this compatibility, that might
be a place to offer it.
Its also worth noting that we have precident for this. If you look at the git
log, this particular structure has been extended about 6 times in the life of
sctp.
> >
> >>>
> >>> 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?
> >>
> > No, I think there are a few others (maybe paddrparams?)
> >
> >>>
> >>>
> >>> 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?
> >>
> > I think the thought was to differentiate between someone passing a legit larger
> > structure from a newer UAPI, from someone just passing in a massive
> > inappropriately sized buffer (even if the return on both is the same).
> >
> >> 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.
> >>
> > I'm less than excited about making the kernel check an unbounded user space
> > buffer, thats seems like a potential DOS attack from an unpriviledged user to
> > me. I'm also still hung up on the notion that, despite how we do this, this
> > patch is going into the latest kernel, so it will only work on a kernel that
> > already understands the most recent set of subscriptions. It would work if we,
> > again someday in the future extended this struct, someone built against that
> > newer UAPI, and then tried to run it on a kernel that had this patch.
>
> The patch is going into the latest, but can also be backported on future
> stables.
> I don't think "not fixing it because it's not fixed yet" is a good
> reason to keep things the way they are. But maybe that's just me.
> Given that the structure has already been extended several times, there
> is pretty much nothing to keep this from happening again and again.
>
Also misleading, as it assumes that we're not intentionally doing this. I get
wanting to support running applications built for newer kernels on older
kernels, but thats just not something that we do, and to say thats broken is
misleading. Older applications are required to run on newer kernels, but not
vice versa, which is what you are asking for.
And yes, this patch can be backported to older stable kernels, but by that same
token, so can the patches that extend the struct, which would also fix the
problem, while supporting the newer features, which seems to me to be the better
solution for applications which are looking for that support.
> >
> > FWIW, there is an existing implied method to determine the available
> > subscription events. sctp_getsockopt_events does clamp the size of the output
> > buffer, and returns that information in the optlen field via put_user. An
> > application that was build against UAPIs from 4.19 could pass in the 4.19
> > sctp_event_subscribe struct to sctp_getsockopt_events, and read the output
> > length, whcih would inform the application of the events that the kernel is
> > capable of reporting, and limit itself to only using those events. Its not a
> > perfect solution, but its direct, understandable and portable.
> >
> > Neil
> >
> >>>
> >>>>
> >>>>>
> >>>>> 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
>
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH net] sctp: make sctp_setsockopt_events() less strict about the option length
@ 2019-02-07 14:44 ` Neil Horman
0 siblings, 0 replies; 51+ messages in thread
From: Neil Horman @ 2019-02-07 14:44 UTC (permalink / raw)
To: Julien Gomes
Cc: Marcelo Ricardo Leitner, netdev, linux-sctp, linux-kernel, davem,
vyasevich, lucien.xin
On Wed, Feb 06, 2019 at 01:48:42PM -0800, Julien Gomes wrote:
>
>
> On 2/6/19 1:23 PM, Neil Horman wrote:
> > On Wed, Feb 06, 2019 at 07:07:23PM -0200, 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.
> >>
> > Yeah, I'm not supportive of codifying that knoweldge in the kernel. If we were
> > to support bi-directional versioning, I would encode it into lksctp-tools rather
> > than the kernel.
>
> I'm not sure that forcing a library on users is a good reason to break UAPI.
>
Thats a misleading statement. We've never supported running newer applications
on older kernels, and no one is forcing anyone to use the lksctp-tools library,
I was only suggesting that, if we were to support this compatibility, that might
be a place to offer it.
Its also worth noting that we have precident for this. If you look at the git
log, this particular structure has been extended about 6 times in the life of
sctp.
> >
> >>>
> >>> 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?
> >>
> > No, I think there are a few others (maybe paddrparams?)
> >
> >>>
> >>>
> >>> 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?
> >>
> > I think the thought was to differentiate between someone passing a legit larger
> > structure from a newer UAPI, from someone just passing in a massive
> > inappropriately sized buffer (even if the return on both is the same).
> >
> >> 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.
> >>
> > I'm less than excited about making the kernel check an unbounded user space
> > buffer, thats seems like a potential DOS attack from an unpriviledged user to
> > me. I'm also still hung up on the notion that, despite how we do this, this
> > patch is going into the latest kernel, so it will only work on a kernel that
> > already understands the most recent set of subscriptions. It would work if we,
> > again someday in the future extended this struct, someone built against that
> > newer UAPI, and then tried to run it on a kernel that had this patch.
>
> The patch is going into the latest, but can also be backported on future
> stables.
> I don't think "not fixing it because it's not fixed yet" is a good
> reason to keep things the way they are. But maybe that's just me.
> Given that the structure has already been extended several times, there
> is pretty much nothing to keep this from happening again and again.
>
Also misleading, as it assumes that we're not intentionally doing this. I get
wanting to support running applications built for newer kernels on older
kernels, but thats just not something that we do, and to say thats broken is
misleading. Older applications are required to run on newer kernels, but not
vice versa, which is what you are asking for.
And yes, this patch can be backported to older stable kernels, but by that same
token, so can the patches that extend the struct, which would also fix the
problem, while supporting the newer features, which seems to me to be the better
solution for applications which are looking for that support.
> >
> > FWIW, there is an existing implied method to determine the available
> > subscription events. sctp_getsockopt_events does clamp the size of the output
> > buffer, and returns that information in the optlen field via put_user. An
> > application that was build against UAPIs from 4.19 could pass in the 4.19
> > sctp_event_subscribe struct to sctp_getsockopt_events, and read the output
> > length, whcih would inform the application of the events that the kernel is
> > capable of reporting, and limit itself to only using those events. Its not a
> > perfect solution, but its direct, understandable and portable.
> >
> > Neil
> >
> >>>
> >>>>
> >>>>>
> >>>>> 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
>
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH net] sctp: make sctp_setsockopt_events() less strict about the option length
2019-02-06 21:07 ` Marcelo Ricardo Leitner
@ 2019-02-06 21:26 ` Julien Gomes
-1 siblings, 0 replies; 51+ messages in thread
From: Julien Gomes @ 2019-02-06 21:26 UTC (permalink / raw)
To: Marcelo Ricardo Leitner
Cc: netdev, linux-sctp, linux-kernel, davem, nhorman, vyasevich, lucien.xin
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
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH net] sctp: make sctp_setsockopt_events() less strict about the option length
@ 2019-02-06 21:26 ` Julien Gomes
0 siblings, 0 replies; 51+ messages in thread
From: Julien Gomes @ 2019-02-06 21:26 UTC (permalink / raw)
To: Marcelo Ricardo Leitner
Cc: netdev, linux-sctp, linux-kernel, davem, nhorman, vyasevich, lucien.xin
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
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH net] sctp: make sctp_setsockopt_events() less strict about the option length
2019-02-06 21:26 ` Julien Gomes
@ 2019-02-06 21:39 ` Neil Horman
-1 siblings, 0 replies; 51+ messages in thread
From: Neil Horman @ 2019-02-06 21:39 UTC (permalink / raw)
To: Julien Gomes
Cc: Marcelo Ricardo Leitner, netdev, linux-sctp, linux-kernel, davem,
vyasevich, lucien.xin
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).
> 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
>
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH net] sctp: make sctp_setsockopt_events() less strict about the option length
@ 2019-02-06 21:39 ` Neil Horman
0 siblings, 0 replies; 51+ messages in thread
From: Neil Horman @ 2019-02-06 21:39 UTC (permalink / raw)
To: Julien Gomes
Cc: Marcelo Ricardo Leitner, netdev, linux-sctp, linux-kernel, davem,
vyasevich, lucien.xin
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).
> 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
>
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH net] sctp: make sctp_setsockopt_events() less strict about the option length
2019-02-06 21:39 ` Neil Horman
@ 2019-02-06 21:48 ` Julien Gomes
-1 siblings, 0 replies; 51+ messages in thread
From: Julien Gomes @ 2019-02-06 21:48 UTC (permalink / raw)
To: Neil Horman
Cc: Marcelo Ricardo Leitner, netdev, linux-sctp, linux-kernel, davem,
vyasevich, lucien.xin
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...
>
>> 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
>>
--
Julien Gomes
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH net] sctp: make sctp_setsockopt_events() less strict about the option length
@ 2019-02-06 21:48 ` Julien Gomes
0 siblings, 0 replies; 51+ messages in thread
From: Julien Gomes @ 2019-02-06 21:48 UTC (permalink / raw)
To: Neil Horman
Cc: Marcelo Ricardo Leitner, netdev, linux-sctp, linux-kernel, davem,
vyasevich, lucien.xin
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...
>
>> 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
>>
--
Julien Gomes
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH net] sctp: make sctp_setsockopt_events() less strict about the option length
2019-02-06 21:48 ` Julien Gomes
@ 2019-02-06 21:53 ` Julien Gomes
-1 siblings, 0 replies; 51+ messages in thread
From: Julien Gomes @ 2019-02-06 21:53 UTC (permalink / raw)
To: Neil Horman
Cc: Marcelo Ricardo Leitner, netdev, linux-sctp, linux-kernel, davem,
vyasevich, lucien.xin
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@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
>>>
>
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH net] sctp: make sctp_setsockopt_events() less strict about the option length
@ 2019-02-06 21:53 ` Julien Gomes
0 siblings, 0 replies; 51+ messages in thread
From: Julien Gomes @ 2019-02-06 21:53 UTC (permalink / raw)
To: Neil Horman
Cc: Marcelo Ricardo Leitner, netdev, linux-sctp, linux-kernel, davem,
vyasevich, lucien.xin
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@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
>>>
>
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH net] sctp: make sctp_setsockopt_events() less strict about the option length
2019-02-06 21:48 ` Julien Gomes
@ 2019-02-07 14:48 ` Neil Horman
-1 siblings, 0 replies; 51+ messages in thread
From: Neil Horman @ 2019-02-07 14:48 UTC (permalink / raw)
To: Julien Gomes
Cc: Marcelo Ricardo Leitner, netdev, linux-sctp, linux-kernel, davem,
vyasevich, lucien.xin
On Wed, Feb 06, 2019 at 01:48:44PM -0800, 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.
>
Probably not, but I'm not going to pick a magic number to gate whats ok and
whats not for sockopt validation.
> 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...
>
We really don't have to, I refer you to my previous not referencing the fact
that the getsockopt variant of this call will return the expected length of this
option for the running kernel, allowing userspace to know explicitly what the
buffer size should be, and by extension, what options are supported
Neil
> >
> >> 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
> >>
>
> --
> Julien Gomes
>
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH net] sctp: make sctp_setsockopt_events() less strict about the option length
@ 2019-02-07 14:48 ` Neil Horman
0 siblings, 0 replies; 51+ messages in thread
From: Neil Horman @ 2019-02-07 14:48 UTC (permalink / raw)
To: Julien Gomes
Cc: Marcelo Ricardo Leitner, netdev, linux-sctp, linux-kernel, davem,
vyasevich, lucien.xin
On Wed, Feb 06, 2019 at 01:48:44PM -0800, 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.
>
Probably not, but I'm not going to pick a magic number to gate whats ok and
whats not for sockopt validation.
> 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...
>
We really don't have to, I refer you to my previous not referencing the fact
that the getsockopt variant of this call will return the expected length of this
option for the running kernel, allowing userspace to know explicitly what the
buffer size should be, and by extension, what options are supported
Neil
> >
> >> 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
> >>
>
> --
> Julien Gomes
>
^ permalink raw reply [flat|nested] 51+ messages in thread
* RE: [PATCH net] sctp: make sctp_setsockopt_events() less strict about the option length
2019-02-06 21:07 ` Marcelo Ricardo Leitner
@ 2019-02-07 17:33 ` David Laight
-1 siblings, 0 replies; 51+ messages in thread
From: David Laight @ 2019-02-07 17:33 UTC (permalink / raw)
To: 'Marcelo Ricardo Leitner', Julien Gomes
Cc: netdev, linux-sctp, linux-kernel, davem, nhorman, vyasevich, lucien.xin
From: Marcelo Ricardo Leitner
> Sent: 06 February 2019 21:07
>
> 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.
It is probably better to break the recompilation of the few programs
that use the new fields (and have them not work on old kernels)
than to stop recompilations of old programs stop working on old
kernels or have requested new options silently ignored.
There are all sorts of reasons why programs get built on systems that
are newer than the ones they need to run on.
I'm currently planning to get around the glibc 'memcpy()' fubar so I
can retire some very old build systems before their disks die.
Fortunately our sctp code is in the kernel - so has to be compiled
with the correct headers.
> > 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.
Agreed, these structures should never be changed.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 51+ messages in thread
* RE: [PATCH net] sctp: make sctp_setsockopt_events() less strict about the option length
@ 2019-02-07 17:33 ` David Laight
0 siblings, 0 replies; 51+ messages in thread
From: David Laight @ 2019-02-07 17:33 UTC (permalink / raw)
To: 'Marcelo Ricardo Leitner', Julien Gomes
Cc: netdev, linux-sctp, linux-kernel, davem, nhorman, vyasevich, lucien.xin
From: Marcelo Ricardo Leitner
> Sent: 06 February 2019 21:07
>
> 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.
It is probably better to break the recompilation of the few programs
that use the new fields (and have them not work on old kernels)
than to stop recompilations of old programs stop working on old
kernels or have requested new options silently ignored.
There are all sorts of reasons why programs get built on systems that
are newer than the ones they need to run on.
I'm currently planning to get around the glibc 'memcpy()' fubar so I
can retire some very old build systems before their disks die.
Fortunately our sctp code is in the kernel - so has to be compiled
with the correct headers.
> > 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.
Agreed, these structures should never be changed.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH net] sctp: make sctp_setsockopt_events() less strict about the option length
2019-02-07 17:33 ` David Laight
@ 2019-02-07 17:47 ` 'Marcelo Ricardo Leitner'
-1 siblings, 0 replies; 51+ messages in thread
From: 'Marcelo Ricardo Leitner' @ 2019-02-07 17:47 UTC (permalink / raw)
To: David Laight
Cc: Julien Gomes, netdev, linux-sctp, linux-kernel, davem, nhorman,
vyasevich, lucien.xin
On Thu, Feb 07, 2019 at 05:33:07PM +0000, David Laight wrote:
> From: Marcelo Ricardo Leitner
> > Sent: 06 February 2019 21:07
> >
> > 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.
>
> It is probably better to break the recompilation of the few programs
> that use the new fields (and have them not work on old kernels)
> than to stop recompilations of old programs stop working on old
> kernels or have requested new options silently ignored.
I got confused here, not sure what you mean. Seems there is one "stop"
word too many.
>
> There are all sorts of reasons why programs get built on systems that
> are newer than the ones they need to run on.
> I'm currently planning to get around the glibc 'memcpy()' fubar so I
> can retire some very old build systems before their disks die.
You can virtualize those. That's not really a good reason for
building with newer kernel and running on old systems, as virtually
any old system can be virtualized.
Marcelo
>
> Fortunately our sctp code is in the kernel - so has to be compiled
> with the correct headers.
>
> > > 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.
>
> Agreed, these structures should never be changed.
>
> David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH net] sctp: make sctp_setsockopt_events() less strict about the option length
@ 2019-02-07 17:47 ` 'Marcelo Ricardo Leitner'
0 siblings, 0 replies; 51+ messages in thread
From: 'Marcelo Ricardo Leitner' @ 2019-02-07 17:47 UTC (permalink / raw)
To: David Laight
Cc: Julien Gomes, netdev, linux-sctp, linux-kernel, davem, nhorman,
vyasevich, lucien.xin
On Thu, Feb 07, 2019 at 05:33:07PM +0000, David Laight wrote:
> From: Marcelo Ricardo Leitner
> > Sent: 06 February 2019 21:07
> >
> > 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.
>
> It is probably better to break the recompilation of the few programs
> that use the new fields (and have them not work on old kernels)
> than to stop recompilations of old programs stop working on old
> kernels or have requested new options silently ignored.
I got confused here, not sure what you mean. Seems there is one "stop"
word too many.
>
> There are all sorts of reasons why programs get built on systems that
> are newer than the ones they need to run on.
> I'm currently planning to get around the glibc 'memcpy()' fubar so I
> can retire some very old build systems before their disks die.
You can virtualize those. That's not really a good reason for
building with newer kernel and running on old systems, as virtually
any old system can be virtualized.
Marcelo
>
> Fortunately our sctp code is in the kernel - so has to be compiled
> with the correct headers.
>
> > > 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.
>
> Agreed, these structures should never be changed.
>
> David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>
^ permalink raw reply [flat|nested] 51+ messages in thread
* RE: [PATCH net] sctp: make sctp_setsockopt_events() less strict about the option length
2019-02-07 17:47 ` 'Marcelo Ricardo Leitner'
@ 2019-02-08 9:53 ` David Laight
-1 siblings, 0 replies; 51+ messages in thread
From: David Laight @ 2019-02-08 9:53 UTC (permalink / raw)
To: 'Marcelo Ricardo Leitner'
Cc: Julien Gomes, netdev, linux-sctp, linux-kernel, davem, nhorman,
vyasevich, lucien.xin
From: 'Marcelo Ricardo Leitner'
> Sent: 07 February 2019 17:47
...
> > > 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.
> >
> > It is probably better to break the recompilation of the few programs
> > that use the new fields (and have them not work on old kernels)
> > than to stop recompilations of old programs stop working on old
> > kernels or have requested new options silently ignored.
>
> I got confused here, not sure what you mean. Seems there is one "stop"
> word too many.
More confusing than I intended...
With the current kernel and headers a 'new program' (one that
needs the new options) will fail to run on an old kernel - which is good.
However a recompilation of an 'old program' (that doesn't use
the new options) will also fail to run on an old kernel - which is bad.
Changing the kernel to ignore extra events flags breaks the 'new'
program.
Versioning the structure now (even though it should have been done
earlier) won't change the behaviour of existing binaries.
However a recompilation of an 'old' program would use the 'old'
structure and work on old kernels.
Attempts to recompile a 'new' program will fail - until the structure
name (or some #define to enable the extra fields) is changed.
Breaking compilations is much better than unexpected run-time
behaviour.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 51+ messages in thread
* RE: [PATCH net] sctp: make sctp_setsockopt_events() less strict about the option length
@ 2019-02-08 9:53 ` David Laight
0 siblings, 0 replies; 51+ messages in thread
From: David Laight @ 2019-02-08 9:53 UTC (permalink / raw)
To: 'Marcelo Ricardo Leitner'
Cc: Julien Gomes, netdev, linux-sctp, linux-kernel, davem, nhorman,
vyasevich, lucien.xin
From: 'Marcelo Ricardo Leitner'
> Sent: 07 February 2019 17:47
...
> > > 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.
> >
> > It is probably better to break the recompilation of the few programs
> > that use the new fields (and have them not work on old kernels)
> > than to stop recompilations of old programs stop working on old
> > kernels or have requested new options silently ignored.
>
> I got confused here, not sure what you mean. Seems there is one "stop"
> word too many.
More confusing than I intended...
With the current kernel and headers a 'new program' (one that
needs the new options) will fail to run on an old kernel - which is good.
However a recompilation of an 'old program' (that doesn't use
the new options) will also fail to run on an old kernel - which is bad.
Changing the kernel to ignore extra events flags breaks the 'new'
program.
Versioning the structure now (even though it should have been done
earlier) won't change the behaviour of existing binaries.
However a recompilation of an 'old' program would use the 'old'
structure and work on old kernels.
Attempts to recompile a 'new' program will fail - until the structure
name (or some #define to enable the extra fields) is changed.
Breaking compilations is much better than unexpected run-time
behaviour.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH net] sctp: make sctp_setsockopt_events() less strict about the option length
2019-02-08 9:53 ` David Laight
@ 2019-02-08 12:36 ` Neil Horman
-1 siblings, 0 replies; 51+ messages in thread
From: Neil Horman @ 2019-02-08 12:36 UTC (permalink / raw)
To: David Laight
Cc: 'Marcelo Ricardo Leitner',
Julien Gomes, netdev, linux-sctp, linux-kernel, davem, vyasevich,
lucien.xin
On Fri, Feb 08, 2019 at 09:53:03AM +0000, David Laight wrote:
> From: 'Marcelo Ricardo Leitner'
> > Sent: 07 February 2019 17:47
> ...
> > > > 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.
> > >
> > > It is probably better to break the recompilation of the few programs
> > > that use the new fields (and have them not work on old kernels)
> > > than to stop recompilations of old programs stop working on old
> > > kernels or have requested new options silently ignored.
> >
> > I got confused here, not sure what you mean. Seems there is one "stop"
> > word too many.
>
> More confusing than I intended...
>
> With the current kernel and headers a 'new program' (one that
> needs the new options) will fail to run on an old kernel - which is good.
> However a recompilation of an 'old program' (that doesn't use
> the new options) will also fail to run on an old kernel - which is bad.
>
I disagree with this, at least as a unilateral statement. I would assert that
an old program, within the constraints of the issue being discussed here, will
run perfectly well, when built and run against a new kernel.
At issue is the size of the structure sctp_event_subscribe, and the fact that in
several instances over the last few years, its been extended to be larger and
encompass more events to subscribe to.
Nominally an application will use this structure (roughly) as follows:
...
struct sctp_event_subscribe events;
size_t evsize = sizeof(events);
memset(&events, 0, sizeof(events));
events.sctp_send_failure_event = 1; /*example event subscription*/
if (sctp_setsocktpt(sctp_fd, SOL_SCTP, SCTP_EVENTS, &events, &evsize) < 0) {
/* do error recovery */
}
....
Assume this code will be built and run against kernel versions A and B, in
which:
A) has a struct sctp_event_subscribe with a size of 9 bytes
B) has a struct sctp_event_subscribe with a size of 10 bytes (due to the added
field sctp_sender_dry_event)
That gives us 4 cases to handle
1) Application build against kernel A and run on kernel A. This works fine, the
sizes of the struct in question will always match
2) Application is built against kernel A and run on kernel B. In this case,
everything will work because the application passes a buffer of size 9, and the
kernel accepts it, because it allows for buffers to be shorter than the current
struct sctp_event_subscribe size. The kernel simply operates on the options
available in the buffer. The application is none the wiser, because it has no
knoweldge of the new option, nor should it because it was built against kernel
A, that never offered that option
3) Application is built against kernel B and run on kernel B. This works fine
for the same reason as (1).
4) Application is built against kernel B and run on kernel A. This will break
because the application is passing a buffer that is larger than what the kernel
expects, and rightly so. The application is passing in a buffer that is
incompatible with what the running kernel expects.
We could look into ways in which to detect the cases in which this might be
'ok', but I don't see why we should bother, because at some point its still an
error to pass in an incompatible buffer. In my mind this is no different than
trying to run a program that allocates hugepages on a kernel that doesn't
support hugepages (just to make up an example). Applications built against
newer kernel can't expect all the features/semantics/etc to be identical to
older kernels.
> Changing the kernel to ignore extra events flags breaks the 'new'
> program.
>
It shouldn't. Assuming you have a program built against headers from kernel B
(above), if you set a field in the structure that only exists in kernel B, and
try to run it on kernel A, you will get an EINVAL return, which is correct
behavior because you are attempting to deliver information to the kernel that
kernel A (the running kernel) doesn't know about. Thats correct behavior.
> Versioning the structure now (even though it should have been done
> earlier) won't change the behaviour of existing binaries.
>
I won't disagree about the niceness of versioning, but that ship has sailed.
> However a recompilation of an 'old' program would use the 'old'
> structure and work on old kernels.
To be clear, this is situation (1) above, and yeah, running on the kernel you
built your application against should always work from a compatibility
standpoint.
> Attempts to recompile a 'new' program will fail - until the structure
> name (or some #define to enable the extra fields) is changed.
>
Yes, but this is alawys the case for structures that change. If you have an
application built against kernel (B), and uses structure fields that only exist
in that version of the kernel (and not earlier) will fail to compile when built
against kernel (A) headers, and thats expected. This happens with any kernel
api that exists in a newer kernel but not an older kernel.
> Breaking compilations is much better than unexpected run-time
> behaviour.
>
Any time you make a system call to the kernel, you have to be prepared to handle
the resulting error condition, thats not unexpected. To assume that a system
call will always work is bad programming practice.
Neil
> David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>
>
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH net] sctp: make sctp_setsockopt_events() less strict about the option length
@ 2019-02-08 12:36 ` Neil Horman
0 siblings, 0 replies; 51+ messages in thread
From: Neil Horman @ 2019-02-08 12:36 UTC (permalink / raw)
To: David Laight
Cc: 'Marcelo Ricardo Leitner',
Julien Gomes, netdev, linux-sctp, linux-kernel, davem, vyasevich,
lucien.xin
On Fri, Feb 08, 2019 at 09:53:03AM +0000, David Laight wrote:
> From: 'Marcelo Ricardo Leitner'
> > Sent: 07 February 2019 17:47
> ...
> > > > 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.
> > >
> > > It is probably better to break the recompilation of the few programs
> > > that use the new fields (and have them not work on old kernels)
> > > than to stop recompilations of old programs stop working on old
> > > kernels or have requested new options silently ignored.
> >
> > I got confused here, not sure what you mean. Seems there is one "stop"
> > word too many.
>
> More confusing than I intended...
>
> With the current kernel and headers a 'new program' (one that
> needs the new options) will fail to run on an old kernel - which is good.
> However a recompilation of an 'old program' (that doesn't use
> the new options) will also fail to run on an old kernel - which is bad.
>
I disagree with this, at least as a unilateral statement. I would assert that
an old program, within the constraints of the issue being discussed here, will
run perfectly well, when built and run against a new kernel.
At issue is the size of the structure sctp_event_subscribe, and the fact that in
several instances over the last few years, its been extended to be larger and
encompass more events to subscribe to.
Nominally an application will use this structure (roughly) as follows:
...
struct sctp_event_subscribe events;
size_t evsize = sizeof(events);
memset(&events, 0, sizeof(events));
events.sctp_send_failure_event = 1; /*example event subscription*/
if (sctp_setsocktpt(sctp_fd, SOL_SCTP, SCTP_EVENTS, &events, &evsize) < 0) {
/* do error recovery */
}
....
Assume this code will be built and run against kernel versions A and B, in
which:
A) has a struct sctp_event_subscribe with a size of 9 bytes
B) has a struct sctp_event_subscribe with a size of 10 bytes (due to the added
field sctp_sender_dry_event)
That gives us 4 cases to handle
1) Application build against kernel A and run on kernel A. This works fine, the
sizes of the struct in question will always match
2) Application is built against kernel A and run on kernel B. In this case,
everything will work because the application passes a buffer of size 9, and the
kernel accepts it, because it allows for buffers to be shorter than the current
struct sctp_event_subscribe size. The kernel simply operates on the options
available in the buffer. The application is none the wiser, because it has no
knoweldge of the new option, nor should it because it was built against kernel
A, that never offered that option
3) Application is built against kernel B and run on kernel B. This works fine
for the same reason as (1).
4) Application is built against kernel B and run on kernel A. This will break
because the application is passing a buffer that is larger than what the kernel
expects, and rightly so. The application is passing in a buffer that is
incompatible with what the running kernel expects.
We could look into ways in which to detect the cases in which this might be
'ok', but I don't see why we should bother, because at some point its still an
error to pass in an incompatible buffer. In my mind this is no different than
trying to run a program that allocates hugepages on a kernel that doesn't
support hugepages (just to make up an example). Applications built against
newer kernel can't expect all the features/semantics/etc to be identical to
older kernels.
> Changing the kernel to ignore extra events flags breaks the 'new'
> program.
>
It shouldn't. Assuming you have a program built against headers from kernel B
(above), if you set a field in the structure that only exists in kernel B, and
try to run it on kernel A, you will get an EINVAL return, which is correct
behavior because you are attempting to deliver information to the kernel that
kernel A (the running kernel) doesn't know about. Thats correct behavior.
> Versioning the structure now (even though it should have been done
> earlier) won't change the behaviour of existing binaries.
>
I won't disagree about the niceness of versioning, but that ship has sailed.
> However a recompilation of an 'old' program would use the 'old'
> structure and work on old kernels.
To be clear, this is situation (1) above, and yeah, running on the kernel you
built your application against should always work from a compatibility
standpoint.
> Attempts to recompile a 'new' program will fail - until the structure
> name (or some #define to enable the extra fields) is changed.
>
Yes, but this is alawys the case for structures that change. If you have an
application built against kernel (B), and uses structure fields that only exist
in that version of the kernel (and not earlier) will fail to compile when built
against kernel (A) headers, and thats expected. This happens with any kernel
api that exists in a newer kernel but not an older kernel.
> Breaking compilations is much better than unexpected run-time
> behaviour.
>
Any time you make a system call to the kernel, you have to be prepared to handle
the resulting error condition, thats not unexpected. To assume that a system
call will always work is bad programming practice.
Neil
> David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>
>
^ permalink raw reply [flat|nested] 51+ messages in thread