* [PATCH net] sctp: make sure stream nums can match optlen in sctp_setsockopt_reset_streams
@ 2017-12-10 7:40 Xin Long
2017-12-10 13:51 ` Marcelo Ricardo Leitner
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Xin Long @ 2017-12-10 7:40 UTC (permalink / raw)
To: network dev, linux-sctp
Cc: davem, Marcelo Ricardo Leitner, Neil Horman, syzkaller
Now in sctp_setsockopt_reset_streams, it only does the check
optlen < sizeof(*params) for optlen. But it's not enough, as
params->srs_number_streams should also match optlen.
If the streams in params->srs_stream_list are less than stream
nums in params->srs_number_streams, later when dereferencing
the stream list, it could cause a slab-out-of-bounds crash, as
reported by syzbot.
This patch is to fix it by also checking the stream numbers in
sctp_setsockopt_reset_streams to make sure at least it's not
greater than the streams in the list.
Fixes: 7f9d68ac944e ("sctp: implement sender-side procedures for SSN Reset Request Parameter")
Reported-by: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
net/sctp/socket.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 014847e..dbf140d 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -3891,13 +3891,17 @@ static int sctp_setsockopt_reset_streams(struct sock *sk,
struct sctp_association *asoc;
int retval = -EINVAL;
- if (optlen < sizeof(struct sctp_reset_streams))
+ if (optlen < sizeof(*params))
return -EINVAL;
params = memdup_user(optval, optlen);
if (IS_ERR(params))
return PTR_ERR(params);
+ if (params->srs_number_streams * sizeof(__u16) >
+ optlen - sizeof(*params))
+ goto out;
+
asoc = sctp_id2assoc(sk, params->srs_assoc_id);
if (!asoc)
goto out;
--
2.1.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net] sctp: make sure stream nums can match optlen in sctp_setsockopt_reset_streams
2017-12-10 7:40 [PATCH net] sctp: make sure stream nums can match optlen in sctp_setsockopt_reset_streams Xin Long
@ 2017-12-10 13:51 ` Marcelo Ricardo Leitner
2017-12-11 14:54 ` Neil Horman
2017-12-11 19:09 ` David Miller
2 siblings, 0 replies; 6+ messages in thread
From: Marcelo Ricardo Leitner @ 2017-12-10 13:51 UTC (permalink / raw)
To: Xin Long; +Cc: network dev, linux-sctp, davem, Neil Horman, syzkaller
On Sun, Dec 10, 2017 at 03:40:51PM +0800, Xin Long wrote:
> Now in sctp_setsockopt_reset_streams, it only does the check
> optlen < sizeof(*params) for optlen. But it's not enough, as
> params->srs_number_streams should also match optlen.
>
> If the streams in params->srs_stream_list are less than stream
> nums in params->srs_number_streams, later when dereferencing
> the stream list, it could cause a slab-out-of-bounds crash, as
> reported by syzbot.
>
> This patch is to fix it by also checking the stream numbers in
> sctp_setsockopt_reset_streams to make sure at least it's not
> greater than the streams in the list.
>
> Fixes: 7f9d68ac944e ("sctp: implement sender-side procedures for SSN Reset Request Parameter")
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> ---
> net/sctp/socket.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 014847e..dbf140d 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -3891,13 +3891,17 @@ static int sctp_setsockopt_reset_streams(struct sock *sk,
> struct sctp_association *asoc;
> int retval = -EINVAL;
>
> - if (optlen < sizeof(struct sctp_reset_streams))
> + if (optlen < sizeof(*params))
> return -EINVAL;
>
> params = memdup_user(optval, optlen);
> if (IS_ERR(params))
> return PTR_ERR(params);
>
> + if (params->srs_number_streams * sizeof(__u16) >
> + optlen - sizeof(*params))
> + goto out;
> +
> asoc = sctp_id2assoc(sk, params->srs_assoc_id);
> if (!asoc)
> goto out;
> --
> 2.1.0
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] sctp: make sure stream nums can match optlen in sctp_setsockopt_reset_streams
2017-12-10 7:40 [PATCH net] sctp: make sure stream nums can match optlen in sctp_setsockopt_reset_streams Xin Long
2017-12-10 13:51 ` Marcelo Ricardo Leitner
@ 2017-12-11 14:54 ` Neil Horman
2017-12-11 15:36 ` Marcelo Ricardo Leitner
2017-12-11 19:09 ` David Miller
2 siblings, 1 reply; 6+ messages in thread
From: Neil Horman @ 2017-12-11 14:54 UTC (permalink / raw)
To: Xin Long; +Cc: network dev, linux-sctp, davem, Marcelo Ricardo Leitner,
syzkaller
On Sun, Dec 10, 2017 at 03:40:51PM +0800, Xin Long wrote:
> Now in sctp_setsockopt_reset_streams, it only does the check
> optlen < sizeof(*params) for optlen. But it's not enough, as
> params->srs_number_streams should also match optlen.
>
> If the streams in params->srs_stream_list are less than stream
> nums in params->srs_number_streams, later when dereferencing
> the stream list, it could cause a slab-out-of-bounds crash, as
> reported by syzbot.
>
> This patch is to fix it by also checking the stream numbers in
> sctp_setsockopt_reset_streams to make sure at least it's not
> greater than the streams in the list.
>
> Fixes: 7f9d68ac944e ("sctp: implement sender-side procedures for SSN Reset Request Parameter")
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
> net/sctp/socket.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 014847e..dbf140d 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -3891,13 +3891,17 @@ static int sctp_setsockopt_reset_streams(struct sock *sk,
> struct sctp_association *asoc;
> int retval = -EINVAL;
>
> - if (optlen < sizeof(struct sctp_reset_streams))
> + if (optlen < sizeof(*params))
> return -EINVAL;
>
Is this going to work in all corner cases? IIRC struct sctp_reset_stream has
variable length array at the end of it, and so sizeof(struct sctp_reset_streams)
returns just the size of the struct, while sizeof(*params) returns the size of
the entire object (including the array elements). If a user space task
allocates a static memory block to hold this struct and the array, and passes it
in with a shorter optlen (if for example, they do not intend to use all the
array elements), this will cause a failure, where no failure is truly warranted.
It seems the correct check to me should be the origional sizeof(struct
sctp_reset_streams) check, and the below check to ensure that there are at least
the same number of array elements available as indicated in srs_nuber_streams.
Regards
Neil
> params = memdup_user(optval, optlen);
> if (IS_ERR(params))
> return PTR_ERR(params);
>
> + if (params->srs_number_streams * sizeof(__u16) >
> + optlen - sizeof(*params))
> + goto out;
> +
> asoc = sctp_id2assoc(sk, params->srs_assoc_id);
> if (!asoc)
> goto out;
> --
> 2.1.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] sctp: make sure stream nums can match optlen in sctp_setsockopt_reset_streams
2017-12-11 14:54 ` Neil Horman
@ 2017-12-11 15:36 ` Marcelo Ricardo Leitner
2017-12-11 18:28 ` Neil Horman
0 siblings, 1 reply; 6+ messages in thread
From: Marcelo Ricardo Leitner @ 2017-12-11 15:36 UTC (permalink / raw)
To: Neil Horman; +Cc: Xin Long, network dev, linux-sctp, davem, syzkaller
Hi,
On Mon, Dec 11, 2017 at 09:54:34AM -0500, Neil Horman wrote:
> On Sun, Dec 10, 2017 at 03:40:51PM +0800, Xin Long wrote:
> > Now in sctp_setsockopt_reset_streams, it only does the check
> > optlen < sizeof(*params) for optlen. But it's not enough, as
> > params->srs_number_streams should also match optlen.
> >
> > If the streams in params->srs_stream_list are less than stream
> > nums in params->srs_number_streams, later when dereferencing
> > the stream list, it could cause a slab-out-of-bounds crash, as
> > reported by syzbot.
> >
> > This patch is to fix it by also checking the stream numbers in
> > sctp_setsockopt_reset_streams to make sure at least it's not
> > greater than the streams in the list.
> >
> > Fixes: 7f9d68ac944e ("sctp: implement sender-side procedures for SSN Reset Request Parameter")
> > Reported-by: Dmitry Vyukov <dvyukov@google.com>
> > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > ---
> > net/sctp/socket.c | 6 +++++-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > index 014847e..dbf140d 100644
> > --- a/net/sctp/socket.c
> > +++ b/net/sctp/socket.c
> > @@ -3891,13 +3891,17 @@ static int sctp_setsockopt_reset_streams(struct sock *sk,
> > struct sctp_association *asoc;
> > int retval = -EINVAL;
> >
> > - if (optlen < sizeof(struct sctp_reset_streams))
> > + if (optlen < sizeof(*params))
> > return -EINVAL;
> >
> Is this going to work in all corner cases? IIRC struct
> sctp_reset_stream has variable length array at the end of it, and so
> sizeof(struct sctp_reset_streams) returns just the size of the
> struct, while sizeof(*params) returns the size of the entire object
> (including the array elements). If a user space task allocates a
I don't think it can include the array elements as such information
can't be passed from the application to the kernel other than via
optlen parameter. There is no metadata around the struct that could
allow that, and sizeof() is a constant, it can't be assuming different
values in runtime.
Cheers,
Marcelo
> static memory block to hold this struct and the array, and passes it
> in with a shorter optlen (if for example, they do not intend to use
> all the array elements), this will cause a failure, where no failure
> is truly warranted. It seems the correct check to me should be the
> origional sizeof(struct sctp_reset_streams) check, and the below
> check to ensure that there are at least the same number of array
> elements available as indicated in srs_nuber_streams.
>
> Regards
> Neil
>
> > params = memdup_user(optval, optlen);
> > if (IS_ERR(params))
> > return PTR_ERR(params);
> >
> > + if (params->srs_number_streams * sizeof(__u16) >
> > + optlen - sizeof(*params))
> > + goto out;
> > +
> > asoc = sctp_id2assoc(sk, params->srs_assoc_id);
> > if (!asoc)
> > goto out;
> > --
> > 2.1.0
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] sctp: make sure stream nums can match optlen in sctp_setsockopt_reset_streams
2017-12-11 15:36 ` Marcelo Ricardo Leitner
@ 2017-12-11 18:28 ` Neil Horman
0 siblings, 0 replies; 6+ messages in thread
From: Neil Horman @ 2017-12-11 18:28 UTC (permalink / raw)
To: Marcelo Ricardo Leitner
Cc: Xin Long, network dev, linux-sctp, davem, syzkaller
On Mon, Dec 11, 2017 at 01:36:39PM -0200, Marcelo Ricardo Leitner wrote:
> Hi,
>
> On Mon, Dec 11, 2017 at 09:54:34AM -0500, Neil Horman wrote:
> > On Sun, Dec 10, 2017 at 03:40:51PM +0800, Xin Long wrote:
> > > Now in sctp_setsockopt_reset_streams, it only does the check
> > > optlen < sizeof(*params) for optlen. But it's not enough, as
> > > params->srs_number_streams should also match optlen.
> > >
> > > If the streams in params->srs_stream_list are less than stream
> > > nums in params->srs_number_streams, later when dereferencing
> > > the stream list, it could cause a slab-out-of-bounds crash, as
> > > reported by syzbot.
> > >
> > > This patch is to fix it by also checking the stream numbers in
> > > sctp_setsockopt_reset_streams to make sure at least it's not
> > > greater than the streams in the list.
> > >
> > > Fixes: 7f9d68ac944e ("sctp: implement sender-side procedures for SSN Reset Request Parameter")
> > > Reported-by: Dmitry Vyukov <dvyukov@google.com>
> > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > ---
> > > net/sctp/socket.c | 6 +++++-
> > > 1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > > index 014847e..dbf140d 100644
> > > --- a/net/sctp/socket.c
> > > +++ b/net/sctp/socket.c
> > > @@ -3891,13 +3891,17 @@ static int sctp_setsockopt_reset_streams(struct sock *sk,
> > > struct sctp_association *asoc;
> > > int retval = -EINVAL;
> > >
> > > - if (optlen < sizeof(struct sctp_reset_streams))
> > > + if (optlen < sizeof(*params))
> > > return -EINVAL;
> > >
> > Is this going to work in all corner cases? IIRC struct
> > sctp_reset_stream has variable length array at the end of it, and so
> > sizeof(struct sctp_reset_streams) returns just the size of the
> > struct, while sizeof(*params) returns the size of the entire object
> > (including the array elements). If a user space task allocates a
>
> I don't think it can include the array elements as such information
> can't be passed from the application to the kernel other than via
> optlen parameter. There is no metadata around the struct that could
> allow that, and sizeof() is a constant, it can't be assuming different
> values in runtime.
>
> Cheers,
> Marcelo
>%
Yup, you're right, its fine.
Acked-by: Neil Horman <nhorman@tuxdriver.com>
> > static memory block to hold this struct and the array, and passes it
> > in with a shorter optlen (if for example, they do not intend to use
> > all the array elements), this will cause a failure, where no failure
> > is truly warranted. It seems the correct check to me should be the
> > origional sizeof(struct sctp_reset_streams) check, and the below
> > check to ensure that there are at least the same number of array
> > elements available as indicated in srs_nuber_streams.
> >
> > Regards
> > Neil
> >
> > > params = memdup_user(optval, optlen);
> > > if (IS_ERR(params))
> > > return PTR_ERR(params);
> > >
> > > + if (params->srs_number_streams * sizeof(__u16) >
> > > + optlen - sizeof(*params))
> > > + goto out;
> > > +
> > > asoc = sctp_id2assoc(sk, params->srs_assoc_id);
> > > if (!asoc)
> > > goto out;
> > > --
> > > 2.1.0
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] sctp: make sure stream nums can match optlen in sctp_setsockopt_reset_streams
2017-12-10 7:40 [PATCH net] sctp: make sure stream nums can match optlen in sctp_setsockopt_reset_streams Xin Long
2017-12-10 13:51 ` Marcelo Ricardo Leitner
2017-12-11 14:54 ` Neil Horman
@ 2017-12-11 19:09 ` David Miller
2 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2017-12-11 19:09 UTC (permalink / raw)
To: lucien.xin; +Cc: netdev, linux-sctp, marcelo.leitner, nhorman, syzkaller
From: Xin Long <lucien.xin@gmail.com>
Date: Sun, 10 Dec 2017 15:40:51 +0800
> Now in sctp_setsockopt_reset_streams, it only does the check
> optlen < sizeof(*params) for optlen. But it's not enough, as
> params->srs_number_streams should also match optlen.
>
> If the streams in params->srs_stream_list are less than stream
> nums in params->srs_number_streams, later when dereferencing
> the stream list, it could cause a slab-out-of-bounds crash, as
> reported by syzbot.
>
> This patch is to fix it by also checking the stream numbers in
> sctp_setsockopt_reset_streams to make sure at least it's not
> greater than the streams in the list.
>
> Fixes: 7f9d68ac944e ("sctp: implement sender-side procedures for SSN Reset Request Parameter")
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
Applied and queued up for -stable, thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-12-11 19:09 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-12-10 7:40 [PATCH net] sctp: make sure stream nums can match optlen in sctp_setsockopt_reset_streams Xin Long
2017-12-10 13:51 ` Marcelo Ricardo Leitner
2017-12-11 14:54 ` Neil Horman
2017-12-11 15:36 ` Marcelo Ricardo Leitner
2017-12-11 18:28 ` Neil Horman
2017-12-11 19:09 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).