Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 28 Oct 2021 07:00:50 -0700
From:   Jakub Kicinski <kuba@kernel.org>
To:     Jason Gunthorpe <jgg@nvidia.com>
Cc:     Ziyang Xuan <william.xuanziyang@huawei.com>, davem@davemloft.net,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-rdma@vger.kernel.org
Subject: Re: [PATCH net] net: vlan: fix a UAF in vlan_dev_real_dev()
Message-ID: <20211028070050.6ca7893b@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
In-Reply-To: <20211028114503.GM2744544@nvidia.com>
References: <20211027121606.3300860-1-william.xuanziyang@huawei.com>
        <20211027184640.7955767e@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
        <20211028114503.GM2744544@nvidia.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Thu, 28 Oct 2021 08:45:03 -0300 Jason Gunthorpe wrote:
> > But will make all the callers of vlan_dev_real_dev() feel like they
> > should NULL-check the result, which is not necessary.  
> 
> Isn't it better to reliably return NULL instead of a silent UAF in
> this edge case? 

I don't know what the best practice is for maintaining sanity of
unregistered objects.

If there really is a requirement for the real_dev pointer to be sane we
may want to move the put_device(real_dev) to vlan_dev_free(). There
should not be any risk of circular dependency but I'm not 100% sure.

> > RDMA must be calling this helper on a vlan which was already
> > unregistered, can we fix RDMA instead?  
> 
> RDMA holds a get on the netdev which prevents unregistration, however
> unregister_vlan_dev() does:
> 
>         unregister_netdevice_queue(dev, head);
>         dev_put(real_dev);
> 
> Which corrupts the still registered vlan device while it is sitting in
> the queue waiting to unregister. So, it is not true that a registered
> vlan device always has working vlan_dev_real_dev().

That's not my reading, unless we have a different definition of
"registered". The RDMA code in question runs from a workqueue, at the
time the UNREGISTER notification is generated all objects are still
alive and no UAF can happen. Past UNREGISTER extra care is needed when
accessing the object.

Note that unregister_vlan_dev() may queue the unregistration, without
running it. If it clears real_dev the UNREGISTER notification will no
longer be able to access real_dev, which used to be completely legal.