[PATCH net 0/2] sched: A few small fixes for sch_cake

[PATCH net 0/2] sched: A few small fixes for sch_cake

[Toke Høiland-Jørgensen]

Hi Dave

Kevin noticed a few issues with the way CAKE reads the skb protocol and the IP
diffserv fields. This series fixes those two issues, and should probably go to
in 4.19 as well. However, the previous refactoring patch means they don't apply
as-is; I can send a follow-up directly to stable if that's OK with you?

-Toke

---

Toke Høiland-Jørgensen (2):
      sch_cake: Use tc_skb_protocol() helper for getting packet protocol
      sch_cake: Make sure we can write the IP header before changing DSCP bits


 net/sched/sch_cake.c |   13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

[PATCH net 1/2] sch_cake: Use tc_skb_protocol() helper for getting packet protocol

[Toke Høiland-Jørgensen]

We shouldn't be using skb->protocol directly as that will miss cases with
hardware-accelerated VLAN tags. Use the helper instead to get the right
protocol number.

Reported-by: Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk>
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 net/sched/sch_cake.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
index acc9b9da985f..a3b55e18df04 100644
--- a/net/sched/sch_cake.c
+++ b/net/sched/sch_cake.c
@@ -1519,7 +1519,7 @@ static u8 cake_handle_diffserv(struct sk_buff *skb, u16 wash)
 {
 	u8 dscp;
 
-	switch (skb->protocol) {
+	switch (tc_skb_protocol(skb)) {
 	case htons(ETH_P_IP):
 		dscp = ipv4_get_dsfield(ip_hdr(skb)) >> 2;
 		if (wash && dscp)

[PATCH net 2/2] sch_cake: Make sure we can write the IP header before changing DSCP bits

[Toke Høiland-Jørgensen]

There is not actually any guarantee that the IP headers are valid before we
access the DSCP bits of the packets. Fix this using the same approach taken
in sch_dsmark.

Reported-by: Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk>
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 net/sched/sch_cake.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
index a3b55e18df04..259d97bc2abd 100644
--- a/net/sched/sch_cake.c
+++ b/net/sched/sch_cake.c
@@ -1517,16 +1517,27 @@ static unsigned int cake_drop(struct Qdisc *sch, struct sk_buff **to_free)
 
 static u8 cake_handle_diffserv(struct sk_buff *skb, u16 wash)
 {
+	int wlen = skb_network_offset(skb);
 	u8 dscp;
 
 	switch (tc_skb_protocol(skb)) {
 	case htons(ETH_P_IP):
+		wlen += sizeof(struct iphdr);
+		if (!pskb_may_pull(skb, wlen) ||
+		    skb_try_make_writable(skb, wlen))
+			return 0;
+
 		dscp = ipv4_get_dsfield(ip_hdr(skb)) >> 2;
 		if (wash && dscp)
 			ipv4_change_dsfield(ip_hdr(skb), INET_ECN_MASK, 0);
 		return dscp;
 
 	case htons(ETH_P_IPV6):
+		wlen += sizeof(struct ipv6hdr);
+		if (!pskb_may_pull(skb, wlen) ||
+		    skb_try_make_writable(skb, wlen))
+			return 0;
+
 		dscp = ipv6_get_dsfield(ipv6_hdr(skb)) >> 2;
 		if (wash && dscp)
 			ipv6_change_dsfield(ipv6_hdr(skb), INET_ECN_MASK, 0);

Re: [PATCH net 2/2] sch_cake: Make sure we can write the IP header before changing DSCP bits

[Stephen Hemminger]

On Thu, 04 Apr 2019 15:01:33 +0200
Toke Høiland-Jørgensen <toke@redhat.com> wrote:

>  static u8 cake_handle_diffserv(struct sk_buff *skb, u16 wash)
>  {
> +	int wlen = skb_network_offset(skb);

In theory this could be negative, you should handle that?
Rather than calling may_pull() with a huge unsigned value.

Re: [PATCH net 2/2] sch_cake: Make sure we can write the IP header before changing DSCP bits

[Toke Høiland-Jørgensen]

Stephen Hemminger <stephen@networkplumber.org> writes:

> On Thu, 04 Apr 2019 15:01:33 +0200
> Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
>>  static u8 cake_handle_diffserv(struct sk_buff *skb, u16 wash)
>>  {
>> +	int wlen = skb_network_offset(skb);
>
> In theory this could be negative, you should handle that?
> Rather than calling may_pull() with a huge unsigned value.

Huh, that would imply that skb->network_header points to before
skb->head; when does that happen?

Also, pskb_may_pull() does check for len > skb->len, so I guess a
follow-up question would be, "does it happen often enough to warrant
handling at this level"?

Also, I copied that bit from sch_dsmark, so if you really thing it needs
to be fixed, I guess we should fix both...

-Toke

Re: [PATCH net 2/2] sch_cake: Make sure we can write the IP header before changing DSCP bits

[Stephen Hemminger]

On Thu, 04 Apr 2019 22:44:33 +0200
Toke Høiland-Jørgensen <toke@redhat.com> wrote:

> Stephen Hemminger <stephen@networkplumber.org> writes:
> 
> > On Thu, 04 Apr 2019 15:01:33 +0200
> > Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >  
> >>  static u8 cake_handle_diffserv(struct sk_buff *skb, u16 wash)
> >>  {
> >> +	int wlen = skb_network_offset(skb);  
> >
> > In theory this could be negative, you should handle that?
> > Rather than calling may_pull() with a huge unsigned value.  
> 
> Huh, that would imply that skb->network_header points to before
> skb->head; when does that happen?
> 
> Also, pskb_may_pull() does check for len > skb->len, so I guess a
> follow-up question would be, "does it happen often enough to warrant
> handling at this level"?
> 
> Also, I copied that bit from sch_dsmark, so if you really thing it needs
> to be fixed, I guess we should fix both...
> 
> -Toke

It should never happen just paranoid

Re: [PATCH net 2/2] sch_cake: Make sure we can write the IP header before changing DSCP bits

[Toke Høiland-Jørgensen]

Stephen Hemminger <stephen@networkplumber.org> writes:

> On Thu, 04 Apr 2019 22:44:33 +0200
> Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
>> Stephen Hemminger <stephen@networkplumber.org> writes:
>> 
>> > On Thu, 04 Apr 2019 15:01:33 +0200
>> > Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>> >  
>> >>  static u8 cake_handle_diffserv(struct sk_buff *skb, u16 wash)
>> >>  {
>> >> +	int wlen = skb_network_offset(skb);  
>> >
>> > In theory this could be negative, you should handle that?
>> > Rather than calling may_pull() with a huge unsigned value.  
>> 
>> Huh, that would imply that skb->network_header points to before
>> skb->head; when does that happen?
>> 
>> Also, pskb_may_pull() does check for len > skb->len, so I guess a
>> follow-up question would be, "does it happen often enough to warrant
>> handling at this level"?
>> 
>> Also, I copied that bit from sch_dsmark, so if you really thing it needs
>> to be fixed, I guess we should fix both...
>> 
>> -Toke
>
> It should never happen just paranoid

Ah, right. Well in that case I think any overflow is handled fine inside
pskb_may_pull():

	if (unlikely(len > skb->len))
		return 0;


-Toke

Re: [PATCH net 0/2] sched: A few small fixes for sch_cake

[David Miller]

From: Toke Høiland-Jørgensen <toke@redhat.com>
Date: Thu, 04 Apr 2019 15:01:33 +0200

> Hi Dave
> 
> Kevin noticed a few issues with the way CAKE reads the skb protocol and the IP
> diffserv fields. This series fixes those two issues, and should probably go to
> in 4.19 as well. However, the previous refactoring patch means they don't apply
> as-is; I can send a follow-up directly to stable if that's OK with you?

Series applied, and queued up for -stable.

Indeed, posting a backport for v4.19 would be great.