[PATCH net-next 1/2] net: diag: slightly refactor the inet_diag_bc_audit error checks.

[PATCH net-next 1/2] net: diag: slightly refactor the inet_diag_bc_audit error checks.

[Lorenzo Colitti]

This simplifies the code a bit and also allows inet_diag_bc_audit
to send to userspace an error that isn't EINVAL.

Signed-off-by: Lorenzo Colitti <lorenzo@google.com>
---
 net/ipv4/inet_diag.c | 35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c
index 38c2c47..a835f94 100644
--- a/net/ipv4/inet_diag.c
+++ b/net/ipv4/inet_diag.c
@@ -706,10 +706,23 @@ static bool valid_port_comparison(const struct inet_diag_bc_op *op,
 	return true;
 }
 
-static int inet_diag_bc_audit(const void *bytecode, int bytecode_len)
+static bool valid_markcond(const struct inet_diag_bc_op *op, int len,
+			   int *min_len)
 {
-	const void *bc = bytecode;
-	int  len = bytecode_len;
+	*min_len += sizeof(struct inet_diag_markcond);
+	return len >= *min_len;
+}
+
+static int inet_diag_bc_audit(struct nlattr *attr)
+{
+	const void *bytecode, *bc;
+	int bytecode_len, len;
+
+	if (!attr || nla_len(attr) < sizeof(struct inet_diag_bc_op))
+		return -EINVAL;
+
+	bytecode = bc = nla_data(attr);
+	len = bytecode_len = nla_len(attr);
 
 	while (len > 0) {
 		int min_len = sizeof(struct inet_diag_bc_op);
@@ -1020,13 +1033,13 @@ static int inet_diag_rcv_msg_compat(struct sk_buff *skb, struct nlmsghdr *nlh)
 	if (nlh->nlmsg_flags & NLM_F_DUMP) {
 		if (nlmsg_attrlen(nlh, hdrlen)) {
 			struct nlattr *attr;
+			int err;
 
 			attr = nlmsg_find_attr(nlh, hdrlen,
 					       INET_DIAG_REQ_BYTECODE);
-			if (!attr ||
-			    nla_len(attr) < sizeof(struct inet_diag_bc_op) ||
-			    inet_diag_bc_audit(nla_data(attr), nla_len(attr)))
-				return -EINVAL;
+			err = inet_diag_bc_audit(attr);
+			if (err)
+				return err;
 		}
 		{
 			struct netlink_dump_control c = {
@@ -1051,13 +1064,13 @@ static int inet_diag_handler_cmd(struct sk_buff *skb, struct nlmsghdr *h)
 	    h->nlmsg_flags & NLM_F_DUMP) {
 		if (nlmsg_attrlen(h, hdrlen)) {
 			struct nlattr *attr;
+			int err;
 
 			attr = nlmsg_find_attr(h, hdrlen,
 					       INET_DIAG_REQ_BYTECODE);
-			if (!attr ||
-			    nla_len(attr) < sizeof(struct inet_diag_bc_op) ||
-			    inet_diag_bc_audit(nla_data(attr), nla_len(attr)))
-				return -EINVAL;
+			err = inet_diag_bc_audit(attr);
+			if (err)
+				return err;
 		}
 		{
 			struct netlink_dump_control c = {
-- 
2.8.0.rc3.226.g39d4020

[PATCH net-next 2/2] net: diag: allow socket bytecode filters to match socket marks

[Lorenzo Colitti]

This allows a privileged process to filter by socket mark when
dumping sockets via INET_DIAG_BY_FAMILY. This is useful on
systems that use mark-based routing such as Android.

The ability to filter socket marks requires CAP_NET_ADMIN, which
is consistent with other privileged operations allowed by the
SOCK_DIAG interface such as the ability to destroy sockets and
the ability to inspect BPF filters attached to packet sockets.

Tested: https://android-review.googlesource.com/261350
Signed-off-by: Lorenzo Colitti <lorenzo@google.com>
---
 include/uapi/linux/inet_diag.h |  6 ++++++
 net/ipv4/inet_diag.c           | 28 +++++++++++++++++++++++++---
 2 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/include/uapi/linux/inet_diag.h b/include/uapi/linux/inet_diag.h
index abbd1dc..2939d65 100644
--- a/include/uapi/linux/inet_diag.h
+++ b/include/uapi/linux/inet_diag.h
@@ -73,6 +73,7 @@ enum {
 	INET_DIAG_BC_S_COND,
 	INET_DIAG_BC_D_COND,
 	INET_DIAG_BC_DEV_COND,   /* u32 ifindex */
+	INET_DIAG_BC_MARK_COND,
 };
 
 struct inet_diag_hostcond {
@@ -82,6 +83,11 @@ struct inet_diag_hostcond {
 	__be32	addr[0];
 };
 
+struct inet_diag_markcond {
+	u32 mark;
+	u32 mask;
+};
+
 /* Base info structure. It contains socket identity (addrs/ports/cookie)
  * and, alas, the information shown by netstat. */
 struct inet_diag_msg {
diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c
index a835f94..f2a61f1 100644
--- a/net/ipv4/inet_diag.c
+++ b/net/ipv4/inet_diag.c
@@ -45,6 +45,7 @@ struct inet_diag_entry {
 	u16 family;
 	u16 userlocks;
 	u32 ifindex;
+	u32 mark;
 };
 
 static DEFINE_MUTEX(inet_diag_table_mutex);
@@ -580,6 +581,14 @@ static int inet_diag_bc_run(const struct nlattr *_bc,
 				yes = 0;
 			break;
 		}
+		case INET_DIAG_BC_MARK_COND: {
+			struct inet_diag_markcond *cond;
+
+			cond = (struct inet_diag_markcond *)(op + 1);
+			if ((entry->mark & cond->mask) != cond->mark)
+				yes = 0;
+			break;
+		}
 		}
 
 		if (yes) {
@@ -624,6 +633,12 @@ int inet_diag_bc_sk(const struct nlattr *bc, struct sock *sk)
 	entry.dport = ntohs(inet->inet_dport);
 	entry.ifindex = sk->sk_bound_dev_if;
 	entry.userlocks = sk_fullsock(sk) ? sk->sk_userlocks : 0;
+	if (sk_fullsock(sk))
+		entry.mark = sk->sk_mark;
+	else if (sk->sk_state == TCP_NEW_SYN_RECV)
+		entry.mark = inet_rsk(inet_reqsk(sk))->ir_mark;
+	else
+		entry.mark = 0;
 
 	return inet_diag_bc_run(bc, &entry);
 }
@@ -713,10 +728,11 @@ static bool valid_markcond(const struct inet_diag_bc_op *op, int len,
 	return len >= *min_len;
 }
 
-static int inet_diag_bc_audit(struct nlattr *attr)
+static int inet_diag_bc_audit(struct nlattr *attr, const struct sk_buff *skb)
 {
 	const void *bytecode, *bc;
 	int bytecode_len, len;
+	bool net_admin = netlink_net_capable(skb, CAP_NET_ADMIN);
 
 	if (!attr || nla_len(attr) < sizeof(struct inet_diag_bc_op))
 		return -EINVAL;
@@ -745,6 +761,12 @@ static int inet_diag_bc_audit(struct nlattr *attr)
 			if (!valid_port_comparison(bc, len, &min_len))
 				return -EINVAL;
 			break;
+		case INET_DIAG_BC_MARK_COND:
+			if (!net_admin)
+				return -EPERM;
+			if (!valid_markcond(bc, len, &min_len))
+				return -EINVAL;
+			break;
 		case INET_DIAG_BC_AUTO:
 		case INET_DIAG_BC_JMP:
 		case INET_DIAG_BC_NOP:
@@ -1037,7 +1059,7 @@ static int inet_diag_rcv_msg_compat(struct sk_buff *skb, struct nlmsghdr *nlh)
 
 			attr = nlmsg_find_attr(nlh, hdrlen,
 					       INET_DIAG_REQ_BYTECODE);
-			err = inet_diag_bc_audit(attr);
+			err = inet_diag_bc_audit(attr, skb);
 			if (err)
 				return err;
 		}
@@ -1068,7 +1090,7 @@ static int inet_diag_handler_cmd(struct sk_buff *skb, struct nlmsghdr *h)
 
 			attr = nlmsg_find_attr(h, hdrlen,
 					       INET_DIAG_REQ_BYTECODE);
-			err = inet_diag_bc_audit(attr);
+			err = inet_diag_bc_audit(attr, skb);
 			if (err)
 				return err;
 		}
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH net-next 1/2] net: diag: slightly refactor the inet_diag_bc_audit error checks.

[Eric Dumazet]

On Wed, 2016-08-24 at 03:06 +0900, Lorenzo Colitti wrote:
> This simplifies the code a bit and also allows inet_diag_bc_audit
> to send to userspace an error that isn't EINVAL.
> 
> Signed-off-by: Lorenzo Colitti <lorenzo@google.com>
> ---
>  net/ipv4/inet_diag.c | 35 ++++++++++++++++++++++++-----------
>  1 file changed, 24 insertions(+), 11 deletions(-)
> 
> diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c
> index 38c2c47..a835f94 100644
> --- a/net/ipv4/inet_diag.c
> +++ b/net/ipv4/inet_diag.c
> @@ -706,10 +706,23 @@ static bool valid_port_comparison(const struct inet_diag_bc_op *op,
>  	return true;
>  }
>  
> -static int inet_diag_bc_audit(const void *bytecode, int bytecode_len)
> +static bool valid_markcond(const struct inet_diag_bc_op *op, int len,
> +			   int *min_len)
>  {
> -	const void *bc = bytecode;
> -	int  len = bytecode_len;
> +	*min_len += sizeof(struct inet_diag_markcond);
> +	return len >= *min_len;
> +}

I do not see how this can compile, since you introduce "struct
inet_diag_markcond" in the following patch ?

Re: [PATCH net-next 1/2] net: diag: slightly refactor the inet_diag_bc_audit error checks.

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1580 bytes --]

Hi Lorenzo,

[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Lorenzo-Colitti/net-diag-slightly-refactor-the-inet_diag_bc_audit-error-checks/20160824-020848
config: x86_64-randconfig-x015-201634 (attached as .config)
compiler: gcc-6 (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

Note: the linux-review/Lorenzo-Colitti/net-diag-slightly-refactor-the-inet_diag_bc_audit-error-checks/20160824-020848 HEAD a2280f98a90602586a70251ad6f794ebe70f687a builds fine.
      It only hurts bisectibility.

All errors (new ones prefixed by >>):

   net/ipv4/inet_diag.c: In function 'valid_markcond':
>> net/ipv4/inet_diag.c:712:21: error: invalid application of 'sizeof' to incomplete type 'struct inet_diag_markcond'
     *min_len += sizeof(struct inet_diag_markcond);
                        ^~~~~~
   At top level:
   net/ipv4/inet_diag.c:709:13: warning: 'valid_markcond' defined but not used [-Wunused-function]
    static bool valid_markcond(const struct inet_diag_bc_op *op, int len,
                ^~~~~~~~~~~~~~

vim +712 net/ipv4/inet_diag.c

   706		return true;
   707	}
   708	
   709	static bool valid_markcond(const struct inet_diag_bc_op *op, int len,
   710				   int *min_len)
   711	{
 > 712		*min_len += sizeof(struct inet_diag_markcond);
   713		return len >= *min_len;
   714	}
   715	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 31132 bytes --]

Re: [PATCH net-next 2/2] net: diag: allow socket bytecode filters to match socket marks

[David Ahern]

On 8/23/16 12:06 PM, Lorenzo Colitti wrote:
> @@ -713,10 +728,11 @@ static bool valid_markcond(const struct inet_diag_bc_op *op, int len,
>  	return len >= *min_len;
>  }
>  
> -static int inet_diag_bc_audit(struct nlattr *attr)
> +static int inet_diag_bc_audit(struct nlattr *attr, const struct sk_buff *skb)
>  {
>  	const void *bytecode, *bc;
>  	int bytecode_len, len;
> +	bool net_admin = netlink_net_capable(skb, CAP_NET_ADMIN);
>  
>  	if (!attr || nla_len(attr) < sizeof(struct inet_diag_bc_op))
>  		return -EINVAL;

The net_admin arg should be added as the first local to maintain preference for reverse xmas tree order.