[net-next PATCH v2 1/1] net sched actions: skbedit add support for mod-ing skb pkt_type

[net-next PATCH v2 1/1] net sched actions: skbedit add support for mod-ing skb pkt_type

[Jamal Hadi Salim]

From: Jamal Hadi Salim <hadi@mojatatu.com>

Extremely useful for setting packet type to host so i dont
have to modify the dst mac address using pedit (which requires
that i know the mac address)

Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 include/net/tc_act/tc_skbedit.h        | 10 +++++-----
 include/uapi/linux/tc_act/tc_skbedit.h |  2 ++
 net/sched/act_skbedit.c                | 18 +++++++++++++++++-
 3 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/include/net/tc_act/tc_skbedit.h b/include/net/tc_act/tc_skbedit.h
index b496d5a..d01a5d4 100644
--- a/include/net/tc_act/tc_skbedit.h
+++ b/include/net/tc_act/tc_skbedit.h
@@ -24,11 +24,11 @@
 
 struct tcf_skbedit {
 	struct tcf_common	common;
-	u32			flags;
-	u32     		priority;
-	u32     		mark;
-	u16			queue_mapping;
-	/* XXX: 16-bit pad here? */
+	u32		flags;
+	u32		priority;
+	u32		mark;
+	u16		queue_mapping;
+	u16		ptype;
 };
 #define to_skbedit(a) \
 	container_of(a->priv, struct tcf_skbedit, common)
diff --git a/include/uapi/linux/tc_act/tc_skbedit.h b/include/uapi/linux/tc_act/tc_skbedit.h
index fecb5cc..a4d00c6 100644
--- a/include/uapi/linux/tc_act/tc_skbedit.h
+++ b/include/uapi/linux/tc_act/tc_skbedit.h
@@ -27,6 +27,7 @@
 #define SKBEDIT_F_PRIORITY		0x1
 #define SKBEDIT_F_QUEUE_MAPPING		0x2
 #define SKBEDIT_F_MARK			0x4
+#define SKBEDIT_F_PTYPE			0x8
 
 struct tc_skbedit {
 	tc_gen;
@@ -40,6 +41,7 @@ enum {
 	TCA_SKBEDIT_QUEUE_MAPPING,
 	TCA_SKBEDIT_MARK,
 	TCA_SKBEDIT_PAD,
+	TCA_SKBEDIT_PTYPE,
 	__TCA_SKBEDIT_MAX
 };
 #define TCA_SKBEDIT_MAX (__TCA_SKBEDIT_MAX - 1)
diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
index 8f235ff..8a7a34b 100644
--- a/net/sched/act_skbedit.c
+++ b/net/sched/act_skbedit.c
@@ -47,6 +47,8 @@ static int tcf_skbedit(struct sk_buff *skb, const struct tc_action *a,
 		skb_set_queue_mapping(skb, d->queue_mapping);
 	if (d->flags & SKBEDIT_F_MARK)
 		skb->mark = d->mark;
+	if (d->flags & SKBEDIT_F_PTYPE)
+		skb->pkt_type = d->ptype;
 
 	spin_unlock(&d->tcf_lock);
 	return d->tcf_action;
@@ -57,6 +59,7 @@ static const struct nla_policy skbedit_policy[TCA_SKBEDIT_MAX + 1] = {
 	[TCA_SKBEDIT_PRIORITY]		= { .len = sizeof(u32) },
 	[TCA_SKBEDIT_QUEUE_MAPPING]	= { .len = sizeof(u16) },
 	[TCA_SKBEDIT_MARK]		= { .len = sizeof(u32) },
+	[TCA_SKBEDIT_PTYPE]		= { .len = sizeof(u16) },
 };
 
 static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
@@ -68,7 +71,7 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
 	struct tc_skbedit *parm;
 	struct tcf_skbedit *d;
 	u32 flags = 0, *priority = NULL, *mark = NULL;
-	u16 *queue_mapping = NULL;
+	u16 *queue_mapping = NULL, *ptype = NULL;
 	int ret = 0, err, aexists = 0;
 
 	if (nla == NULL)
@@ -91,6 +94,13 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
 		queue_mapping = nla_data(tb[TCA_SKBEDIT_QUEUE_MAPPING]);
 	}
 
+	if (tb[TCA_SKBEDIT_PTYPE] != NULL) {
+		flags |= SKBEDIT_F_PTYPE;
+		ptype = nla_data(tb[TCA_SKBEDIT_PTYPE]);
+		if (*ptype > PACKET_KERNEL)
+			return -EINVAL;
+	}
+
 	if (tb[TCA_SKBEDIT_MARK] != NULL) {
 		flags |= SKBEDIT_F_MARK;
 		mark = nla_data(tb[TCA_SKBEDIT_MARK]);
@@ -135,6 +145,8 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
 		d->queue_mapping = *queue_mapping;
 	if (flags & SKBEDIT_F_MARK)
 		d->mark = *mark;
+	if (flags & SKBEDIT_F_PTYPE)
+		d->ptype = *ptype;
 
 	d->tcf_action = parm->action;
 
@@ -172,6 +184,10 @@ static int tcf_skbedit_dump(struct sk_buff *skb, struct tc_action *a,
 	    nla_put(skb, TCA_SKBEDIT_MARK, sizeof(d->mark),
 		    &d->mark))
 		goto nla_put_failure;
+	if ((d->flags & SKBEDIT_F_PTYPE) &&
+	    nla_put(skb, TCA_SKBEDIT_PTYPE, sizeof(d->ptype),
+		    &d->ptype))
+		goto nla_put_failure;
 
 	tcf_tm_dump(&t, &d->tcf_tm);
 	if (nla_put_64bit(skb, TCA_SKBEDIT_TM, sizeof(t), &t, TCA_SKBEDIT_PAD))
-- 
1.9.1

Re: [net-next PATCH v2 1/1] net sched actions: skbedit add support for mod-ing skb pkt_type

[Daniel Borkmann]

Hi Jamal,

On 06/12/2016 11:24 PM, Jamal Hadi Salim wrote:
> From: Jamal Hadi Salim <hadi@mojatatu.com>
>
> Extremely useful for setting packet type to host so i dont
> have to modify the dst mac address using pedit (which requires
> that i know the mac address)
>
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>

I'm wondering if this is a good idea, I was thinking about something
like this as well some time ago. So far pkt_type is just exposed as
read-only to user space right now and I'm a bit worried that when we
allow to set it arbitrarily, then this could lead to hard to debug
issues since skb->pkt_type is used in a lot of places with possibly
different assumptions and applications now need to mistrust the kernel
whether skb->pkt_type was actually what the kernel itself set in the
first place or skbedit with possibly some nonsense value (like rewriting
PACKET_OUTGOING into PACKET_LOOPBACK, etc). Did you audit that this
is safe?

Thanks,
Daniel

Re: [net-next PATCH v2 1/1] net sched actions: skbedit add support for mod-ing skb pkt_type

[Jamal Hadi Salim]

On 16-06-13 04:00 AM, Daniel Borkmann wrote:
> Hi Jamal,
>
> On 06/12/2016 11:24 PM, Jamal Hadi Salim wrote:
>> From: Jamal Hadi Salim <hadi@mojatatu.com>
>>
>> Extremely useful for setting packet type to host so i dont
>> have to modify the dst mac address using pedit (which requires
>> that i know the mac address)
>>
>> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
>
> I'm wondering if this is a good idea, I was thinking about something
> like this as well some time ago.

Good ;-> What was your uses case?
In my case it is mostly to allow packets coming into the system
with the wrong MAC address (but correct IP address).
I dont want to keep checking what the MAC address is and adding
a rule to change it before it goes up the stack.

> So far pkt_type is just exposed as
> read-only to user space right now and I'm a bit worried that when we
> allow to set it arbitrarily, then this could lead to hard to debug
> issues since skb->pkt_type is used in a lot of places with possibly
> different assumptions and applications now need to mistrust the kernel
> whether skb->pkt_type was actually what the kernel itself set in the
> first place or skbedit with possibly some nonsense value (like rewriting
> PACKET_OUTGOING into PACKET_LOOPBACK, etc).

Separation of church from state.
In Unix we allow people to shoot their big toe if they want to.
If someone wants to change the destination MAC address to something
stupid they can. If someone wants to set the skbmark they can.
They'll find out it is a bad idea pretty quickly if it was not
what they intended. This is no different.

>Did you audit that this is safe?

Do you see a security issue? then that would need an audit.

cheers,
jamal

Re: [net-next PATCH v2 1/1] net sched actions: skbedit add support for mod-ing skb pkt_type

[Daniel Borkmann]

On 06/13/2016 01:52 PM, Jamal Hadi Salim wrote:
> On 16-06-13 04:00 AM, Daniel Borkmann wrote:
>> Hi Jamal,
>>
>> On 06/12/2016 11:24 PM, Jamal Hadi Salim wrote:
>>> From: Jamal Hadi Salim <hadi@mojatatu.com>
>>>
>>> Extremely useful for setting packet type to host so i dont
>>> have to modify the dst mac address using pedit (which requires
>>> that i know the mac address)
>>>
>>> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
>>
>> I'm wondering if this is a good idea, I was thinking about something
>> like this as well some time ago.
>
> Good ;-> What was your uses case?
> In my case it is mostly to allow packets coming into the system
> with the wrong MAC address (but correct IP address).
> I dont want to keep checking what the MAC address is and adding
> a rule to change it before it goes up the stack.

Yeah, had the same use-case here. ;)

>> So far pkt_type is just exposed as
>> read-only to user space right now and I'm a bit worried that when we
>> allow to set it arbitrarily, then this could lead to hard to debug
>> issues since skb->pkt_type is used in a lot of places with possibly
>> different assumptions and applications now need to mistrust the kernel
>> whether skb->pkt_type was actually what the kernel itself set in the
>> first place or skbedit with possibly some nonsense value (like rewriting
>> PACKET_OUTGOING into PACKET_LOOPBACK, etc).
>
> Separation of church from state.
> In Unix we allow people to shoot their big toe if they want to.
> If someone wants to change the destination MAC address to something
> stupid they can. If someone wants to set the skbmark they can.
> They'll find out it is a bad idea pretty quickly if it was not
> what they intended. This is no different.
>
>> Did you audit that this is safe?
>
> Do you see a security issue? then that would need an audit.

Looks like nft_meta_set_eval() can already do that, but restricted
via pkt_type_ok(), so we can't play bad games with PACKET_LOOPBACK
et al. Wasn't aware of that. Hm, so if you want to make use of this
from tc as well, probably it makes sense to add a generic helper as
in skb_pkt_type_mangle() and place it into skbuff.h?

Thanks,
Daniel

Re: [net-next PATCH v2 1/1] net sched actions: skbedit add support for mod-ing skb pkt_type

[Jamal Hadi Salim]

On 16-06-13 08:21 AM, Daniel Borkmann wrote:

>
> Looks like nft_meta_set_eval() can already do that, but restricted
> via pkt_type_ok(), so we can't play bad games with PACKET_LOOPBACK
> et al. Wasn't aware of that. Hm, so if you want to make use of this
> from tc as well, probably it makes sense to add a generic helper as
> in skb_pkt_type_mangle() and place it into skbuff.h?
>

Ok, will do with next update.

cheers,
jamal