On Wed, Jan 27, 2021 at 04:23:32PM +0100, Greg Kurz wrote:
> On Wed, 27 Jan 2021 14:14:30 +0000
> Stefan Hajnoczi <stefa...@redhat.com> wrote:
>
> > On Wed, Jan 27, 2021 at 02:01:54PM +0100, Miklos Szeredi wrote:
> > > On Wed, Jan 27, 2021 at 12:21 PM Stefan Hajnoczi <stefa...@redhat.com>
> > > wrote:
> > > }
> > > > @@ -1654,9 +1677,11 @@ static void update_open_flags(int writeback, int
> > > > allow_direct_io,
> > > > static void lo_create(fuse_req_t req, fuse_ino_t parent, const char
> > > > *name,
> > > > mode_t mode, struct fuse_file_info *fi)
> > > > {
> > > > + int open_flags = (fi->flags | O_CREAT) & ~O_NOFOLLOW;
> > > > int fd;
> > > > struct lo_data *lo = lo_data(req);
> > > > struct lo_inode *parent_inode;
> > > > + struct lo_inode *existing_inode = NULL;
> > > > struct fuse_entry_param e;
> > > > int err;
> > > > struct lo_cred old = {};
> > > > @@ -1682,11 +1707,23 @@ static void lo_create(fuse_req_t req,
> > > > fuse_ino_t parent, const char *name,
> > > >
> > > > update_open_flags(lo->writeback, lo->allow_direct_io, fi);
> > > >
> > > > - fd = openat(parent_inode->fd, name, (fi->flags | O_CREAT) &
> > > > ~O_NOFOLLOW,
> > > > - mode);
> > > > + /* First, try to create a new file but don't open existing files */
> > > > + fd = openat(parent_inode->fd, name, open_flags | O_EXCL, mode);
> > > > err = fd == -1 ? errno : 0;
> > > > +
> > > > lo_restore_cred(&old);
> > > >
> > > > + /* Second, open existing files if O_EXCL was not specified */
> > > > + if (err == EEXIST && !(fi->flags & O_EXCL)) {
> > > > + existing_inode = lookup_name(req, parent, name);
> > > > + if (existing_inode) {
> > > > + fd = lo_inode_open(lo, existing_inode, open_flags);
> > > > + if (fd < 0) {
> > > > + err = -fd;
> > > > + }
> > > > + }
> > > > + }
> > > > +
> > > > if (!err) {
> > > > ssize_t fh;
> > >
> > > It's more of a mess than I thought.
> > >
> > > The problem here is there can also be a race between the open and the
> > > subsequent lo_do_lookup().
> > >
> > > At this point it's probably enough to verify that fuse_entry_param
> > > refers to the same object as the fh (using fstat and comparing st_dev
> > > and st_ino).
> >
> > Can you describe the race in detail? FUSE_CREATE vs FUSE_OPEN?
> > FUSE_CREATE vs FUSE_CREATE?
> >
> > > Also O_CREAT open is not supposed to return ENOENT, so failure to open
> > > without O_CREAT (race between O_CREAT open and plain open) should at
> > > least translate error to ESTALE or EIO.
> >
> > Thanks, will fix.
> >
>
> Please wait, as explained in another mail, ENOENT can happen with
> O_CREAT and guest userspace should be ready to handle it.Thanks, I have now read the discussion between Miklos and yourself on the previous revision. You showed an interesting O_CREAT case where ENOENT does occur. The O_NOFOLLOW issue is worth fixing but it's not directly related to this CVE so it can be done in a separate patch. Miklos, Greg: Any other topics to discuss regarding this patch or shall we merge it? Stefan
signature.asc
Description: PGP signature
