Re: [PATCH] net: sctp, forbid negative length
From: Jiri SlabyDate: Fri, 21 Oct 2016 14:13:24 +0200 > Most of getsockopt handlers in net/sctp/socket.c check len against > sizeof some structure like: > if (len < sizeof(int)) > return -EINVAL; > > On the first look, the check seems to be correct. But since len is int > and sizeof returns size_t, int gets promoted to unsigned size_t too. So > the test returns false for negative lengths. Yes, (-1 < sizeof(long)) is > false. > > Fix this in sctp by explicitly checking len < 0 before any getsockopt > handler is called. > > Note that sctp_getsockopt_events already handled the negative case. > Since we added the < 0 check elsewhere, this one can be removed. > > If not checked, this is the result: ... > Signed-off-by: Jiri Slaby Applied and queued up for -stable, thanks.
Re: [PATCH] net: sctp, forbid negative length
On Fri, Oct 21, 2016 at 02:13:24PM +0200, Jiri Slaby wrote: > Most of getsockopt handlers in net/sctp/socket.c check len against > sizeof some structure like: > if (len < sizeof(int)) > return -EINVAL; > > On the first look, the check seems to be correct. But since len is int > and sizeof returns size_t, int gets promoted to unsigned size_t too. So > the test returns false for negative lengths. Yes, (-1 < sizeof(long)) is > false. > > Fix this in sctp by explicitly checking len < 0 before any getsockopt > handler is called. > > Note that sctp_getsockopt_events already handled the negative case. > Since we added the < 0 check elsewhere, this one can be removed. > > If not checked, this is the result: > UBSAN: Undefined behaviour in ../mm/page_alloc.c:2722:19 > shift exponent 52 is too large for 32-bit type 'int' > CPU: 1 PID: 24535 Comm: syz-executor Not tainted 4.8.1-0-syzkaller #1 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS > rel-1.9.1-0-gb3ef39f-prebuilt.qemu-project.org 04/01/2014 > 88006d99f2a8 b2f7bdea 41b58ab3 > b4363c14 b2f7bcde 88006d99f2d0 88006d99f270 > 0034 b5096422 > Call Trace: > [] ? __ubsan_handle_shift_out_of_bounds+0x29c/0x300 > ... > [] ? kmalloc_order+0x24/0x90 > [] ? kmalloc_order_trace+0x24/0x220 > [] ? __kmalloc+0x330/0x540 > [] ? sctp_getsockopt_local_addrs+0x174/0xca0 [sctp] > [] ? sctp_getsockopt+0x10d/0x1b0 [sctp] > [] ? sock_common_getsockopt+0xb9/0x150 > [] ? SyS_getsockopt+0x1a5/0x270 > > Signed-off-by: Jiri Slaby> Cc: Vlad Yasevich > Cc: Neil Horman > Cc: "David S. Miller" > Cc: linux-s...@vger.kernel.org > Cc: netdev@vger.kernel.org > --- > net/sctp/socket.c | 5 - > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c > index fb02c7033307..9fbb6feb8c27 100644 > --- a/net/sctp/socket.c > +++ b/net/sctp/socket.c > @@ -4687,7 +4687,7 @@ static int sctp_getsockopt_disable_fragments(struct > sock *sk, int len, > static int sctp_getsockopt_events(struct sock *sk, int len, char __user > *optval, > int __user *optlen) > { > - if (len <= 0) > + if (len == 0) > return -EINVAL; > if (len > sizeof(struct sctp_event_subscribe)) > len = sizeof(struct sctp_event_subscribe); > @@ -6430,6 +6430,9 @@ static int sctp_getsockopt(struct sock *sk, int level, > int optname, > if (get_user(len, optlen)) > return -EFAULT; > > + if (len < 0) > + return -EINVAL; > + > lock_sock(sk); > > switch (optname) { > -- > 2.10.1 > > Acked-by: Neil Horman
[PATCH] net: sctp, forbid negative length
Most of getsockopt handlers in net/sctp/socket.c check len against sizeof some structure like: if (len < sizeof(int)) return -EINVAL; On the first look, the check seems to be correct. But since len is int and sizeof returns size_t, int gets promoted to unsigned size_t too. So the test returns false for negative lengths. Yes, (-1 < sizeof(long)) is false. Fix this in sctp by explicitly checking len < 0 before any getsockopt handler is called. Note that sctp_getsockopt_events already handled the negative case. Since we added the < 0 check elsewhere, this one can be removed. If not checked, this is the result: UBSAN: Undefined behaviour in ../mm/page_alloc.c:2722:19 shift exponent 52 is too large for 32-bit type 'int' CPU: 1 PID: 24535 Comm: syz-executor Not tainted 4.8.1-0-syzkaller #1 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.9.1-0-gb3ef39f-prebuilt.qemu-project.org 04/01/2014 88006d99f2a8 b2f7bdea 41b58ab3 b4363c14 b2f7bcde 88006d99f2d0 88006d99f270 0034 b5096422 Call Trace: [] ? __ubsan_handle_shift_out_of_bounds+0x29c/0x300 ... [] ? kmalloc_order+0x24/0x90 [] ? kmalloc_order_trace+0x24/0x220 [] ? __kmalloc+0x330/0x540 [] ? sctp_getsockopt_local_addrs+0x174/0xca0 [sctp] [] ? sctp_getsockopt+0x10d/0x1b0 [sctp] [] ? sock_common_getsockopt+0xb9/0x150 [] ? SyS_getsockopt+0x1a5/0x270 Signed-off-by: Jiri SlabyCc: Vlad Yasevich Cc: Neil Horman Cc: "David S. Miller" Cc: linux-s...@vger.kernel.org Cc: netdev@vger.kernel.org --- net/sctp/socket.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/net/sctp/socket.c b/net/sctp/socket.c index fb02c7033307..9fbb6feb8c27 100644 --- a/net/sctp/socket.c +++ b/net/sctp/socket.c @@ -4687,7 +4687,7 @@ static int sctp_getsockopt_disable_fragments(struct sock *sk, int len, static int sctp_getsockopt_events(struct sock *sk, int len, char __user *optval, int __user *optlen) { - if (len <= 0) + if (len == 0) return -EINVAL; if (len > sizeof(struct sctp_event_subscribe)) len = sizeof(struct sctp_event_subscribe); @@ -6430,6 +6430,9 @@ static int sctp_getsockopt(struct sock *sk, int level, int optname, if (get_user(len, optlen)) return -EFAULT; + if (len < 0) + return -EINVAL; + lock_sock(sk); switch (optname) { -- 2.10.1