Patchwork [net-next,1/2] net_sched: don't do precise pkt_len computation for untrusted packets

login
register
mail settings
Submitter Jason Wang
Date March 15, 2013, 7:41 a.m.
Message ID <1363333305-54398-1-git-send-email-jasowang@redhat.com>
Download mbox | patch
Permalink /patch/227864/
State Rejected
Delegated to: David Miller
Headers show

Comments

Jason Wang - March 15, 2013, 7:41 a.m.
Commit 1def9238d4aa2 (net_sched: more precise pkt_len computation) tries to do
precise packet len computation for GSO packets, but it does not check whether
the packets were from untrusted source. This is wrong since: we haven't done
header check before so both gso_segs and headers may not be correct. So this
patch just bypass the precise pkt_len computation for packet from untrusted
source (SKB_GSO_DODGY).

Cc: Eric Dumazet <edumazet@google.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 net/core/dev.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
David Miller - March 17, 2013, 4:10 p.m.
From: Jason Wang <jasowang@redhat.com>
Date: Fri, 15 Mar 2013 15:41:44 +0800

> Commit 1def9238d4aa2 (net_sched: more precise pkt_len computation) tries to do
> precise packet len computation for GSO packets, but it does not check whether
> the packets were from untrusted source. This is wrong since: we haven't done
> header check before so both gso_segs and headers may not be correct. So this
> patch just bypass the precise pkt_len computation for packet from untrusted
> source (SKB_GSO_DODGY).
> 
> Cc: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>

I do not think this is appropriate or even necessary.

All the user can do by reporting an incorrect header size or GSO segs
is hurt himself, by making his traffic take more packet scheduler
quota.

When we do precise accounting, it increases, never decreases, the
amount that a packet "costs" as far as the packet scheduler is
concerned.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jason Wang - March 19, 2013, 9:25 a.m.
On 03/18/2013 12:10 AM, David Miller wrote:
> From: Jason Wang <jasowang@redhat.com>
> Date: Fri, 15 Mar 2013 15:41:44 +0800
>
>> Commit 1def9238d4aa2 (net_sched: more precise pkt_len computation) tries to do
>> precise packet len computation for GSO packets, but it does not check whether
>> the packets were from untrusted source. This is wrong since: we haven't done
>> header check before so both gso_segs and headers may not be correct. So this
>> patch just bypass the precise pkt_len computation for packet from untrusted
>> source (SKB_GSO_DODGY).
>>
>> Cc: Eric Dumazet <edumazet@google.com>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> I do not think this is appropriate or even necessary.
>
> All the user can do by reporting an incorrect header size or GSO segs
> is hurt himself, by making his traffic take more packet scheduler
> quota.

I believe before doing header check for untrusted packets, the only
thing we can trust is skb->len and that's we've used before
1def9238d4aa2. But after that, we're trying to use unchecked or
meaningless value (e.g gso_segs were reset to zero in
tun/macvtap/packet), and guest then can utilize this to result a very
huge (-1U) pkt_len by filling evil value in the header. Can all kinds of
packet scheduler survive this kinds of possible DOS?
>
> When we do precise accounting, it increases, never decreases, the
> amount that a packet "costs" as far as the packet scheduler is
> concerned.
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet - March 19, 2013, 12:10 p.m.
On Tue, 2013-03-19 at 17:25 +0800, Jason Wang wrote:

> I believe before doing header check for untrusted packets, the only
> thing we can trust is skb->len and that's we've used before
> 1def9238d4aa2. But after that, we're trying to use unchecked or
> meaningless value (e.g gso_segs were reset to zero in
> tun/macvtap/packet), and guest then can utilize this to result a very
> huge (-1U) pkt_len by filling evil value in the header. Can all kinds of
> packet scheduler survive this kinds of possible DOS?

I would use the flow dissector to fix the transport header from all
DODGY providers.

Daniel Borkmann is working on a patch serie adding nhoff into flow_keys,
and adding __skb_get_poff(const struct sk_buff *skb), for a BPF
extension we talked about in Copenhagen / Netfilter Workshop.

You could then set the transport header offset to the right value.

(and drop evil packets before they go further in the stack)

if (gso_packet(skb)) {
	u32 poff = __skb_get_poff(skb);

	if (!poff) {
		drop_evil_packet(skb);
	} else {
		skb_set_transport_header(skb, poff);
		...
	}
}



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet - March 19, 2013, 12:58 p.m.
On Tue, 2013-03-19 at 05:10 -0700, Eric Dumazet wrote:
> On Tue, 2013-03-19 at 17:25 +0800, Jason Wang wrote:
> 
> > I believe before doing header check for untrusted packets, the only
> > thing we can trust is skb->len and that's we've used before
> > 1def9238d4aa2. But after that, we're trying to use unchecked or
> > meaningless value (e.g gso_segs were reset to zero in
> > tun/macvtap/packet), and guest then can utilize this to result a very
> > huge (-1U) pkt_len by filling evil value in the header. Can all kinds of
> > packet scheduler survive this kinds of possible DOS?
> 
> I would use the flow dissector to fix the transport header from all
> DODGY providers.
> 
> Daniel Borkmann is working on a patch serie adding nhoff into flow_keys,
> and adding __skb_get_poff(const struct sk_buff *skb), for a BPF
> extension we talked about in Copenhagen / Netfilter Workshop.
> 
> You could then set the transport header offset to the right value.
> 
> (and drop evil packets before they go further in the stack)
> 
> if (gso_packet(skb)) {
> 	u32 poff = __skb_get_poff(skb);
> 
> 	if (!poff) {
> 		drop_evil_packet(skb);
> 	} else {
> 		skb_set_transport_header(skb, poff);
> 		...
> 	}
> }


Oh well, no need to use __skb_get_poff() but plain skb_flow_dissect()
(once patched to include thoff in struct flow_keys)

struct flow_keys keys;

if (!skb_flow_dissect(skb, &keys))
	goto drop;

if ((gso_type & (SKB_GSO_TCPV4|SKB_GSO_TCPV6)) &&
    keys.ip_proto != IP_PROTO_TCP)
	goto drop;

skb_set_transport_header(skb, keys.thoff);



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jason Wang - March 20, 2013, 6:19 a.m.
On 03/19/2013 08:58 PM, Eric Dumazet wrote:
> On Tue, 2013-03-19 at 05:10 -0700, Eric Dumazet wrote:
>> On Tue, 2013-03-19 at 17:25 +0800, Jason Wang wrote:
>>
>>> I believe before doing header check for untrusted packets, the only
>>> thing we can trust is skb->len and that's we've used before
>>> 1def9238d4aa2. But after that, we're trying to use unchecked or
>>> meaningless value (e.g gso_segs were reset to zero in
>>> tun/macvtap/packet), and guest then can utilize this to result a very
>>> huge (-1U) pkt_len by filling evil value in the header. Can all kinds of
>>> packet scheduler survive this kinds of possible DOS?
>> I would use the flow dissector to fix the transport header from all
>> DODGY providers.
>>
>> Daniel Borkmann is working on a patch serie adding nhoff into flow_keys,
>> and adding __skb_get_poff(const struct sk_buff *skb), for a BPF
>> extension we talked about in Copenhagen / Netfilter Workshop.
>>
>> You could then set the transport header offset to the right value.
>>
>> (and drop evil packets before they go further in the stack)
>>
>> if (gso_packet(skb)) {
>> 	u32 poff = __skb_get_poff(skb);
>>
>> 	if (!poff) {
>> 		drop_evil_packet(skb);
>> 	} else {
>> 		skb_set_transport_header(skb, poff);
>> 		...
>> 	}
>> }
>
> Oh well, no need to use __skb_get_poff() but plain skb_flow_dissect()
> (once patched to include thoff in struct flow_keys)
>
> struct flow_keys keys;
>
> if (!skb_flow_dissect(skb, &keys))
> 	goto drop;
>
> if ((gso_type & (SKB_GSO_TCPV4|SKB_GSO_TCPV6)) &&
>     keys.ip_proto != IP_PROTO_TCP)
> 	goto drop;
>
> skb_set_transport_header(skb, keys.thoff);

I was consider a just skb_reset_transport_header() here. Consider the
transport header maybe checked and reset during header check for packets
of gso or partial checksum. And bypass precise pkt len computation.

Some problems with skb_flow_dissect():

- it can only recognizes a subset of all ethernet protocols. The may
blocks guest who may want to use other protocol such as IPX.
- almost no check in the validity of the L4 protocol header which may be
used by qdisc_pkt_len_init(), which may still give a chance to evil
guest to use
- gso_segs were untouched (still zero)

Another method is doing header check here which needs more work.
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet - March 20, 2013, 1:46 p.m.
On Wed, 2013-03-20 at 14:19 +0800, Jason Wang wrote:

> I was consider a just skb_reset_transport_header() here. Consider the
> transport header maybe checked and reset during header check for packets
> of gso or partial checksum. And bypass precise pkt len computation.
> 
> Some problems with skb_flow_dissect():
> 
> - it can only recognizes a subset of all ethernet protocols. The may
> blocks guest who may want to use other protocol such as IPX.

Oh yes IPX 

> - almost no check in the validity of the L4 protocol header which may be
> used by qdisc_pkt_len_init(), which may still give a chance to evil
> guest to use
> - gso_segs were untouched (still zero)
> 
> Another method is doing header check here which needs more work.

Most uses are caught by the helper.

So in the case dissection fails, just reset transport header instead of
dropping as I suggested.

Otherwise, set the transport header to the right value

-> precise qdisc pkt lengths.

Instead of pretending DODGY packets have no headers, at least try to do
the right thing.



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/net/core/dev.c b/net/core/dev.c
index 90cee5b..480114d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2586,7 +2586,7 @@  static void qdisc_pkt_len_init(struct sk_buff *skb)
 	/* To get more precise estimation of bytes sent on wire,
 	 * we add to pkt_len the headers size of all segments
 	 */
-	if (shinfo->gso_size)  {
+	if (shinfo->gso_size && !(shinfo->gso_type & SKB_GSO_DODGY))  {
 		unsigned int hdr_len;
 
 		/* mac layer + network layer */