linux-kernel - Re: [PATCH v2] fuse: prevent possible NULL pointer dereference in fuse_iomap_writeback

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: <CAJnrk1aWaZLcZkQ_OZhQd8ZfHC=ix6_TZ8ZW270PWu0418gOmA@mail.gmail.com>
Date: Wed, 3 Sep 2025 10:03:28 -0700
From: Joanne Koong <joannelkoong@...il.com>
To: Luis Henriques <luis@...lia.com>
Cc: Miklos Szeredi <miklos@...redi.hu>, linux-fsdevel@...r.kernel.org, 
	linux-kernel@...r.kernel.org, kernel-dev@...lia.com
Subject: Re: [PATCH v2] fuse: prevent possible NULL pointer dereference in fuse_iomap_writeback_{range,submit}()

On Wed, Sep 3, 2025 at 1:35 AM Luis Henriques <luis@...lia.com> wrote:
>
> These two functions make use of the WARN_ON_ONCE() macro to help debugging
> a NULL wpc->wb_ctx.  However, this doesn't prevent the possibility of NULL
> pointer dereferences in the code.  This patch adds some extra defensive
> checks to avoid these NULL pointer accesses.
>
> Fixes: ef7e7cbb323f ("fuse: use iomap for writeback")
> Signed-off-by: Luis Henriques <luis@...lia.com>
> ---
> Hi!
>
> This v2 results from Joanne's inputs -- I now believe that it is better to
> keep the WARN_ON_ONCE() macros, but it's still good to try to minimise
> the undesirable effects of a NULL wpc->wb_ctx.
>
> I've also added the 'Fixes:' tag to the commit message.
>
>  fs/fuse/file.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index 5525a4520b0f..990c287bc3e3 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -2135,14 +2135,18 @@ static ssize_t fuse_iomap_writeback_range(struct iomap_writepage_ctx *wpc,
>                                           unsigned len, u64 end_pos)
>  {
>         struct fuse_fill_wb_data *data = wpc->wb_ctx;
> -       struct fuse_writepage_args *wpa = data->wpa;
> -       struct fuse_args_pages *ap = &wpa->ia.ap;
> +       struct fuse_writepage_args *wpa;
> +       struct fuse_args_pages *ap;
>         struct inode *inode = wpc->inode;
>         struct fuse_inode *fi = get_fuse_inode(inode);
>         struct fuse_conn *fc = get_fuse_conn(inode);
>         loff_t offset = offset_in_folio(folio, pos);
>
> -       WARN_ON_ONCE(!data);
> +       if (WARN_ON_ONCE(!data))
> +               return -EIO;

imo this WARN_ON_ONCE (and the one below) should be left as is instead
of embedded in the "if" construct. The data pointer passed in is set
by fuse and as such, we're able to reasonably guarantee that data is a
valid pointer. Looking at other examples of WARN_ON in the fuse
codebase, the places where an "if" construct is used are for cases
where the assumptions that are made are more delicate (eg folio
mapping state, in fuse_try_move_folio()) and less clearly obvious. I
think this WARN_ON_ONCE here and below should be left as is.


Thanks,
Joanne

> +
> +       wpa = data->wpa;
> +       ap = &wpa->ia.ap;
>
>         if (!data->ff) {
>                 data->ff = fuse_write_file_get(fi);
> @@ -2182,7 +2186,8 @@ static int fuse_iomap_writeback_submit(struct iomap_writepage_ctx *wpc,
>  {
>         struct fuse_fill_wb_data *data = wpc->wb_ctx;
>
> -       WARN_ON_ONCE(!data);
> +       if (WARN_ON_ONCE(!data))
> +               return error ? error : -EIO;
>
>         if (data->wpa) {
>                 WARN_ON(!data->wpa->ia.ap.num_folios);