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:   Fri, 29 Oct 2021 09:13:24 -0300
From:   Jason Gunthorpe <jgg@nvidia.com>
To:     "Ziyang Xuan (William)" <william.xuanziyang@huawei.com>
Cc:     Jakub Kicinski <kuba@kernel.org>, 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: <20211029121324.GT2744544@nvidia.com>
References: <20211027121606.3300860-1-william.xuanziyang@huawei.com>
 <20211027184640.7955767e@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
 <20211028114503.GM2744544@nvidia.com>
 <20211028070050.6ca7893b@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
 <b573b01c-2cc9-4722-6289-f7b9e0a43e19@huawei.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <b573b01c-2cc9-4722-6289-f7b9e0a43e19@huawei.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?E+bg2Ii36b9VA3ocraODrdJjWnlLK7MgA8OMDaEz3oxSaUlB1Rg3TF8k/Jn6?=
 =?us-ascii?Q?8EhBUN0xoHu3bj30+lLxUdWgGCo/9jywItHE/9HKmJPvBMUtJaxjhwpNELV6?=
 =?us-ascii?Q?/83O+U3SeLEXcIjNmvJggPnE/HIRikIHAYHd9H4VUPwYBhZJGOAIMvCWE8s4?=
 =?us-ascii?Q?gpUMarGowfti++Qu5T25uCykltfZk3kusnGXRitcT1VlOtqznAXWR9KHBVSC?=
 =?us-ascii?Q?Xb0op/7ngE+13osHcKuDe8J2SVce+/d4z/6EuFXzf0vnI+Qn92K8aB8BEfun?=
 =?us-ascii?Q?+GGnJrz3OLipltPq4o9Ml6ZgDlQm7hiprH0Gvyc4JeVDO0Xa6E1SsiF6dGUR?=
 =?us-ascii?Q?5K73088Lvi5PTv4p+0gHtm0wXql70jOqc9aCRwCm1IqXesosKtzjRguCfHkj?=
 =?us-ascii?Q?zNPDaXmax9Mg/wtQ9z1K6azU2onaw9ZoQIt9sYeMazVwCC115HmkWmDU7zGe?=
 =?us-ascii?Q?eaG+zS9ITWdgDgWqJD8Ad9K00so2a/OoOGDFtaT3nfBpQYExOn676yTcvNHh?=
 =?us-ascii?Q?DLigTpUpIzMlpgXurZOYKb8F9Cclx9C0NCxhxbx00+/ol5II3dh6Qt1HYdQQ?=
 =?us-ascii?Q?0W73/yQaDlPQWU5+Z31zU6Aw0s8O9k56GfC8ANkn5f0cWfnMry4MWqc7po+K?=
 =?us-ascii?Q?H18V46CNWvMsuQb7DlBj62Ph191v6BR3MkiIIrCoTjUuyx6dTqcY65jELNEs?=
 =?us-ascii?Q?idSK4uC0bvgmLnENS13rJQtpC7JHrHBdlb3ldcImS8TEXCIejs4/lueemqPc?=
 =?us-ascii?Q?P7uDkgK7VTh8tmCkx/+Yj7+/Vee1HOu19Fm390OE09Do8wjD9YCXAc1KtYhI?=
 =?us-ascii?Q?Q84UD1Z7g2+M8BkswY6pgZaHfpVD++XwDkoqRfmgCJiUDhRHD+TMAewn0yUQ?=
 =?us-ascii?Q?UnIeA0IykTys5Mr5J8UyJGZ7JUY5TA6BCw8sZ38eB7hG/dqYkrA9lj12sVru?=
 =?us-ascii?Q?RepUqcFbxZt6DQnxGefz4ezJhrHwIj6igOyZXitrpaQquHXUIN5ZDIJsDLeA?=
 =?us-ascii?Q?TuMEv7hrokmhix+PmU9qCtsbg1B7E//iMJwj4FkQ8wFX0PcKoeQpZznQcxz/?=
 =?us-ascii?Q?jE7pVCgc0emy4bCXPzyhbbnMUN0B+5c4jpmiLHGvIgsWg+PB8ZXJL+FLiBVm?=
 =?us-ascii?Q?LKOJylWS2CmxQ9qMy+H8Nnymila57GqRzBuLpf2kJyRzIVbiPDTkT6r95MJr?=
 =?us-ascii?Q?NSmBESFhAAo/M/kkpMuga1YHT66GzGJbxI4pG62DcyF2Xw/EoHY9SuVR8Lrk?=
 =?us-ascii?Q?8KR2o81Xvux9r4vHKHNeS7rI5PCCIqC0UDssQsyTiriKGZwlvmM2fFdzStuy?=
 =?us-ascii?Q?Dx0mW7/k0GGNki8KQGuah4ULflqFZMeZAwFWa/noQ4uedg88Nw0E1tn5lEDq?=
 =?us-ascii?Q?QYmkt2vtZoNhl7WgtEC67eE4XKjR27eFjHTs9A1a9eDN+3RLy5qK1opiNUkV?=
 =?us-ascii?Q?ZaM40Oa/Wso6sod/uUl3RVY2bc0FzBVkafBBnTuHeyqhEqHnsKectJ2iTgS1?=
 =?us-ascii?Q?5pPTwhXl4khPjEJe6sIhOLj+S/m/PJpXAFd+vNwtIrv8d1WKIcA0rud1pUN6?=
 =?us-ascii?Q?T3x1Nc+oxba40Be2gvc=3D?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: b74122e6-5926-4dea-a41f-08d99ad5829f
X-MS-Exchange-CrossTenant-AuthSource: BL0PR12MB5506.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Oct 2021 12:13:26.2018
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 1qCpV7wVjNhJbOcCJQbOWyC7GKlXpiYdtL6kwhJkSdWR/pKzTxIJfHs6Nlij8LFD
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL1PR12MB5320
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Fri, Oct 29, 2021 at 03:04:35PM +0800, Ziyang Xuan (William) wrote:
> > 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.
> > .
> > 
> 
> I am sorry. I have made a misunderstanding and given a wrong conclusion
> that unregister_vlan_dev() just move the vlan_ndev to a list to unregister
> later and it is possible the real_dev has been freed when we access in
> netdevice_queue_work().
> 
> real_ndev UNREGISTE trigger NETDEV_UNREGISTER notification in
> vlan_device_event(), unregister_vlan_dev() and unregister_netdevice_many()
> are within real_ndev UNREGISTE process. real_dev and vlan_ndev are all
> alive before real_ndev UNREGISTE finished.
> 
> Above is the correction for my previous misunderstanding. But the real
> scenario of the problem is as following:
> 
> __rtnl_newlink
> vlan_newlink
> register_vlan_dev(vlan_ndev, ...)
> register_netdevice(vlan_ndev)
> netdevice_queue_work(..., vlan_ndev) [dev_hold(vlan_ndev)]
> queue_work(gid_cache_wq, ...)

This is exactly what I'm saying, the rdma code saw a registered device
and captured a ref on it, passing it to a work queue.

> rtnl_configure_link(vlan_ndev, ...) [failed]
> ops->dellink(vlan_ndev, &list_kill) [unregister_vlan_dev]
	/* Get rid of the vlan's reference to real_dev */
	dev_put(real_dev);
> unregister_netdevice_many(&list_kill)

Then it released the real_dev reference, leaving a dangled pointer and
goes into unregister_netdevice_many which does:

		dev->reg_state = NETREG_UNREGISTERING;
and eventually

		net_set_todo(dev);

then unlocks RTNL. The get prevents it from progressing past
NETREG_UNREGISTERING

Now later we touch the vlan dev, it is reg_state UNREGISTERED and it's
memory is corrupted because it dropped the ref it was holding on the
pointer it returns, which has now since been freed.

The only reason the dangled pointer doesn't cause larger problems, is
because rtnl saves it - but continuing to reference a pointer that no
longer has a valid ref is certainly a bad practice.

> So my first solution as following for the problem is correct.
> https://lore.kernel.org/linux-rdma/20211025163941.GA393143@nvidia.com/T/#m44abbf1ea5e4b5237610c1b389c3340d92a03b8d

No, it still isn't.

Jakub's path would be to test vlan_dev->reg_state != NETREG_REGISTERED
in the work queue, but that feels pretty hacky to me as the main point
of the UNREGISTERING state is to keep the object alive enough that
those with outstanding gets can compelte their work and release the
get. Leaving a wrecked object in UNREGISTERING is a bad design.

Jason