[PATCH net] sctp: fix sockopt size check

[PATCH net] sctp: fix sockopt size check

[Marcelo Ricardo Leitner]

The problem is not on being bigger than what we want, but on being
smaller, as it causes read of invalid memory.

Note that the struct changes on commit 7e8616d8e773 didn't affect
sctp_setsockopt_events one but that's where this check was flipped.

Fixes: 7e8616d8e773 ("[SCTP]: Update AUTH structures to match
declarations in draft-16.")
Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.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 1425ec2bbd5ae359a8e0408a89a6da6bb60bd87e..6c4f0dac2104d38ba6420ce6740224866a2ece82 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -2195,7 +2195,7 @@ static int sctp_setsockopt_events(struct sock *sk, char __user *optval,
 	struct sctp_association *asoc;
 	struct sctp_ulpevent *event;
 
-	if (optlen > sizeof(struct sctp_event_subscribe))
+	if (optlen < sizeof(struct sctp_event_subscribe))
 		return -EINVAL;
 	if (copy_from_user(&sctp_sk(sk)->subscribe, optval, optlen))
 		return -EFAULT;
-- 
2.4.3

Re: [PATCH net] sctp: fix sockopt size check

[David Miller]

From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Date: Tue, 28 Jul 2015 11:16:23 -0300

> The problem is not on being bigger than what we want, but on being
> smaller, as it causes read of invalid memory.
> 
> Note that the struct changes on commit 7e8616d8e773 didn't affect
> sctp_setsockopt_events one but that's where this check was flipped.
> 
> Fixes: 7e8616d8e773 ("[SCTP]: Update AUTH structures to match
> declarations in draft-16.")
> Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

This makes things worse.

The copy_from_user() call is bounded by optlen, so if you allow it to
be any arbitrary large value the user can write past the end of the
structure, corrupting kernel memory.

No, the test is correct, or at least necessary, as-is.

Re: [PATCH net] sctp: fix sockopt size check

[Marcelo Ricardo Leitner]

On Wed, Jul 29, 2015 at 05:07:31PM -0700, David Miller wrote:
> From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> Date: Tue, 28 Jul 2015 11:16:23 -0300
> 
> > The problem is not on being bigger than what we want, but on being
> > smaller, as it causes read of invalid memory.
> > 
> > Note that the struct changes on commit 7e8616d8e773 didn't affect
> > sctp_setsockopt_events one but that's where this check was flipped.
> > 
> > Fixes: 7e8616d8e773 ("[SCTP]: Update AUTH structures to match
> > declarations in draft-16.")
> > Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> 
> This makes things worse.
> 
> The copy_from_user() call is bounded by optlen, so if you allow it to
> be any arbitrary large value the user can write past the end of the
> structure, corrupting kernel memory.

Indeed. I should have changed copy_from_user() to copy the size of the
struct too. But then the issue I thought there was, there isn't and it
just allows partial updates, as it won't read any further than optlen.

> No, the test is correct, or at least necessary, as-is.

Yes. Please drop this. Thanks.

  Marcelo