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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230619160254.33579-1-kuniyu@amazon.com>
Date: Mon, 19 Jun 2023 09:02:54 -0700
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <kuba@...nel.org>
CC: <davem@...emloft.net>, <dsahern@...nel.org>, <edumazet@...gle.com>,
	<kuni1840@...il.com>, <kuniyu@...zon.com>, <netdev@...r.kernel.org>,
	<pabeni@...hat.com>
Subject: Re: [PATCH v1 net-next 1/5] ipv6: rpl: Remove pskb(_may)?_pull() in ipv6_rpl_srh_rcv().

From: Jakub Kicinski <kuba@...nel.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!