Re: [Cake] [PATCH net 2/2] sch_cake: Make sure we can write the IP header before changing DSCP bits
Stephen Hemminger writes: > On Thu, 04 Apr 2019 22:44:33 +0200 > Toke Høiland-Jørgensen wrote: > >> Stephen Hemminger writes: >> >> > On Thu, 04 Apr 2019 15:01:33 +0200 >> > Toke Høiland-Jørgensen 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 ___ Cake mailing list Cake@lists.bufferbloat.net https://lists.bufferbloat.net/listinfo/cake
Re: [Cake] [PATCH net 2/2] sch_cake: Make sure we can write the IP header before changing DSCP bits
On Thu, 04 Apr 2019 22:44:33 +0200 Toke Høiland-Jørgensen wrote: > Stephen Hemminger writes: > > > On Thu, 04 Apr 2019 15:01:33 +0200 > > Toke Høiland-Jørgensen 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 ___ Cake mailing list Cake@lists.bufferbloat.net https://lists.bufferbloat.net/listinfo/cake
Re: [Cake] [PATCH net 2/2] sch_cake: Make sure we can write the IP header before changing DSCP bits
Stephen Hemminger writes: > On Thu, 04 Apr 2019 15:01:33 +0200 > Toke Høiland-Jørgensen 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 ___ Cake mailing list Cake@lists.bufferbloat.net https://lists.bufferbloat.net/listinfo/cake
Re: [Cake] [PATCH net 2/2] sch_cake: Make sure we can write the IP header before changing DSCP bits
On Thu, 04 Apr 2019 15:01:33 +0200 Toke Høiland-Jørgensen 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. ___ Cake mailing list Cake@lists.bufferbloat.net https://lists.bufferbloat.net/listinfo/cake
[Cake] [PATCH net 2/2] sch_cake: Make sure we can write the IP header before changing DSCP bits
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 Signed-off-by: Toke Høiland-Jørgensen --- 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); ___ Cake mailing list Cake@lists.bufferbloat.net https://lists.bufferbloat.net/listinfo/cake