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

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

[Kuniyuki Iwashima]

Brad Spencer provided a detailed report [0] 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 initialise the variable before calling
getsockopt() and do not strictly check the returned 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.
As a result, only 1 byte is set to optval.

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

Note that this changes the behaviour on big-endian systems, but we document
that the size of optval is int in the man page.

  $ man 7 netlink
  ...
  Socket options
       To set or get a netlink socket option, call getsockopt(2) to read
       or setsockopt(2) to write the option with the option level argument
       set to SOL_NETLINK.  Unless otherwise noted, optval is a pointer to
       an int.

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>
---
v2:
  * Keep the len check and setting

v1: https://lore.kernel.org/netdev/20230419004246.25770-1-kuniyu@amazon.com/
---
 net/netlink/af_netlink.c | 61 +++++++++++-----------------------------
 1 file changed, 17 insertions(+), 44 deletions(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index f365dfdd672d..5c0d17b3984c 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;
@@ -1753,40 +1753,27 @@ static int netlink_getsockopt(struct socket *sock, int level, int optname,
 		return -EINVAL;
 
 	switch (optname) {
-	case NETLINK_PKTINFO:
+	case NETLINK_LIST_MEMBERSHIPS:
+		break;
+	default:
 		if (len < sizeof(int))
 			return -EINVAL;
 		len = sizeof(int);
+	}
+
+	switch (optname) {
+	case NETLINK_PKTINFO:
 		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 +1790,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 v2 net] netlink: Use copy_to_user() for optval in netlink_getsockopt().

[Jakub Kicinski]

On Thu, 20 Apr 2023 16:33:51 -0700 Kuniyuki Iwashima wrote:
> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> index f365dfdd672d..5c0d17b3984c 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;
> @@ -1753,40 +1753,27 @@ static int netlink_getsockopt(struct socket *sock, int level, int optname,
>  		return -EINVAL;
>  
>  	switch (optname) {
> -	case NETLINK_PKTINFO:
> +	case NETLINK_LIST_MEMBERSHIPS:
> +		break;
> +	default:
>  		if (len < sizeof(int))
>  			return -EINVAL;
>  		len = sizeof(int);
> +	}


> -		err = 0;
>  		break;
>  	default:
> -		err = -ENOPROTOOPT;
> +		return -ENOPROTOOPT;
>  	}
> -	return err;
> +
> +	if (put_user(len, optlen) ||
> +	    copy_to_user(optval, &val, len))
> +		return -EFAULT;

Maybe this is a nit pick but we'd unnecessarily change the return value
to unknown opts when len < 4, from -ENOPROTOOPT to -EINVAL, right?

How about we do:

	unsigned int flag;

	flag = 0;
	switch (optname) {
	case NETLINK_PKTINFO:
		flag = NETLINK_F_RECV_PKTINFO;
 		break;
 	case NETLINK_BROADCAST_ERROR:
		flag = NETLINK_F_BROADCAST_SEND_ERROR;
		break;
	...
	case NETLINK_LIST_MEMBERSHIPS: {
	...
	default:
		return -ENOPROTOOPT;
	}

	if (flag) {
		if (len < sizeof(int))
			return -EINVAL;
		len = sizeof(int);
 		val = nlk->flags & flag ? 1 : 0;
		if (put_user(len, optlen) || 
		    copy_to_user(optval, &val, len))
			return -EFAULT;
		...
	}

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

[Johannes Berg]

On Thu, 2023-04-20 at 23:33 +0000, Kuniyuki Iwashima wrote:
> Brad Spencer provided a detailed report [0] 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.

Nit: not "more than" but "at least" (and sizeof(int), I guess).

> The options return a flag value that fits into 1 byte, but such behaviour
> confuses users who do not initialise the variable before calling
> getsockopt() and do not strictly check the returned 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.
> As a result, only 1 byte is set to optval.

Maybe as a future thing, we should make the getsockopt method prototype
have void here, so this kind of thing becomes a compilation error? That
affects a fair number I guess, though I can't think of any socket
options that really _should_ be just a char, so if it fails anywhere
that might uncover additional bugs (and potentially avoid future ones)?


johannes

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

[Kuniyuki Iwashima]

From:   Jakub Kicinski <kuba@kernel.org>
Date:   Thu, 20 Apr 2023 20:33:24 -0700
> On Thu, 20 Apr 2023 16:33:51 -0700 Kuniyuki Iwashima wrote:
> > diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> > index f365dfdd672d..5c0d17b3984c 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;
> > @@ -1753,40 +1753,27 @@ static int netlink_getsockopt(struct socket *sock, int level, int optname,
> >  		return -EINVAL;
> >  
> >  	switch (optname) {
> > -	case NETLINK_PKTINFO:
> > +	case NETLINK_LIST_MEMBERSHIPS:
> > +		break;
> > +	default:
> >  		if (len < sizeof(int))
> >  			return -EINVAL;
> >  		len = sizeof(int);
> > +	}
> 
> 
> > -		err = 0;
> >  		break;
> >  	default:
> > -		err = -ENOPROTOOPT;
> > +		return -ENOPROTOOPT;
> >  	}
> > -	return err;
> > +
> > +	if (put_user(len, optlen) ||
> > +	    copy_to_user(optval, &val, len))
> > +		return -EFAULT;
> 
> Maybe this is a nit pick but we'd unnecessarily change the return value
> to unknown opts when len < 4, from -ENOPROTOOPT to -EINVAL, right?

Exactly, we should not change it.

> 
> How about we do:

This is cleaner, will do in v3.

Thanks!

> 
> 	unsigned int flag;
> 
> 	flag = 0;
> 	switch (optname) {
> 	case NETLINK_PKTINFO:
> 		flag = NETLINK_F_RECV_PKTINFO;
>  		break;
>  	case NETLINK_BROADCAST_ERROR:
> 		flag = NETLINK_F_BROADCAST_SEND_ERROR;
> 		break;
> 	...
> 	case NETLINK_LIST_MEMBERSHIPS: {
> 	...
> 	default:
> 		return -ENOPROTOOPT;
> 	}
> 
> 	if (flag) {
> 		if (len < sizeof(int))
> 			return -EINVAL;
> 		len = sizeof(int);
>  		val = nlk->flags & flag ? 1 : 0;
> 		if (put_user(len, optlen) || 
> 		    copy_to_user(optval, &val, len))
> 			return -EFAULT;
> 		...
> 	}

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

[Kuniyuki Iwashima]

From:   Johannes Berg <johannes@sipsolutions.net>
Date:   Fri, 21 Apr 2023 09:56:37 +0200
> On Thu, 2023-04-20 at 23:33 +0000, Kuniyuki Iwashima wrote:
> > Brad Spencer provided a detailed report [0] 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.
> 
> Nit: not "more than" but "at least" (and sizeof(int), I guess).

Will change in v3.

> 
> > The options return a flag value that fits into 1 byte, but such behaviour
> > confuses users who do not initialise the variable before calling
> > getsockopt() and do not strictly check the returned 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.
> > As a result, only 1 byte is set to optval.
> 
> Maybe as a future thing, we should make the getsockopt method prototype
> have void here, so this kind of thing becomes a compilation error? That
> affects a fair number I guess, though I can't think of any socket
> options that really _should_ be just a char, so if it fails anywhere
> that might uncover additional bugs (and potentially avoid future ones)?

Ah, cool, we can uncover the same issue easily by doing so and
fix it unless the handler accepts char.

Thanks!