Re: [PATCH v1 net-next 1/5] ipv6: rpl: Remove pskb(_may)?_pull() in ipv6_rpl_srh_rcv().

[Kuniyuki Iwashima <kuniyu@amazon.com>]

From: Jakub Kicinski <kuba@kernel.org>
Date: Sat, 17 Jun 2023 01:00:14 -0700
> On Wed, 14 Jun 2023 16:01:03 -0700 Kuniyuki Iwashima wrote:
> > -	if (!pskb_may_pull(skb, ipv6_rpl_srh_size(n, hdr->cmpri,
> > -						  hdr->cmpre))) {
> 
> Are we checking that 
> 
> 	ipv6_rpl_srh_size(n, hdr->cmpri, hdr->cmpre) < (hdrlen + 1) << 3
> 
> somewhere?

sizeof(struct ipv6_rpl_sr_hdr) is (1 << 3), and n is calculated from
hdrlen, cmpri, cmpre, and pad.

Here n could underflow, but the irregular case is caught by the two
conditionals below, where segments_left >= 1.

  1) n <  U64_MAX -> n + 1 > 255
  2) n == U64_MAX -> n + 1 == 0 < segments_left

So, the formula below holds.

  n1 = (hdr->hdrlen << 3) - hdr->pad - (16 - hdr->cmpre)
  n2 = n1 / (16 - hdr->cmpri)

  n2 * (16 - hdr->cmpri) + hdr->pad + (16 - hdr->cmpre) = hdr->hdrlen << 3
  (Here pad could be equal or greater than 0)

  n2 * (16 - hdr->cmpri) + (16 - hdr->cmpre) <= hdr->hdrlen << 3

  ipv6_rpl_srh_size(n, hdr->cmpri, hdr->cmpre) <= (hdrlen + 1) << 3

---8<---
	n = (hdr->hdrlen << 3) - hdr->pad - (16 - hdr->cmpre);
	r = do_div(n, (16 - hdr->cmpri));
	/* checks if calculation was without remainder and n fits into
	 * unsigned char which is segments_left field. Should not be
	 * higher than that.
	 */
	if (r || (n + 1) > 255) {
		kfree_skb(skb);
		return -1;
	}

	if (hdr->segments_left > n + 1) {
...
		return -1;
	}

	hdr->segments_left--;
	i = n - hdr->segments_left;
---8<---


> 
> also nit:
> 
> > As Eric Dumazet pointed out [0], ipv6_rthdr_rcv() pulls these data
> > 
> >   - Segment Routing Header : 8
> >   - Hdr Ext Len            : skb_transport_header(skb)[1] << 3
> 
> +1 missing here, AFAICT

The +1 here is the 8 bytes header.  Hdr Ext Len does not include
the header length.

In ipv6_rthdr_rcv(), the first pskb_may_pull() expresses it as 8,
and 2nd one uses (1 << 3).  We cannot use sizeof() here because
each Ext Hdr has different struct, although all of them are 8 bytes.

---8<---
	if (!pskb_may_pull(skb, skb_transport_offset(skb) + 8) ||
	    !pskb_may_pull(skb, (skb_transport_offset(skb) +
				 ((skb_transport_header(skb)[1] + 1) << 3)))) {
		__IP6_INC_STATS(net, idev, IPSTATS_MIB_INHDRERRORS);
		kfree_skb(skb);
		return -1;
	}
---8<---

Thanks!