[PATCH v1 net] netlink: Use copy_to_user() for optval in netlink_getsockopt().

[PATCH v1 net] netlink: Use copy_to_user() for optval in netlink_getsockopt().

[Kuniyuki Iwashima]

Brad Spencer provided a detailed report that when calling getsockopt()
for AF_NETLINK, some SOL_NETLINK options set only 1 byte even though such
options require more than int as length.

The options return a flag value that fits into 1 byte, but such behaviour
confuses users who do not strictly check the value as char.

Currently, netlink_getsockopt() uses put_user() to copy data to optlen and
optval, but put_user() casts the data based on the pointer, char *optval.
So, only 1 byte is set to optval.

To avoid this behaviour, we need to use copy_to_user() or cast optval for
put_user().

Now getsockopt() accepts char as optval as the flags are only 1 byte.

Fixes: 9a4595bc7e67 ("[NETLINK]: Add set/getsockopt options to support more than 32 groups")
Fixes: be0c22a46cfb ("netlink: add NETLINK_BROADCAST_ERROR socket option")
Fixes: 38938bfe3489 ("netlink: add NETLINK_NO_ENOBUFS socket flag")
Fixes: 0a6a3a23ea6e ("netlink: add NETLINK_CAP_ACK socket option")
Fixes: 2d4bc93368f5 ("netlink: extended ACK reporting")
Fixes: 89d35528d17d ("netlink: Add new socket option to enable strict checking on dumps")
Reported-by: Brad Spencer <bspencer@blackberry.com>
Link: https://lore.kernel.org/netdev/ZD7VkNWFfp22kTDt@datsun.rim.net/
Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/netlink/af_netlink.c | 56 +++++++---------------------------------
 1 file changed, 10 insertions(+), 46 deletions(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index f365dfdd672d..3106f09d5f1c 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1742,7 +1742,7 @@ static int netlink_getsockopt(struct socket *sock, int level, int optname,
 {
 	struct sock *sk = sock->sk;
 	struct netlink_sock *nlk = nlk_sk(sk);
-	int len, val, err;
+	int len, val;
 
 	if (level != SOL_NETLINK)
 		return -ENOPROTOOPT;
@@ -1754,39 +1754,17 @@ static int netlink_getsockopt(struct socket *sock, int level, int optname,
 
 	switch (optname) {
 	case NETLINK_PKTINFO:
-		if (len < sizeof(int))
-			return -EINVAL;
-		len = sizeof(int);
 		val = nlk->flags & NETLINK_F_RECV_PKTINFO ? 1 : 0;
-		if (put_user(len, optlen) ||
-		    put_user(val, optval))
-			return -EFAULT;
-		err = 0;
 		break;
 	case NETLINK_BROADCAST_ERROR:
-		if (len < sizeof(int))
-			return -EINVAL;
-		len = sizeof(int);
 		val = nlk->flags & NETLINK_F_BROADCAST_SEND_ERROR ? 1 : 0;
-		if (put_user(len, optlen) ||
-		    put_user(val, optval))
-			return -EFAULT;
-		err = 0;
 		break;
 	case NETLINK_NO_ENOBUFS:
-		if (len < sizeof(int))
-			return -EINVAL;
-		len = sizeof(int);
 		val = nlk->flags & NETLINK_F_RECV_NO_ENOBUFS ? 1 : 0;
-		if (put_user(len, optlen) ||
-		    put_user(val, optval))
-			return -EFAULT;
-		err = 0;
 		break;
 	case NETLINK_LIST_MEMBERSHIPS: {
-		int pos, idx, shift;
+		int pos, idx, shift, err = 0;
 
-		err = 0;
 		netlink_lock_table();
 		for (pos = 0; pos * 8 < nlk->ngroups; pos += sizeof(u32)) {
 			if (len - pos < sizeof(u32))
@@ -1803,40 +1781,26 @@ static int netlink_getsockopt(struct socket *sock, int level, int optname,
 		if (put_user(ALIGN(nlk->ngroups / 8, sizeof(u32)), optlen))
 			err = -EFAULT;
 		netlink_unlock_table();
-		break;
+		return err;
 	}
 	case NETLINK_CAP_ACK:
-		if (len < sizeof(int))
-			return -EINVAL;
-		len = sizeof(int);
 		val = nlk->flags & NETLINK_F_CAP_ACK ? 1 : 0;
-		if (put_user(len, optlen) ||
-		    put_user(val, optval))
-			return -EFAULT;
-		err = 0;
 		break;
 	case NETLINK_EXT_ACK:
-		if (len < sizeof(int))
-			return -EINVAL;
-		len = sizeof(int);
 		val = nlk->flags & NETLINK_F_EXT_ACK ? 1 : 0;
-		if (put_user(len, optlen) || put_user(val, optval))
-			return -EFAULT;
-		err = 0;
 		break;
 	case NETLINK_GET_STRICT_CHK:
-		if (len < sizeof(int))
-			return -EINVAL;
-		len = sizeof(int);
 		val = nlk->flags & NETLINK_F_STRICT_CHK ? 1 : 0;
-		if (put_user(len, optlen) || put_user(val, optval))
-			return -EFAULT;
-		err = 0;
 		break;
 	default:
-		err = -ENOPROTOOPT;
+		return -ENOPROTOOPT;
 	}
-	return err;
+
+	if (put_user(len, optlen) ||
+	    copy_to_user(optval, &val, len))
+		return -EFAULT;
+
+	return 0;
 }
 
 static void netlink_cmsg_recv_pktinfo(struct msghdr *msg, struct sk_buff *skb)
-- 
2.30.2

Re: [PATCH v1 net] netlink: Use copy_to_user() for optval in netlink_getsockopt().

[Jakub Kicinski]

On Tue, 18 Apr 2023 17:42:46 -0700 Kuniyuki Iwashima wrote:
> Brad Spencer provided a detailed report that when calling getsockopt()
> for AF_NETLINK, some SOL_NETLINK options set only 1 byte even though such
> options require more than int as length.
> 
> The options return a flag value that fits into 1 byte, but such behaviour
> confuses users who do not strictly check the value as char.
> 
> Currently, netlink_getsockopt() uses put_user() to copy data to optlen and
> optval, but put_user() casts the data based on the pointer, char *optval.
> So, only 1 byte is set to optval.
> 
> To avoid this behaviour, we need to use copy_to_user() or cast optval for
> put_user().
> 
> Now getsockopt() accepts char as optval as the flags are only 1 byte.

I think it's worth doing, but it will change the return value on big
endian, right?

Re: [PATCH v1 net] netlink: Use copy_to_user() for optval in netlink_getsockopt().

[Kuniyuki Iwashima]

From:   Jakub Kicinski <kuba@kernel.org>
Date:   Tue, 18 Apr 2023 20:33:18 -0700
> On Tue, 18 Apr 2023 17:42:46 -0700 Kuniyuki Iwashima wrote:
> > Brad Spencer provided a detailed report that when calling getsockopt()
> > for AF_NETLINK, some SOL_NETLINK options set only 1 byte even though such
> > options require more than int as length.
> > 
> > The options return a flag value that fits into 1 byte, but such behaviour
> > confuses users who do not strictly check the value as char.
> > 
> > Currently, netlink_getsockopt() uses put_user() to copy data to optlen and
> > optval, but put_user() casts the data based on the pointer, char *optval.
> > So, only 1 byte is set to optval.
> > 
> > To avoid this behaviour, we need to use copy_to_user() or cast optval for
> > put_user().
> > 
> > Now getsockopt() accepts char as optval as the flags are only 1 byte.
> 
> I think it's worth doing, but it will change the return value on big
> endian, right?

Ah, I missed that point, yes.  I hope big endian users always
initialise the value to avoid the tricky behaviour.

Re: [PATCH v1 net] netlink: Use copy_to_user() for optval in netlink_getsockopt().

[Johannes Berg]

On Wed, 2023-04-19 at 00:42 +0000, Kuniyuki Iwashima wrote:
> Brad Spencer provided a detailed report that when calling getsockopt()
> for AF_NETLINK, some SOL_NETLINK options set only 1 byte even though such
> options require more than int as length.
> 
> The options return a flag value that fits into 1 byte, but such behaviour
> confuses users who do not strictly check the value as char.

Yeah that's iffy. I guess nobody really leaves it uninitialized.

> Currently, netlink_getsockopt() uses put_user() to copy data to optlen and
> optval, but put_user() casts the data based on the pointer, char *optval.
> So, only 1 byte is set to optval.

Which also means it only ever sets to "1" on little endian systems, on
big endian it would set to "0x0100'0000" (if initialized to 0 first),
right?

> To avoid this behaviour, we need to use copy_to_user() or cast optval for
> put_user().

Right.

> Now getsockopt() accepts char as optval as the flags are only 1 byte.

Personally, I don't think we should allow his. We document (*) and check
this as taking an int, and while it would _fit_, I don't really think
it's a good idea to weaken this and allow different types.
I don't see value in it either, certainly not now since nobody can be
using it, and not really in the future either since you're not going to
pack these things in memory, and having an int vs. char on the stack
really makes basically no difference.
And when we start seeing code that actually uses this, it'll just be
more things to support in the userspace API, be more confusing with
socket options that aren't just flags, etc.

IOW, I think you should keep the size checks.


(*) man 7 netlink:
"Unless otherwise noted, optval is a pointer to an int."


> @@ -1754,39 +1754,17 @@ static int netlink_getsockopt(struct socket *sock, int level, int optname,
> 
>         switch (optname) {
>         case NETLINK_PKTINFO:
> -               if (len < sizeof(int))
> -                       return -EINVAL;
> -               len = sizeof(int);

On the other hand, this is actually accepting say a u64 now, and then
sets only 4 bytes of it, though at least it also sets the size to what
it wrote out.

So I guess here we can argue either
 1) keep writing len to 4 and set 4 bytes of the output
 2) keep the length as is and set all bytes of the output

but (2) gets confusing if you say used 6 bytes buffer as input? I mean,
yeah, I'd really hope nobody does that.

If Jakub is feeling adventurous maybe we should attempt to see if we
break anything by accepting only == sizeof(int) rather than >= ... :-)


> +       if (put_user(len, optlen) ||

You never change len now, so there's no point writing it back? Or do we
somehow need to make sure this is writable? But what for?

> +           copy_to_user(optval, &val, len))

There's some magic in copy_to_user() now, but I don't think it will save
you here - to me this seems really wrong now because 'len' is controlled
by the user, and sizeof(val) is only 4 bytes - so wouldn't this overrun
even in the case I mentioned above where the user used a u64 and 'len'
is actually 8, not 4? 

Also, as Jakub points out, even in the case where len *is* 4, you've now
changed the behaviour on big endian.

I think that's probably *fine* since the bug meant you basically had to
initialise to 0 and then check the entire value though, but maybe that
warrants some discussion in the commit log.

So my 2 cents:
 * I wouldn't remove the checks that the size is at least sizeof(int)
 * I'd - even if it's not strictly backwards compatible - think about
   restricting to *exactly* sizeof(int), which would make the issue
   with the copy_to_user() go away as well (**)
 * if we don't restrict the input length, then need to be much more
   careful about the copy_to_user() I think, but then what if someone
   specifies something really huge as the size?


(**) I only worry slightly that today somebody could've used an
uninitialised value as the optlen and gotten away with it, but I hope
that's not the case, that'd be a strange pattern, and if you ever hit 0
it fails anyway. I'm not really worried someone explicitly wanted to use
a bigger buffer.


johannes

Re: [PATCH v1 net] netlink: Use copy_to_user() for optval in netlink_getsockopt().

[Kuniyuki Iwashima]

From:   Johannes Berg <johannes@sipsolutions.net>
Date:   Wed, 19 Apr 2023 09:17:37 +0200
> On Wed, 2023-04-19 at 00:42 +0000, Kuniyuki Iwashima wrote:
> > Brad Spencer provided a detailed report that when calling getsockopt()
> > for AF_NETLINK, some SOL_NETLINK options set only 1 byte even though such
> > options require more than int as length.
> > 
> > The options return a flag value that fits into 1 byte, but such behaviour
> > confuses users who do not strictly check the value as char.
> 
> Yeah that's iffy. I guess nobody really leaves it uninitialized.
> 
> > Currently, netlink_getsockopt() uses put_user() to copy data to optlen and
> > optval, but put_user() casts the data based on the pointer, char *optval.
> > So, only 1 byte is set to optval.
> 
> Which also means it only ever sets to "1" on little endian systems, on
> big endian it would set to "0x0100'0000" (if initialized to 0 first),
> right?

Yes.


> 
> > To avoid this behaviour, we need to use copy_to_user() or cast optval for
> > put_user().
> 
> Right.
> 
> > Now getsockopt() accepts char as optval as the flags are only 1 byte.
> 
> Personally, I don't think we should allow his. We document (*) and check
> this as taking an int, and while it would _fit_, I don't really think
> it's a good idea to weaken this and allow different types.
> I don't see value in it either, certainly not now since nobody can be
> using it, and not really in the future either since you're not going to
> pack these things in memory, and having an int vs. char on the stack
> really makes basically no difference.
> And when we start seeing code that actually uses this, it'll just be
> more things to support in the userspace API, be more confusing with
> socket options that aren't just flags, etc.
> 
> IOW, I think you should keep the size checks.
> 
> 
> (*) man 7 netlink:
> "Unless otherwise noted, optval is a pointer to an int."

Oh, I didn't know it's documented.

I tried to follow other SOL_XXX handlers, for example, we can
get SO_REUSEADDR as char.  But I'm fine to keep the size check.


> 
> 
> > @@ -1754,39 +1754,17 @@ static int netlink_getsockopt(struct socket *sock, int level, int optname,
> > 
> >         switch (optname) {
> >         case NETLINK_PKTINFO:
> > -               if (len < sizeof(int))
> > -                       return -EINVAL;
> > -               len = sizeof(int);
> 
> On the other hand, this is actually accepting say a u64 now, and then
> sets only 4 bytes of it, though at least it also sets the size to what
> it wrote out.
> 
> So I guess here we can argue either
>  1) keep writing len to 4 and set 4 bytes of the output
>  2) keep the length as is and set all bytes of the output
> 
> but (2) gets confusing if you say used 6 bytes buffer as input? I mean,
> yeah, I'd really hope nobody does that.
> 
> If Jakub is feeling adventurous maybe we should attempt to see if we
> break anything by accepting only == sizeof(int) rather than >= ... :-)

Yes, this is another thing I concerned.  I thought we would have the
same report if we didn't clear the high 32 bits when a user passed u64.

If we want to avoid it, we have to use u64 as val in netlink_getsockopt().
This is even broken for a strange user who passes u128 though :P

> 
> 
> > +       if (put_user(len, optlen) ||
> 
> You never change len now, so there's no point writing it back? Or do we
> somehow need to make sure this is writable? But what for?
> 
> > +           copy_to_user(optval, &val, len))
> 
> There's some magic in copy_to_user() now, but I don't think it will save
> you here - to me this seems really wrong now because 'len' is controlled
> by the user, and sizeof(val) is only 4 bytes - so wouldn't this overrun
> even in the case I mentioned above where the user used a u64 and 'len'
> is actually 8, not 4?

You are right.  I seem to be confused to try to accept char ~ u64 :/
Yes, at least we have to set upper bound for len based on val's actual
size as we do in sk_getsockopt().


> 
> Also, as Jakub points out, even in the case where len *is* 4, you've now
> changed the behaviour on big endian.
> 
> I think that's probably *fine* since the bug meant you basically had to
> initialise to 0 and then check the entire value though, but maybe that
> warrants some discussion in the commit log.

Agreed.


> 
> So my 2 cents:
>  * I wouldn't remove the checks that the size is at least sizeof(int)
>  * I'd - even if it's not strictly backwards compatible - think about
>    restricting to *exactly* sizeof(int), which would make the issue
>    with the copy_to_user() go away as well (**)
>  * if we don't restrict the input length, then need to be much more
>    careful about the copy_to_user() I think, but then what if someone
>    specifies something really huge as the size?

I'm fine either, but I would prefer the latter using u64 for val and
set limit for len as sizeof(u64).


> 
> 
> (**) I only worry slightly that today somebody could've used an
> uninitialised value as the optlen and gotten away with it, but I hope
> that's not the case, that'd be a strange pattern, and if you ever hit 0
> it fails anyway. I'm not really worried someone explicitly wanted to use
> a bigger buffer.
> 
> 
> johannes

Re: [PATCH v1 net] netlink: Use copy_to_user() for optval in netlink_getsockopt().

[Johannes Berg]

On Wed, 2023-04-19 at 10:52 -0700, Kuniyuki Iwashima wrote:

> > So I guess here we can argue either
> >  1) keep writing len to 4 and set 4 bytes of the output
> >  2) keep the length as is and set all bytes of the output
> > 
> > but (2) gets confusing if you say used 6 bytes buffer as input? I mean,
> > yeah, I'd really hope nobody does that.
> > 
> > If Jakub is feeling adventurous maybe we should attempt to see if we
> > break anything by accepting only == sizeof(int) rather than >= ... :-)
> 
> Yes, this is another thing I concerned.  I thought we would have the
> same report if we didn't clear the high 32 bits when a user passed u64.
> 
> If we want to avoid it, we have to use u64 as val in netlink_getsockopt().
> This is even broken for a strange user who passes u128 though :P

Yeah ... hence I'm saying we shouldn't bother.

> > So my 2 cents:
> >  * I wouldn't remove the checks that the size is at least sizeof(int)
> >  * I'd - even if it's not strictly backwards compatible - think about
> >    restricting to *exactly* sizeof(int), which would make the issue
> >    with the copy_to_user() go away as well (**)
> >  * if we don't restrict the input length, then need to be much more
> >    careful about the copy_to_user() I think, but then what if someone
> >    specifies something really huge as the size?
> 
> I'm fine either, but I would prefer the latter using u64 for val and
> set limit for len as sizeof(u64).
> 

That doesn't actually work on big endian, if you have a u64 value 1
that's 00 00 00 00 00 00 00 01, now if you just copy_to_user() that to
an int value (that should've been used) you only get 00 00 00 00 out ...
I guess with the semantics we could technically set it to ff ... ff, but
then there's probably _someone_ out there who expects only 0 or 1 in an
int, or something? So that means to allow a value larger than int
(smaller isn't allowed now) you'd actually have to write some additional
tricky code that checks what the size is and writes it accordingly ...
or just like now writes only the 4 bytes out and sets optlen, but I
suspects that really only works today anyway because everyone uses an
int as documented (and smaller won't work).

So I still think it's better to at least try to just say it _has_ to be
exactly an int, and write that out - and failing that, keep the
behaviour we have today, but hopefully that won't be needed.

johannes

Re: [PATCH v1 net] netlink: Use copy_to_user() for optval in netlink_getsockopt().

[Johannes Berg]

On Wed, 2023-04-19 at 21:46 +0200, Johannes Berg wrote:
> 
> > > So my 2 cents:
> > >  * I wouldn't remove the checks that the size is at least sizeof(int)
> > >  * I'd - even if it's not strictly backwards compatible - think about
> > >    restricting to *exactly* sizeof(int), which would make the issue
> > >    with the copy_to_user() go away as well (**)
> > >  * if we don't restrict the input length, then need to be much more
> > >    careful about the copy_to_user() I think, but then what if someone
> > >    specifies something really huge as the size?
> > 
> > I'm fine either, but I would prefer the latter using u64 for val and
> > set limit for len as sizeof(u64).
> > 
> 
> That doesn't actually work on big endian,

[snip]

and come to think of it, that also makes the code in your patch now only
work for 'char' or 'short' on little endian, which is again another
tricky gotcha, and also there another argument for not allowing that?

johannes

Re: [PATCH v1 net] netlink: Use copy_to_user() for optval in netlink_getsockopt().

[Kuniyuki Iwashima]

From: Jakub Kicinski <kuba@kernel.org>
Date: Wed, 19 Apr 2023 16:09:08 -0700
> On Wed, 19 Apr 2023 09:17:37 +0200 Johannes Berg wrote:
> > > @@ -1754,39 +1754,17 @@ static int netlink_getsockopt(struct socket *sock, int level, int optname,
> > >
> > >         switch (optname) {
> > >         case NETLINK_PKTINFO:
> > > -               if (len < sizeof(int))
> > > -                       return -EINVAL;
> > > -               len = sizeof(int);
> >
> > On the other hand, this is actually accepting say a u64 now, and then
> > sets only 4 bytes of it, though at least it also sets the size to what
> > it wrote out.
> >
> > So I guess here we can argue either
> >  1) keep writing len to 4 and set 4 bytes of the output
> >  2) keep the length as is and set all bytes of the output
> >
> > but (2) gets confusing if you say used 6 bytes buffer as input? I mean,
> > yeah, I'd really hope nobody does that.
> >
> > If Jakub is feeling adventurous maybe we should attempt to see if we
> > break anything by accepting only == sizeof(int) rather than >= ... :-)
> 
> Can't think of a strong reason either way, so I'd keep the check
> and len setting as is.

Ok, I'll respin v2 with the existing check and len setting.

Thank you, Johannes and Jakub!

Re: [PATCH v1 net] netlink: Use copy_to_user() for optval in netlink_getsockopt().

[Johannes Berg]

On Wed, 2023-04-19 at 16:09 -0700, Jakub Kicinski wrote:
> On Wed, 19 Apr 2023 09:17:37 +0200 Johannes Berg wrote:
> > > @@ -1754,39 +1754,17 @@ static int netlink_getsockopt(struct socket *sock, int level, int optname,
> > > 
> > >         switch (optname) {
> > >         case NETLINK_PKTINFO:
> > > -               if (len < sizeof(int))
> > > -                       return -EINVAL;
> > > -               len = sizeof(int);  
> > 
> > On the other hand, this is actually accepting say a u64 now, and then
> > sets only 4 bytes of it, though at least it also sets the size to what
> > it wrote out.
> > 
> > So I guess here we can argue either
> >  1) keep writing len to 4 and set 4 bytes of the output
> >  2) keep the length as is and set all bytes of the output
> > 
> > but (2) gets confusing if you say used 6 bytes buffer as input? I mean,
> > yeah, I'd really hope nobody does that.
> > 
> > If Jakub is feeling adventurous maybe we should attempt to see if we
> > break anything by accepting only == sizeof(int) rather than >= ... :-)
> 
> Can't think of a strong reason either way, so I'd keep the check 
> and len setting as is.

Yeah, fair. The only reason really would be to make it very clear that
using something bigger than an int isn't going to work right.

johannes