From mboxrd@z Thu Jan  1 00:00:00 1970
From: Toke =?utf-8?Q?H=C3=B8iland-J=C3=B8rgensen?= <toke@toke.dk>
Subject: Re: [PATCH net-next v4] Add Common Applications Kept Enhanced (cake) qdisc
Date: Fri, 27 Apr 2018 15:38:20 +0200
Message-ID: <87in8c4vn7.fsf@toke.dk>
References: <20180427121706.23273-1-toke@toke.dk> <efb401ac-bc79-cbc5-cd03-120803b65b4d@gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Cc: cake@lists.bufferbloat.net, Dave Taht <dave.taht@gmail.com>
To: Eric Dumazet <eric.dumazet@gmail.com>, netdev@vger.kernel.org
Return-path: <netdev-owner@vger.kernel.org>
Received: from mail.toke.dk ([52.28.52.200]:45619 "EHLO mail.toke.dk"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1758357AbeD0NiW (ORCPT <rfc822;netdev@vger.kernel.org>);
        Fri, 27 Apr 2018 09:38:22 -0400
In-Reply-To: <efb401ac-bc79-cbc5-cd03-120803b65b4d@gmail.com>
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

Eric Dumazet <eric.dumazet@gmail.com> writes:

> On 04/27/2018 05:17 AM, Toke H=C3=B8iland-J=C3=B8rgensen wrote:
>
> ...
>
>> +
>> +static struct sk_buff *cake_ack_filter(struct cake_sched_data *q,
>> +				       struct cake_flow *flow)
>> +{
>> +	int seglen;
>> +	struct sk_buff *skb =3D flow->tail, *skb_check, *skb_check_prev;
>> +	struct iphdr *iph, *iph_check;
>> +	struct ipv6hdr *ipv6h, *ipv6h_check;
>> +	struct tcphdr *tcph, *tcph_check;
>> +	bool otherconn_ack_seen =3D false;
>> +	struct sk_buff *otherconn_checked_to =3D NULL;
>> +	bool thisconn_redundant_seen =3D false, thisconn_seen_last =3D false;
>> +	struct sk_buff *thisconn_checked_to =3D NULL, *thisconn_ack =3D NULL;
>> +	bool aggressive =3D q->ack_filter =3D=3D CAKE_ACK_AGGRESSIVE;
>> +
>> +	/* no other possible ACKs to filter */
>> +	if (flow->head =3D=3D skb)
>> +		return NULL;
>> +
>> +	iph =3D skb->encapsulation ? inner_ip_hdr(skb) : ip_hdr(skb);
>> +	ipv6h =3D skb->encapsulation ? inner_ipv6_hdr(skb) : ipv6_hdr(skb);
>> +
>> +	/* check that the innermost network header is v4/v6, and contains TCP =
*/
>> +	if (pskb_may_pull(skb, ((unsigned char *)iph - skb->head) + sizeof(str=
uct iphdr)) &&
>> +	    iph->version =3D=3D 4) {
>> +		if (iph->protocol !=3D IPPROTO_TCP)
>> +			return NULL;
>> +		seglen =3D ntohs(iph->tot_len) - (4 * iph->ihl);
>> +		tcph =3D (struct tcphdr *)((void *)iph + (4 * iph->ihl));
>> +		if (!pskb_may_pull(skb, ((unsigned char *)tcph - skb->head) + sizeof(=
struct tcphdr)))
>> +			return NULL;
>> +	} else if (pskb_may_pull(skb, ((unsigned char *)ipv6h - skb->head) + s=
izeof(struct ipv6hdr) + sizeof(struct tcphdr)) &&
>> +	           ipv6h->version =3D=3D 6) {
>> +		if (ipv6h->nexthdr !=3D IPPROTO_TCP)
>> +			return NULL;
>> +		seglen =3D ntohs(ipv6h->payload_len);
>> +		tcph =3D (struct tcphdr *)((void *)ipv6h +
>> +					 sizeof(struct ipv6hdr));
>> +	} else {
>> +		return NULL;
>> +	}
>> +
>
>
> This is still broken.
>
> After pskb_may_pull(), skb->head might have been reallocated.
>
> You need to recompute iph , ipv6h, tcph, otherwise you are reading
> freed memory and crash kernels with sufficient debugging (KASAN and
> other CONFIG_DEBUG_PAGEALLOC / CONFIG_DEBUG_SLAB like options)

Ah, right. Will fix.

Is it safe to dereference the iph pointer before calling
pskb_may_pull()?

-Toke