netdev - RE: Fix skb_set_peeked use-after-free bug

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: <063D6719AE5E284EB5DD2968C1650D6D1CB7275E@AcuExch.aculab.com>
Date:	Tue, 4 Aug 2015 09:15:13 +0000
From:	David Laight <David.Laight@...LAB.COM>
To:	'Herbert Xu' <herbert@...dor.apana.org.au>,
	Brenden Blanco <bblanco@...mgrid.com>
CC:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"khlebnikov@...dex-team.ru" <khlebnikov@...dex-team.ru>
Subject: RE: Fix skb_set_peeked use-after-free bug

From: Herbert Xu
> Sent: 04 August 2015 08:43
> Brenden Blanco <bblanco@...mgrid.com> wrote:
> >> [  318.244596] BUG: unable to handle kernel NULL pointer dereference
> >> at 000000000000008e
> >> [  318.245182] IP: [<ffffffff81455e7c>] __skb_recv_datagram+0xbc/0x5a0
> >
> > Replying to myself, and adding commit interested parties...
> >
> > I went through the git log for the function in question, and
> > positively identified that the following commit introduces the crash:
> >
> > 738ac1e net: Clone skb before setting peeked flag
> >
> > Null dereference is at line 224 of net/core/datagram.c (according to
> > my objdump dis-assembly):
> 
> Sorry, brain fart on my part.  Please try this patch.

You've introduced a memory leak if skb_clone() fails.

> The commit 738ac1ebb96d02e0d23bc320302a6ea94c612dec ("net: Clone
> skb before setting peeked flag") introduced a use-after-free bug
> in skb_recv_datagram.  This is because skb_set_peeked may create
> a new skb and free the existing one.  As it stands the caller will
> continue to use the old freed skb.
> 
> This patch fixes it by making skb_set_peeked return the new skb
> (or the old one if unchanged).
...
>  	nskb = skb_clone(skb, GFP_ATOMIC);
>  	if (!nskb)
> -		return -ENOMEM;
> +		return ERR_PTR(-ENOMEM);

Here the original skb is still allocated.

> -				error = skb_set_peeked(skb);
> -				if (error)
> +				skb = skb_set_peeked(skb);

You've now lost the address of the original skb.

> +				error = PTR_ERR(skb);
> +				if (IS_ERR(skb))
>  					goto unlock_err;

	David


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html