netdev - Re: [v2 PATCH 1/2] macvlan: Fix potential use-after free for broadcasts

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: <CAM_iQpU3qnV6Zf5bCKMM6smSsDVD9Jeb1VffCi7RaCKsHREHdw@mail.gmail.com>
Date:	Wed, 1 Jun 2016 16:36:53 -0700
From:	Cong Wang <xiyou.wangcong@...il.com>
To:	Herbert Xu <herbert@...dor.apana.org.au>
Cc:	David Miller <davem@...emloft.net>,
	Lennert Buytenhek <buytenh@...tstofly.org>,
	Patrick McHardy <kaber@...sh.net>,
	Linux Kernel Network Developers <netdev@...r.kernel.org>,
	Jiri Pirko <jpirko@...hat.com>
Subject: Re: [v2 PATCH 1/2] macvlan: Fix potential use-after free for broadcasts

On Tue, May 31, 2016 at 9:27 PM, Herbert Xu <herbert@...dor.apana.org.au> wrote:
> On Tue, May 31, 2016 at 09:19:37PM -0700, Cong Wang wrote:
>>
>> Hmm, why could this happen? The upper device should be linked
>> with the lower device, where a refcount is already held.
>> Also, the work is cancelled in ->uninit().
>
> Of course it can happen.  We are talking about the source macvlan
> device that we just looked up using the Ethernet address.  That
> device has nothing to do with the packet now so it may be deleted
> at any time.
>
> We do flush the work but only when the all macvlan devices on a
> port have been deleted.  Perhaps you're confusing the source
> device with vlan->lowerdev which is confusingly the actual
> hardware device?

I thought all the on-flying packets are waited by synchronize_net()
during the removal of any of these devices. But since you moved
them to a workqueue, aka process context, so I think it won't
work any more.

Your patch makes sense to me now. :)

Thanks!