linux-kernel - Re: [PATCH] Fuse: Add backing file support for uring

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: <a8af0bfc-d739-49aa-ac3f-4f928741fb7a@bsbernd.com>
Date: Fri, 21 Feb 2025 18:13:35 +0100
From: Bernd Schubert <bernd@...ernd.com>
To: Amir Goldstein <amir73il@...il.com>,
 Moinak Bhattacharyya <moinakb001@...il.com>
Cc: Miklos Szeredi <miklos@...redi.hu>, linux-fsdevel@...r.kernel.org,
 linux-kernel@...r.kernel.org, io-uring@...r.kernel.org
Subject: Re: [PATCH] Fuse: Add backing file support for uring_cmd



On 2/21/25 17:24, Amir Goldstein wrote:
> On Fri, Feb 21, 2025 at 4:36 PM Moinak Bhattacharyya
> <moinakb001@...il.com> wrote:
>>
>> Sorry about that. Correctly-formatted patch follows. Should I send out a
>> V2 instead?
>>
>> Add support for opening and closing backing files in the fuse_uring_cmd
>> callback. Store backing_map (for open) and backing_id (for close) in the
>> uring_cmd data.
>> ---
>>   fs/fuse/dev_uring.c       | 50 +++++++++++++++++++++++++++++++++++++++
>>   include/uapi/linux/fuse.h |  6 +++++
>>   2 files changed, 56 insertions(+)
>>
>> diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c
>> index ebd2931b4f2a..df73d9d7e686 100644
>> --- a/fs/fuse/dev_uring.c
>> +++ b/fs/fuse/dev_uring.c
>> @@ -1033,6 +1033,40 @@ fuse_uring_create_ring_ent(struct io_uring_cmd *cmd,
>>       return ent;
>>   }
>>
>> +/*
>> + * Register new backing file for passthrough, getting backing map from
>> URING_CMD data
>> + */
>> +static int fuse_uring_backing_open(struct io_uring_cmd *cmd,
>> +    unsigned int issue_flags, struct fuse_conn *fc)
>> +{
>> +    const struct fuse_backing_map *map = io_uring_sqe_cmd(cmd->sqe);
>> +    int ret = fuse_backing_open(fc, map);
>> +
> 
> I am not that familiar with io_uring, so I need to ask -
> fuse_backing_open() does
> fb->cred = prepare_creds();
> to record server credentials
> what are the credentials that will be recorded in the context of this
> io_uring command?

This is run from the io_uring_enter() syscall - it should not make
a difference to an ioctl, AFAIK. Someone from @io-uring please
correct me if I'm wrong.

> 
> 
>> +    if (ret < 0) {
>> +        return ret;
>> +    }
>> +
>> +    io_uring_cmd_done(cmd, ret, 0, issue_flags);
>> +    return 0;
>> +}
>> +
>> +/*
>> + * Remove file from passthrough tracking, getting backing_id from
>> URING_CMD data
>> + */
>> +static int fuse_uring_backing_close(struct io_uring_cmd *cmd,
>> +    unsigned int issue_flags, struct fuse_conn *fc)
>> +{
>> +    const int *backing_id = io_uring_sqe_cmd(cmd->sqe);
>> +    int ret = fuse_backing_close(fc, *backing_id);
>> +
>> +    if (ret < 0) {
>> +        return ret;
>> +    }
>> +
>> +    io_uring_cmd_done(cmd, ret, 0, issue_flags);
>> +    return 0;
>> +}
>> +
>>   /*
>>    * Register header and payload buffer with the kernel and puts the
>>    * entry as "ready to get fuse requests" on the queue
>> @@ -1144,6 +1178,22 @@ int fuse_uring_cmd(struct io_uring_cmd *cmd,
>> unsigned int issue_flags)
>>               return err;
>>           }
>>           break;
>> +    case FUSE_IO_URING_CMD_BACKING_OPEN:
>> +        err = fuse_uring_backing_open(cmd, issue_flags, fc);
>> +        if (err) {
>> +            pr_info_once("FUSE_IO_URING_CMD_BACKING_OPEN failed err=%d\n",
>> +                    err);
>> +            return err;
>> +        }
>> +        break;
>> +    case FUSE_IO_URING_CMD_BACKING_CLOSE:
>> +        err = fuse_uring_backing_close(cmd, issue_flags, fc);
>> +        if (err) {
>> +            pr_info_once("FUSE_IO_URING_CMD_BACKING_CLOSE failed err=%d\n",
>> +                    err);
>> +            return err;
>> +        }
>> +        break;
>>       default:
>>           return -EINVAL;
>>       }
>> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
>> index 5e0eb41d967e..634265da1328 100644
>> --- a/include/uapi/linux/fuse.h
>> +++ b/include/uapi/linux/fuse.h
>> @@ -1264,6 +1264,12 @@ enum fuse_uring_cmd {
>>
>>       /* commit fuse request result and fetch next request */
>>       FUSE_IO_URING_CMD_COMMIT_AND_FETCH = 2,
>> +
>> +    /* add new backing file for passthrough */
>> +    FUSE_IO_URING_CMD_BACKING_OPEN = 3,
>> +
>> +    /* remove passthrough file by backing_id */
>> +    FUSE_IO_URING_CMD_BACKING_CLOSE = 4,
>>   };
>>
> 
> An anecdote:
> Why are we using FUSE_DEV_IOC_BACKING_OPEN
> and not passing the backing fd directly in OPEN response?
> 
> The reason for that was security related - there was a concern that
> an adversary would be able to trick some process into writing some fd
> to /dev/fuse, whereas tricking some proces into doing an ioctl is not
> so realistic.
> 
> AFAICT this concern does not exist when OPEN response is via
> io_uring(?), so the backing_id indirection is not strictly needed,
> but for the sake of uniformity with standard fuse protocol,
> I guess we should maintain those commands in io_uring as well.

Yeah, the way it is done is not ideal

fi->backing_id = do_passthrough_open(); /* blocking */
fuse_reply_create()
    fill_open()
      arg->backing_id = f->backing_id; /* f is fi */


I.e. there are still two operations that depend on each other.
Maybe we could find a way to link the SQEs.
Or maybe easier, if the security concern is gone with IO-URING,
just set FOPEN_PASSTHROUGH for requests over io-uring and then
let the client/kernel side do the passthrough open internally?


Thanks,
Bernd