| Message ID | 20210203113719.83633-3-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | virtiofsd: prevent opening of special files (CVE-2020-35517) | expand |
On Wed, 3 Feb 2021 11:37:18 +0000 Stefan Hajnoczi <stefanha@redhat.com> wrote: > lo_do_lookup() finds an existing inode or allocates a new one. It > increments nlookup so that the inode stays alive until the client > releases it. > > Existing callers don't need the struct lo_inode so the function doesn't > return it. Extend the function to optionally return the inode. The next > commit will need it. > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > tools/virtiofsd/passthrough_ll.c | 29 +++++++++++++++++++++-------- > 1 file changed, 21 insertions(+), 8 deletions(-) > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c > index e63cbd3fb7..c87a1f3d72 100644 > --- a/tools/virtiofsd/passthrough_ll.c > +++ b/tools/virtiofsd/passthrough_ll.c > @@ -831,11 +831,13 @@ static int do_statx(struct lo_data *lo, int dirfd, const char *pathname, > } > > /* > - * Increments nlookup and caller must release refcount using > - * lo_inode_put(&parent). > + * Increments nlookup on the inode on success. unref_inode_lolocked() must be > + * called eventually to decrement nlookup again. If inodep is non-NULL, the > + * inode pointer is stored and the caller must call lo_inode_put(). > */ > static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, > - struct fuse_entry_param *e) > + struct fuse_entry_param *e, > + struct lo_inode **inodep) > { > int newfd; > int res; > @@ -845,6 +847,10 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, > struct lo_inode *inode = NULL; > struct lo_inode *dir = lo_inode(req, parent); > > + if (inodep) { > + *inodep = NULL; > + } > + Is this side-effect needed ? If lo_do_lookup() returns an error, it rather seems that the caller shouldn't expect anything to be written here, i.e. the content of *inodep still belongs to the caller and whatever value it previously put in there (as patch 3/3 does) should be preserved IMHO. Apart from that LGTM. > /* > * name_to_handle_at() and open_by_handle_at() can reach here with fuse > * mount point in guest, but we don't have its inode info in the > @@ -913,7 +919,14 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, > pthread_mutex_unlock(&lo->mutex); > } > e->ino = inode->fuse_ino; > - lo_inode_put(lo, &inode); > + > + /* Transfer ownership of inode pointer to caller or drop it */ > + if (inodep) { > + *inodep = inode; > + } else { > + lo_inode_put(lo, &inode); > + } > + > lo_inode_put(lo, &dir); > > fuse_log(FUSE_LOG_DEBUG, " %lli/%s -> %lli\n", (unsigned long long)parent, > @@ -948,7 +961,7 @@ static void lo_lookup(fuse_req_t req, fuse_ino_t parent, const char *name) > return; > } > > - err = lo_do_lookup(req, parent, name, &e); > + err = lo_do_lookup(req, parent, name, &e, NULL); > if (err) { > fuse_reply_err(req, err); > } else { > @@ -1056,7 +1069,7 @@ static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t parent, > goto out; > } > > - saverr = lo_do_lookup(req, parent, name, &e); > + saverr = lo_do_lookup(req, parent, name, &e, NULL); > if (saverr) { > goto out; > } > @@ -1534,7 +1547,7 @@ static void lo_do_readdir(fuse_req_t req, fuse_ino_t ino, size_t size, > > if (plus) { > if (!is_dot_or_dotdot(name)) { > - err = lo_do_lookup(req, ino, name, &e); > + err = lo_do_lookup(req, ino, name, &e, NULL); > if (err) { > goto error; > } > @@ -1732,7 +1745,7 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name, > } > > fi->fh = fh; > - err = lo_do_lookup(req, parent, name, &e); > + err = lo_do_lookup(req, parent, name, &e, NULL); > } > if (lo->cache == CACHE_NONE) { > fi->direct_io = 1;
On Wed, Feb 03, 2021 at 03:20:14PM +0100, Greg Kurz wrote: > On Wed, 3 Feb 2021 11:37:18 +0000 > Stefan Hajnoczi <stefanha@redhat.com> wrote: > > > lo_do_lookup() finds an existing inode or allocates a new one. It > > increments nlookup so that the inode stays alive until the client > > releases it. > > > > Existing callers don't need the struct lo_inode so the function doesn't > > return it. Extend the function to optionally return the inode. The next > > commit will need it. > > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > --- > > tools/virtiofsd/passthrough_ll.c | 29 +++++++++++++++++++++-------- > > 1 file changed, 21 insertions(+), 8 deletions(-) > > > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c > > index e63cbd3fb7..c87a1f3d72 100644 > > --- a/tools/virtiofsd/passthrough_ll.c > > +++ b/tools/virtiofsd/passthrough_ll.c > > @@ -831,11 +831,13 @@ static int do_statx(struct lo_data *lo, int dirfd, const char *pathname, > > } > > > > /* > > - * Increments nlookup and caller must release refcount using > > - * lo_inode_put(&parent). > > + * Increments nlookup on the inode on success. unref_inode_lolocked() must be > > + * called eventually to decrement nlookup again. If inodep is non-NULL, the > > + * inode pointer is stored and the caller must call lo_inode_put(). > > */ > > static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, > > - struct fuse_entry_param *e) > > + struct fuse_entry_param *e, > > + struct lo_inode **inodep) > > { > > int newfd; > > int res; > > @@ -845,6 +847,10 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, > > struct lo_inode *inode = NULL; > > struct lo_inode *dir = lo_inode(req, parent); > > > > + if (inodep) { > > + *inodep = NULL; > > + } > > + > > Is this side-effect needed ? If lo_do_lookup() returns an error, it > rather seems that the caller shouldn't expect anything to be written > here, i.e. the content of *inodep still belongs to the caller and > whatever value it previously put in there (as patch 3/3 does) should > be preserved IMHO. > > Apart from that LGTM. I like this approach because it prevents accessing uninitialized memory in the caller: struct lo_inode *inode; if (lo_do_lookup(..., &inodep) != 0) { goto err; } ... err: lo_inode_put(&inode); <-- uninitialized in the error case!
On Wed, 3 Feb 2021 17:00:06 +0000 Stefan Hajnoczi <stefanha@redhat.com> wrote: > On Wed, Feb 03, 2021 at 03:20:14PM +0100, Greg Kurz wrote: > > On Wed, 3 Feb 2021 11:37:18 +0000 > > Stefan Hajnoczi <stefanha@redhat.com> wrote: > > > > > lo_do_lookup() finds an existing inode or allocates a new one. It > > > increments nlookup so that the inode stays alive until the client > > > releases it. > > > > > > Existing callers don't need the struct lo_inode so the function doesn't > > > return it. Extend the function to optionally return the inode. The next > > > commit will need it. > > > > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > > --- > > > tools/virtiofsd/passthrough_ll.c | 29 +++++++++++++++++++++-------- > > > 1 file changed, 21 insertions(+), 8 deletions(-) > > > > > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c > > > index e63cbd3fb7..c87a1f3d72 100644 > > > --- a/tools/virtiofsd/passthrough_ll.c > > > +++ b/tools/virtiofsd/passthrough_ll.c > > > @@ -831,11 +831,13 @@ static int do_statx(struct lo_data *lo, int dirfd, const char *pathname, > > > } > > > > > > /* > > > - * Increments nlookup and caller must release refcount using > > > - * lo_inode_put(&parent). > > > + * Increments nlookup on the inode on success. unref_inode_lolocked() must be > > > + * called eventually to decrement nlookup again. If inodep is non-NULL, the > > > + * inode pointer is stored and the caller must call lo_inode_put(). > > > */ > > > static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, > > > - struct fuse_entry_param *e) > > > + struct fuse_entry_param *e, > > > + struct lo_inode **inodep) > > > { > > > int newfd; > > > int res; > > > @@ -845,6 +847,10 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, > > > struct lo_inode *inode = NULL; > > > struct lo_inode *dir = lo_inode(req, parent); > > > > > > + if (inodep) { > > > + *inodep = NULL; > > > + } > > > + > > > > Is this side-effect needed ? If lo_do_lookup() returns an error, it > > rather seems that the caller shouldn't expect anything to be written > > here, i.e. the content of *inodep still belongs to the caller and > > whatever value it previously put in there (as patch 3/3 does) should > > be preserved IMHO. > > > > Apart from that LGTM. > > I like this approach because it prevents accessing uninitialized memory > in the caller: > > struct lo_inode *inode; > > if (lo_do_lookup(..., &inodep) != 0) { > goto err; > } > ... > > err: > lo_inode_put(&inode); <-- uninitialized in the error case! My point is that it is the caller's business to ensure that inode doesn't contain garbage if it is to be used irrespective of the outcome of lo_do_lookup(). This is precisely what patch 3/3 does, so I don't understand the ultimate purpose of nullifying the inode pointer _again_ in lo_do_lookup()...
On Thu, Feb 04, 2021 at 09:25:28AM +0100, Greg Kurz wrote: > On Wed, 3 Feb 2021 17:00:06 +0000 > Stefan Hajnoczi <stefanha@redhat.com> wrote: > > > On Wed, Feb 03, 2021 at 03:20:14PM +0100, Greg Kurz wrote: > > > On Wed, 3 Feb 2021 11:37:18 +0000 > > > Stefan Hajnoczi <stefanha@redhat.com> wrote: > > > > > > > lo_do_lookup() finds an existing inode or allocates a new one. It > > > > increments nlookup so that the inode stays alive until the client > > > > releases it. > > > > > > > > Existing callers don't need the struct lo_inode so the function doesn't > > > > return it. Extend the function to optionally return the inode. The next > > > > commit will need it. > > > > > > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > > > --- > > > > tools/virtiofsd/passthrough_ll.c | 29 +++++++++++++++++++++-------- > > > > 1 file changed, 21 insertions(+), 8 deletions(-) > > > > > > > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c > > > > index e63cbd3fb7..c87a1f3d72 100644 > > > > --- a/tools/virtiofsd/passthrough_ll.c > > > > +++ b/tools/virtiofsd/passthrough_ll.c > > > > @@ -831,11 +831,13 @@ static int do_statx(struct lo_data *lo, int dirfd, const char *pathname, > > > > } > > > > > > > > /* > > > > - * Increments nlookup and caller must release refcount using > > > > - * lo_inode_put(&parent). > > > > + * Increments nlookup on the inode on success. unref_inode_lolocked() must be > > > > + * called eventually to decrement nlookup again. If inodep is non-NULL, the > > > > + * inode pointer is stored and the caller must call lo_inode_put(). > > > > */ > > > > static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, > > > > - struct fuse_entry_param *e) > > > > + struct fuse_entry_param *e, > > > > + struct lo_inode **inodep) > > > > { > > > > int newfd; > > > > int res; > > > > @@ -845,6 +847,10 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, > > > > struct lo_inode *inode = NULL; > > > > struct lo_inode *dir = lo_inode(req, parent); > > > > > > > > + if (inodep) { > > > > + *inodep = NULL; > > > > + } > > > > + > > > > > > Is this side-effect needed ? If lo_do_lookup() returns an error, it > > > rather seems that the caller shouldn't expect anything to be written > > > here, i.e. the content of *inodep still belongs to the caller and > > > whatever value it previously put in there (as patch 3/3 does) should > > > be preserved IMHO. > > > > > > Apart from that LGTM. > > > > I like this approach because it prevents accessing uninitialized memory > > in the caller: > > > > struct lo_inode *inode; > > > > if (lo_do_lookup(..., &inodep) != 0) { > > goto err; > > } > > ... > > > > err: > > lo_inode_put(&inode); <-- uninitialized in the error case! > > My point is that it is the caller's business to ensure that inode > doesn't contain garbage if it is to be used irrespective of the > outcome of lo_do_lookup(). This is precisely what patch 3/3 does, > so I don't understand the ultimate purpose of nullifying the > inode pointer _again_ in lo_do_lookup()... APIs should be designed to eliminate classes of errors where possible IMO. Taking care regarding the uninitialized pointer in the error case could be the caller's responsibility, but what's the advantage? (There's a related thing with lo_inode_put(&inode) where it sets *inode = NULL to eliminate use-after-free bugs in callers. It would have been possible to use the same approach as free(3) where it's the caller's responsiblity, but that API design decision in free(3) has caused many bugs in applications.) Stefan
On Thu, 4 Feb 2021 09:45:37 +0000 Stefan Hajnoczi <stefanha@redhat.com> wrote: > On Thu, Feb 04, 2021 at 09:25:28AM +0100, Greg Kurz wrote: > > On Wed, 3 Feb 2021 17:00:06 +0000 > > Stefan Hajnoczi <stefanha@redhat.com> wrote: > > > > > On Wed, Feb 03, 2021 at 03:20:14PM +0100, Greg Kurz wrote: > > > > On Wed, 3 Feb 2021 11:37:18 +0000 > > > > Stefan Hajnoczi <stefanha@redhat.com> wrote: > > > > > > > > > lo_do_lookup() finds an existing inode or allocates a new one. It > > > > > increments nlookup so that the inode stays alive until the client > > > > > releases it. > > > > > > > > > > Existing callers don't need the struct lo_inode so the function doesn't > > > > > return it. Extend the function to optionally return the inode. The next > > > > > commit will need it. > > > > > > > > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > > > > --- > > > > > tools/virtiofsd/passthrough_ll.c | 29 +++++++++++++++++++++-------- > > > > > 1 file changed, 21 insertions(+), 8 deletions(-) > > > > > > > > > > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c > > > > > index e63cbd3fb7..c87a1f3d72 100644 > > > > > --- a/tools/virtiofsd/passthrough_ll.c > > > > > +++ b/tools/virtiofsd/passthrough_ll.c > > > > > @@ -831,11 +831,13 @@ static int do_statx(struct lo_data *lo, int dirfd, const char *pathname, > > > > > } > > > > > > > > > > /* > > > > > - * Increments nlookup and caller must release refcount using > > > > > - * lo_inode_put(&parent). > > > > > + * Increments nlookup on the inode on success. unref_inode_lolocked() must be > > > > > + * called eventually to decrement nlookup again. If inodep is non-NULL, the > > > > > + * inode pointer is stored and the caller must call lo_inode_put(). > > > > > */ > > > > > static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, > > > > > - struct fuse_entry_param *e) > > > > > + struct fuse_entry_param *e, > > > > > + struct lo_inode **inodep) > > > > > { > > > > > int newfd; > > > > > int res; > > > > > @@ -845,6 +847,10 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, > > > > > struct lo_inode *inode = NULL; > > > > > struct lo_inode *dir = lo_inode(req, parent); > > > > > > > > > > + if (inodep) { > > > > > + *inodep = NULL; > > > > > + } > > > > > + > > > > > > > > Is this side-effect needed ? If lo_do_lookup() returns an error, it > > > > rather seems that the caller shouldn't expect anything to be written > > > > here, i.e. the content of *inodep still belongs to the caller and > > > > whatever value it previously put in there (as patch 3/3 does) should > > > > be preserved IMHO. > > > > > > > > Apart from that LGTM. > > > > > > I like this approach because it prevents accessing uninitialized memory > > > in the caller: > > > > > > struct lo_inode *inode; > > > > > > if (lo_do_lookup(..., &inodep) != 0) { > > > goto err; > > > } > > > ... > > > > > > err: > > > lo_inode_put(&inode); <-- uninitialized in the error case! > > > > My point is that it is the caller's business to ensure that inode > > doesn't contain garbage if it is to be used irrespective of the > > outcome of lo_do_lookup(). This is precisely what patch 3/3 does, > > so I don't understand the ultimate purpose of nullifying the > > inode pointer _again_ in lo_do_lookup()... > > APIs should be designed to eliminate classes of errors where possible > IMO. Taking care regarding the uninitialized pointer in the error case > could be the caller's responsibility, but what's the advantage? > Because it is more explicit. FWIW caller is still responsible since it calls lo_inode_put() in the end : initializing inode to NULL like patch 3/3 does warrants that no matter what happens in lo_do_lookup() or even if it isn't called at all, inode can be safely passed to lo_inode_put(). But this change doesn't hurt, especially with the benefits of the rest of this series, so: Reviewed-by: Greg Kurz <groug@kaod.org> > (There's a related thing with lo_inode_put(&inode) where it sets *inode > = NULL to eliminate use-after-free bugs in callers. It would have been > possible to use the same approach as free(3) where it's the caller's > responsiblity, but that API design decision in free(3) has caused > many bugs in applications.) > > Stefan
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c index e63cbd3fb7..c87a1f3d72 100644 --- a/tools/virtiofsd/passthrough_ll.c +++ b/tools/virtiofsd/passthrough_ll.c @@ -831,11 +831,13 @@ static int do_statx(struct lo_data *lo, int dirfd, const char *pathname, } /* - * Increments nlookup and caller must release refcount using - * lo_inode_put(&parent). + * Increments nlookup on the inode on success. unref_inode_lolocked() must be + * called eventually to decrement nlookup again. If inodep is non-NULL, the + * inode pointer is stored and the caller must call lo_inode_put(). */ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, - struct fuse_entry_param *e) + struct fuse_entry_param *e, + struct lo_inode **inodep) { int newfd; int res; @@ -845,6 +847,10 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, struct lo_inode *inode = NULL; struct lo_inode *dir = lo_inode(req, parent); + if (inodep) { + *inodep = NULL; + } + /* * name_to_handle_at() and open_by_handle_at() can reach here with fuse * mount point in guest, but we don't have its inode info in the @@ -913,7 +919,14 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t parent, const char *name, pthread_mutex_unlock(&lo->mutex); } e->ino = inode->fuse_ino; - lo_inode_put(lo, &inode); + + /* Transfer ownership of inode pointer to caller or drop it */ + if (inodep) { + *inodep = inode; + } else { + lo_inode_put(lo, &inode); + } + lo_inode_put(lo, &dir); fuse_log(FUSE_LOG_DEBUG, " %lli/%s -> %lli\n", (unsigned long long)parent, @@ -948,7 +961,7 @@ static void lo_lookup(fuse_req_t req, fuse_ino_t parent, const char *name) return; } - err = lo_do_lookup(req, parent, name, &e); + err = lo_do_lookup(req, parent, name, &e, NULL); if (err) { fuse_reply_err(req, err); } else { @@ -1056,7 +1069,7 @@ static void lo_mknod_symlink(fuse_req_t req, fuse_ino_t parent, goto out; } - saverr = lo_do_lookup(req, parent, name, &e); + saverr = lo_do_lookup(req, parent, name, &e, NULL); if (saverr) { goto out; } @@ -1534,7 +1547,7 @@ static void lo_do_readdir(fuse_req_t req, fuse_ino_t ino, size_t size, if (plus) { if (!is_dot_or_dotdot(name)) { - err = lo_do_lookup(req, ino, name, &e); + err = lo_do_lookup(req, ino, name, &e, NULL); if (err) { goto error; } @@ -1732,7 +1745,7 @@ static void lo_create(fuse_req_t req, fuse_ino_t parent, const char *name, } fi->fh = fh; - err = lo_do_lookup(req, parent, name, &e); + err = lo_do_lookup(req, parent, name, &e, NULL); } if (lo->cache == CACHE_NONE) { fi->direct_io = 1;
lo_do_lookup() finds an existing inode or allocates a new one. It increments nlookup so that the inode stays alive until the client releases it. Existing callers don't need the struct lo_inode so the function doesn't return it. Extend the function to optionally return the inode. The next commit will need it. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- tools/virtiofsd/passthrough_ll.c | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-)