Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 15 Nov 2022 12:12:44 -0800
From:   Dmitry Torokhov <dmitry.torokhov@gmail.com>
To:     Luis Chamberlain <mcgrof@kernel.org>
Cc:     Tejun Heo <tj@kernel.org>,
        Matthieu Castet <castet.matthieu@free.fr>,
        Stanislaw Gruszka <stf_xl@wp.pl>, ming.lei@redhat.com,
        Hillf Danton <hdanton@sina.com>,
        syzbot <syzbot+6bc35f3913193fe7f0d3@syzkaller.appspotmail.com>,
        linux-kernel@vger.kernel.org, syzkaller-bugs@googlegroups.com
Subject: Re: [syzbot] KASAN: use-after-free Read in
 kernfs_next_descendant_post (2)
Message-ID: <Y3PyvDZsJUXs1uSk@google.com>
References: <000000000000c183f505eb721745@google.com>
 <20221021225228.1750-1-hdanton@sina.com>
 <Y2BRzPxroVylPLan@slm.duckdns.org>
 <Y3J8GKR905SZ84EE@bombadil.infradead.org>
 <Y3KDxpuoY4PB22vt@google.com>
 <Y3Pp7geXZRX3ltNg@bombadil.infradead.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y3Pp7geXZRX3ltNg@bombadil.infradead.org>
Precedence: bulk

On Tue, Nov 15, 2022 at 11:35:10AM -0800, Luis Chamberlain wrote:
> On Mon, Nov 14, 2022 at 10:07:02AM -0800, Dmitry Torokhov wrote:
> > I do not see how moving the point where we acquire device refcount
> > around fixes anything.
> 
> The patch I posted does two things, moving the point where we acquire
> device refcount was just one so it was not clear that what I really
> wanted to be enforce a check for first, and that is that the driver
> *did* do the correct thing.
> 
> So while we can surely expect the driver to do proper device refcounting
> and waiting on device removal, buggy drivers do exist and we should
> strive to not allow UAF with them.

You can not enforce any of that from the firmware loader itself.

> 
> So something like this:
> 
> From 92c8f4465a205e744c70dcba320708f72900442e Mon Sep 17 00:00:00 2001
> From: Luis Chamberlain <mcgrof@kernel.org>
> Date: Tue, 15 Nov 2022 10:02:13 -0800
> Subject: [PATCH] firmware_loader: avoid UAF on buggy request_firmware_nowait()
>  users
> 
> request_firmware_nowait() is documented as requiring the caller to
> ensure to maintain the the reference count of @device during the
> lifetime of the call to request_firmware_nowait() and the callback.
> 
> It would seem drivers exist which don't follow these rules though,
> and things like syzbot can trigger UAF if the device gets nuked
> as request_firmware_nowait() is being called. Instead of enabling
> use UAF, defend against such improperly written drivers and complain
> about it.

I fail to see how are you defending against improperly written drivers
and in what cases you expect your check to trigger. It is impossible for 
get_device() device to fail for non-NULL device (check the code), so
your test will never trigger.

> 
> Make the documentaiton a bit clearer and give a hint as to how to easily
> accomplish device lifetime maintenance on the driver using a completion
> and a wait_for_completion().

It is not clear to me why the caller must keep reference to device. The
callback is called with struct firmware and context pointer, which may
or may not be tied to a device instance. What you want to say is that
the caller must ensure that context is valid until after callback is
invoked.

The firmware loader uses device structure itself and does acquire
a reference, so it does the right thing, but the caller is free to drop
the device reference if it chooses to do so.

So for what its worth it is a NAK from me.

> 
> Fixes: 0cfc1e1e7b534 ("firmware loader: fix device lifetime")
> Fixes: f8a4bd3456b98 ("firmware loader: embed device into firmware_priv structure")
> Cc: stable@vger.kernel.org # v2.6.36
> Reported-by: syzbot+6bc35f3913193fe7f0d3@syzkaller.appspotmail.com
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
>  drivers/base/firmware_loader/main.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
> index 7c3590fd97c2..6ac92dfdd85e 100644
> --- a/drivers/base/firmware_loader/main.c
> +++ b/drivers/base/firmware_loader/main.c
> @@ -1118,15 +1118,16 @@ static void request_firmware_work_func(struct work_struct *work)
>   * @uevent: sends uevent to copy the firmware image if this flag
>   *	is non-zero else the firmware copy must be done manually.
>   * @name: name of firmware file
> - * @device: device for which firmware is being loaded
> + * @device: device for which firmware is being loaded. The caller must hold
> + * 	the reference count of @device during the lifetime of this routine
> + * 	and the @cont callback. This typically can be done with a completion
> + * 	and wait_for_completion prior to device teardown.
>   * @gfp: allocation flags
>   * @context: will be passed over to @cont, and
>   *	@fw may be %NULL if firmware request fails.
>   * @cont: function will be called asynchronously when the firmware
>   *	request is over.
>   *
> - *	Caller must hold the reference count of @device.
> - *
>   *	Asynchronous variant of request_firmware() for user contexts:
>   *		- sleep for as small periods as possible since it may
>   *		  increase kernel boot time of built-in device drivers
> @@ -1171,7 +1172,12 @@ request_firmware_nowait(
>  		return -EFAULT;
>  	}
>  
> -	get_device(fw_work->device);
> +	if (WARN_ON(!get_device(fw_work->device))) {
> +		module_put(module);
> +		kfree_const(fw_work->name);
> +		kfree(fw_work);
> +		return -ENODEV;
> +	}
>  	INIT_WORK(&fw_work->work, request_firmware_work_func);
>  	schedule_work(&fw_work->work);
>  	return 0;
> -- 
> 2.35.1
> 

Thanks.

-- 
Dmitry