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

[PATCH net-next v2 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 | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c
index 38c2c47..a89b68c 100644
--- a/net/ipv4/inet_diag.c
+++ b/net/ipv4/inet_diag.c
@@ -706,10 +706,16 @@ 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 int inet_diag_bc_audit(const struct nlattr *attr)
 {
-	const void *bc = bytecode;
-	int  len = bytecode_len;
+	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 +1026,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 +1057,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 v2 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           | 36 +++++++++++++++++++++++++++++++++---
 2 files changed, 39 insertions(+), 3 deletions(-)

diff --git a/include/uapi/linux/inet_diag.h b/include/uapi/linux/inet_diag.h
index abbd1dc..5581206 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 a89b68c..abfbe49 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);
 }
@@ -706,8 +721,17 @@ static bool valid_port_comparison(const struct inet_diag_bc_op *op,
 	return true;
 }
 
-static int inet_diag_bc_audit(const struct nlattr *attr)
+static bool valid_markcond(const struct inet_diag_bc_op *op, int len,
+			   int *min_len)
 {
+	*min_len += sizeof(struct inet_diag_markcond);
+	return len >= *min_len;
+}
+
+static int inet_diag_bc_audit(const struct nlattr *attr,
+			      const struct sk_buff *skb)
+{
+	bool net_admin = netlink_net_capable(skb, CAP_NET_ADMIN);
 	const void *bytecode, *bc;
 	int bytecode_len, len;
 
@@ -738,6 +762,12 @@ static int inet_diag_bc_audit(const 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:
@@ -1030,7 +1060,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;
 		}
@@ -1061,7 +1091,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 v2 1/2] net: diag: slightly refactor the inet_diag_bc_audit error checks.

[David Ahern]

On 8/24/16 12:46 AM, 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 | 28 +++++++++++++++++-----------
>  1 file changed, 17 insertions(+), 11 deletions(-)
> 

Acked-by: David Ahern <dsa@cumulusnetworks.com>

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

[David Ahern]

On 8/24/16 12:46 AM, Lorenzo Colitti wrote:
> 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.

sock_diag_destroy already requires ADMIN for destroying sockets, so adding here just prevents listing them. Given that and the fact that the diag API does not pass the mark value is there a reason to add the extra ADMIN check?
 

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

[Lorenzo Colitti]

On Wed, Aug 24, 2016 at 11:35 PM, David Ahern <dsa@cumulusnetworks.com> wrote:
> sock_diag_destroy already requires ADMIN for destroying sockets, so adding here just prevents listing them. Given that and the fact that the diag API does not pass the mark value is there a reason to add the extra ADMIN check?

Even though the API does not return the mark value, this code allows
the caller to find the mark of any socket simply by performing n=32
socket dumps, each filtering for a mark/mask of 1<<n/1<<n. Each such
dump will reveal bit n in the mark of all sockets in the system.

If you consider socket marks to be information that's private to the
app, then it's appropriate to restrict access to them to NET_ADMIN. I
thought it prudent to do so, particularly since only applications with
NET_ADMIN can set marks in the first place.

Note that a process with NET_ADMIN can already determine the mark of
any given socket (as long as that socket sends a packet) by installing
32 iptables rules that exactly match the socket's 5-tuple and where
each rule checks one bit in the mark. So this patch doesn't expose any
information that was not previously available.

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

[David Ahern]

On 8/24/16 12:46 AM, Lorenzo Colitti wrote:
> 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           | 36 +++++++++++++++++++++++++++++++++---
>  2 files changed, 39 insertions(+), 3 deletions(-)

Acked-by: David Ahern <dsa@cumulusnetworks.com>

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

[David Miller]

From: Lorenzo Colitti <lorenzo@google.com>
Date: Wed, 24 Aug 2016 15:46:25 +0900

> 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>

Applied.

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

[David Miller]

From: Lorenzo Colitti <lorenzo@google.com>
Date: Wed, 24 Aug 2016 15:46:26 +0900

> 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>

Applied.