diff mbox series

[net-next,05/20] netlink: Add new socket option to enable strict checking on dumps

Message ID 20181004213355.14899-6-dsahern@kernel.org
State Changes Requested, archived
Delegated to: David Miller
Headers show
Series rtnetlink: Add support for rigid checking of data in dump request | expand

Commit Message

David Ahern Oct. 4, 2018, 9:33 p.m. UTC 
From: David Ahern <dsahern@gmail.com>

Add a new socket option, NETLINK_DUMP_STRICT_CHK, that userspace
can use via setsockopt to request strict checking of headers and
attributes on dump requests.

To get dump features such as kernel side filtering based on data in
the header or attributes appended to the dump request, userspace
must call setsockopt() for NETLINK_DUMP_STRICT_CHK and a non-zero
value. Since the netlink sock and its flags are private to the
af_netlink code, the strict checking flag is passed to dump handlers
via a flag in the netlink_callback struct.

For old userspace on new kernel there is no impact as all of the data
checks in later patches are wrapped in a check on the new strict flag.

For new userspace on old kernel, the setsockopt will fail and even if
new userspace sets data in the headers and appended attributes the
kernel will silently ignore it.

New userspace on new kernel setting the socket option gets the benefit
of the improved data dump.

Kernel side the NETLINK_DUMP_STRICT_CHK uapi is converted to a generic
NETLINK_F_STRICT_CHK flag which can potentially be leveraged for tighter
checking on the NEW, DEL, and SET commands.

Signed-off-by: David Ahern <dsahern@gmail.com>
---
 include/linux/netlink.h      |  2 ++
 include/uapi/linux/netlink.h |  1 +
 net/netlink/af_netlink.c     | 21 ++++++++++++++++++++-
 net/netlink/af_netlink.h     |  1 +
 4 files changed, 24 insertions(+), 1 deletion(-)

Comments

Christian Brauner Oct. 5, 2018, 5:36 p.m. UTC | #1 
On Thu, Oct 04, 2018 at 02:33:40PM -0700, David Ahern wrote:
> From: David Ahern <dsahern@gmail.com>
> 
> Add a new socket option, NETLINK_DUMP_STRICT_CHK, that userspace
> can use via setsockopt to request strict checking of headers and
> attributes on dump requests.
> 
> To get dump features such as kernel side filtering based on data in
> the header or attributes appended to the dump request, userspace
> must call setsockopt() for NETLINK_DUMP_STRICT_CHK and a non-zero
> value. Since the netlink sock and its flags are private to the
> af_netlink code, the strict checking flag is passed to dump handlers
> via a flag in the netlink_callback struct.
> 
> For old userspace on new kernel there is no impact as all of the data
> checks in later patches are wrapped in a check on the new strict flag.
> 
> For new userspace on old kernel, the setsockopt will fail and even if
> new userspace sets data in the headers and appended attributes the
> kernel will silently ignore it.
> 
> New userspace on new kernel setting the socket option gets the benefit
> of the improved data dump.
> 
> Kernel side the NETLINK_DUMP_STRICT_CHK uapi is converted to a generic
> NETLINK_F_STRICT_CHK flag which can potentially be leveraged for tighter
> checking on the NEW, DEL, and SET commands.
> 
> Signed-off-by: David Ahern <dsahern@gmail.com>
> ---
>  include/linux/netlink.h      |  2 ++
>  include/uapi/linux/netlink.h |  1 +
>  net/netlink/af_netlink.c     | 21 ++++++++++++++++++++-
>  net/netlink/af_netlink.h     |  1 +
>  4 files changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/netlink.h b/include/linux/netlink.h
> index 88c8a2d83eb3..36bdca2aa42d 100644
> --- a/include/linux/netlink.h
> +++ b/include/linux/netlink.h
> @@ -179,6 +179,8 @@ struct netlink_callback {
>  	struct netlink_ext_ack	*extack;
>  	u16			family;
>  	u16			min_dump_alloc;
> +	unsigned int		strict_check:1,
> +				unused:31;

I like this idea a lot. :) but I'm not a fan of bitfields if not
necessary. Is that really necessary here?

>  	unsigned int		prev_seq, seq;
>  	long			args[6];
>  };
> diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h
> index 776bc92e9118..486ed1f0c0bc 100644
> --- a/include/uapi/linux/netlink.h
> +++ b/include/uapi/linux/netlink.h
> @@ -155,6 +155,7 @@ enum nlmsgerr_attrs {
>  #define NETLINK_LIST_MEMBERSHIPS	9
>  #define NETLINK_CAP_ACK			10
>  #define NETLINK_EXT_ACK			11
> +#define NETLINK_DUMP_STRICT_CHK		12
>  
>  struct nl_pktinfo {
>  	__u32	group;
> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> index 7ac585f33a9e..e613a9f89600 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
> @@ -1706,6 +1706,13 @@ static int netlink_setsockopt(struct socket *sock, int level, int optname,
>  			nlk->flags &= ~NETLINK_F_EXT_ACK;
>  		err = 0;
>  		break;
> +	case NETLINK_DUMP_STRICT_CHK:
> +		if (val)
> +			nlk->flags |= NETLINK_F_STRICT_CHK;
> +		else
> +			nlk->flags &= ~NETLINK_F_STRICT_CHK;
> +		err = 0;
> +		break;
>  	default:
>  		err = -ENOPROTOOPT;
>  	}
> @@ -1799,6 +1806,15 @@ static int netlink_getsockopt(struct socket *sock, int level, int optname,
>  			return -EFAULT;
>  		err = 0;
>  		break;
> +	case NETLINK_DUMP_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;
>  	}
> @@ -2282,9 +2298,9 @@ int __netlink_dump_start(struct sock *ssk, struct sk_buff *skb,
>  			 const struct nlmsghdr *nlh,
>  			 struct netlink_dump_control *control)
>  {
> +	struct netlink_sock *nlk, *nlk2;
>  	struct netlink_callback *cb;
>  	struct sock *sk;
> -	struct netlink_sock *nlk;
>  	int ret;
>  
>  	refcount_inc(&skb->users);
> @@ -2318,6 +2334,9 @@ int __netlink_dump_start(struct sock *ssk, struct sk_buff *skb,
>  	cb->min_dump_alloc = control->min_dump_alloc;
>  	cb->skb = skb;
>  
> +	nlk2 = nlk_sk(NETLINK_CB(skb).sk);
> +	cb->strict_check = !!(nlk2->flags & NETLINK_F_STRICT_CHK);
> +
>  	if (control->start) {
>  		ret = control->start(cb);
>  		if (ret)
> diff --git a/net/netlink/af_netlink.h b/net/netlink/af_netlink.h
> index 962de7b3c023..5f454c8de6a4 100644
> --- a/net/netlink/af_netlink.h
> +++ b/net/netlink/af_netlink.h
> @@ -15,6 +15,7 @@
>  #define NETLINK_F_LISTEN_ALL_NSID	0x10
>  #define NETLINK_F_CAP_ACK		0x20
>  #define NETLINK_F_EXT_ACK		0x40
> +#define NETLINK_F_STRICT_CHK		0x80
>  
>  #define NLGRPSZ(x)	(ALIGN(x, sizeof(unsigned long) * 8) / 8)
>  #define NLGRPLONGS(x)	(NLGRPSZ(x)/sizeof(unsigned long))
> -- 
> 2.11.0
>
David Ahern Oct. 5, 2018, 6:43 p.m. UTC | #2 
On 10/5/18 11:36 AM, Christian Brauner wrote:
>> diff --git a/include/linux/netlink.h b/include/linux/netlink.h
>> index 88c8a2d83eb3..36bdca2aa42d 100644
>> --- a/include/linux/netlink.h
>> +++ b/include/linux/netlink.h
>> @@ -179,6 +179,8 @@ struct netlink_callback {
>>  	struct netlink_ext_ack	*extack;
>>  	u16			family;
>>  	u16			min_dump_alloc;
>> +	unsigned int		strict_check:1,
>> +				unused:31;
> 
> I like this idea a lot. :) but I'm not a fan of bitfields if not
> necessary. Is that really necessary here?
> 

no strong opinions on a bitfield vs a bool.
Christian Brauner Oct. 5, 2018, 6:45 p.m. UTC | #3 
On October 5, 2018 8:43:55 PM GMT+02:00, David Ahern <dsahern@gmail.com> wrote:
>On 10/5/18 11:36 AM, Christian Brauner wrote:
>>> diff --git a/include/linux/netlink.h b/include/linux/netlink.h
>>> index 88c8a2d83eb3..36bdca2aa42d 100644
>>> --- a/include/linux/netlink.h
>>> +++ b/include/linux/netlink.h
>>> @@ -179,6 +179,8 @@ struct netlink_callback {
>>>  	struct netlink_ext_ack	*extack;
>>>  	u16			family;
>>>  	u16			min_dump_alloc;
>>> +	unsigned int		strict_check:1,
>>> +				unused:31;
>> 
>> I like this idea a lot. :) but I'm not a fan of bitfields if not
>> necessary. Is that really necessary here?
>> 
>
>no strong opinions on a bitfield vs a bool.

Just feels like this is something that is
rarely used. Having a bool or traditional 
flag might be more readable and easier to 
maintain. :)
diff mbox series

Patch

diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index 88c8a2d83eb3..36bdca2aa42d 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -179,6 +179,8 @@  struct netlink_callback {
 	struct netlink_ext_ack	*extack;
 	u16			family;
 	u16			min_dump_alloc;
+	unsigned int		strict_check:1,
+				unused:31;
 	unsigned int		prev_seq, seq;
 	long			args[6];
 };
diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h
index 776bc92e9118..486ed1f0c0bc 100644
--- a/include/uapi/linux/netlink.h
+++ b/include/uapi/linux/netlink.h
@@ -155,6 +155,7 @@  enum nlmsgerr_attrs {
 #define NETLINK_LIST_MEMBERSHIPS	9
 #define NETLINK_CAP_ACK			10
 #define NETLINK_EXT_ACK			11
+#define NETLINK_DUMP_STRICT_CHK		12
 
 struct nl_pktinfo {
 	__u32	group;
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 7ac585f33a9e..e613a9f89600 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1706,6 +1706,13 @@  static int netlink_setsockopt(struct socket *sock, int level, int optname,
 			nlk->flags &= ~NETLINK_F_EXT_ACK;
 		err = 0;
 		break;
+	case NETLINK_DUMP_STRICT_CHK:
+		if (val)
+			nlk->flags |= NETLINK_F_STRICT_CHK;
+		else
+			nlk->flags &= ~NETLINK_F_STRICT_CHK;
+		err = 0;
+		break;
 	default:
 		err = -ENOPROTOOPT;
 	}
@@ -1799,6 +1806,15 @@  static int netlink_getsockopt(struct socket *sock, int level, int optname,
 			return -EFAULT;
 		err = 0;
 		break;
+	case NETLINK_DUMP_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;
 	}
@@ -2282,9 +2298,9 @@  int __netlink_dump_start(struct sock *ssk, struct sk_buff *skb,
 			 const struct nlmsghdr *nlh,
 			 struct netlink_dump_control *control)
 {
+	struct netlink_sock *nlk, *nlk2;
 	struct netlink_callback *cb;
 	struct sock *sk;
-	struct netlink_sock *nlk;
 	int ret;
 
 	refcount_inc(&skb->users);
@@ -2318,6 +2334,9 @@  int __netlink_dump_start(struct sock *ssk, struct sk_buff *skb,
 	cb->min_dump_alloc = control->min_dump_alloc;
 	cb->skb = skb;
 
+	nlk2 = nlk_sk(NETLINK_CB(skb).sk);
+	cb->strict_check = !!(nlk2->flags & NETLINK_F_STRICT_CHK);
+
 	if (control->start) {
 		ret = control->start(cb);
 		if (ret)
diff --git a/net/netlink/af_netlink.h b/net/netlink/af_netlink.h
index 962de7b3c023..5f454c8de6a4 100644
--- a/net/netlink/af_netlink.h
+++ b/net/netlink/af_netlink.h
@@ -15,6 +15,7 @@ 
 #define NETLINK_F_LISTEN_ALL_NSID	0x10
 #define NETLINK_F_CAP_ACK		0x20
 #define NETLINK_F_EXT_ACK		0x40
+#define NETLINK_F_STRICT_CHK		0x80
 
 #define NLGRPSZ(x)	(ALIGN(x, sizeof(unsigned long) * 8) / 8)
 #define NLGRPLONGS(x)	(NLGRPSZ(x)/sizeof(unsigned long))