Re: [Qemu-devel] [Virtio-fs] [PATCH 2/5] virtiofsd: prevent lo_lookup() NULL pointer dereference
On 2019/7/29 23:41, Stefan Hajnoczi wrote: > On Mon, Jul 29, 2019 at 08:35:36PM +0800, piaojun wrote: >> Hi Stefan, >> >> On 2019/7/26 17:11, Stefan Hajnoczi wrote: >>> Most lo_do_lookup() have already checked that the parent inode exists. >>> lo_lookup() hasn't and can therefore hit a NULL pointer dereference when >>> lo_inode(req, parent) returns NULL. >>> >>> Signed-off-by: Stefan Hajnoczi >>> --- >>> contrib/virtiofsd/passthrough_ll.c | 4 >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/contrib/virtiofsd/passthrough_ll.c >>> b/contrib/virtiofsd/passthrough_ll.c >>> index 9ae1381618..277a17fc03 100644 >>> --- a/contrib/virtiofsd/passthrough_ll.c >>> +++ b/contrib/virtiofsd/passthrough_ll.c >>> @@ -766,6 +766,10 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t >>> parent, const char *name, >>> struct lo_data *lo = lo_data(req); >>> struct lo_inode *inode, *dir = lo_inode(req, parent); >>> >>> + if (!dir) { >>> + return EBADF; >>> + } >>> + >> >> I worry about that dir will be released or set NULL just after NULL >> checking. Or could we use some lock to prevent the simultaneity? > > Yes, I agree. I haven't audited lo_inode yet, but it needs a refcount > and/or lock to ensure accesses are safe. I'll do that and other things > in a separate patch series. > > Stefan OK, that sounds good. Jun >
Re: [Qemu-devel] [Virtio-fs] [PATCH 2/5] virtiofsd: prevent lo_lookup() NULL pointer dereference
On Mon, Jul 29, 2019 at 08:35:36PM +0800, piaojun wrote: > Hi Stefan, > > On 2019/7/26 17:11, Stefan Hajnoczi wrote: > > Most lo_do_lookup() have already checked that the parent inode exists. > > lo_lookup() hasn't and can therefore hit a NULL pointer dereference when > > lo_inode(req, parent) returns NULL. > > > > Signed-off-by: Stefan Hajnoczi > > --- > > contrib/virtiofsd/passthrough_ll.c | 4 > > 1 file changed, 4 insertions(+) > > > > diff --git a/contrib/virtiofsd/passthrough_ll.c > > b/contrib/virtiofsd/passthrough_ll.c > > index 9ae1381618..277a17fc03 100644 > > --- a/contrib/virtiofsd/passthrough_ll.c > > +++ b/contrib/virtiofsd/passthrough_ll.c > > @@ -766,6 +766,10 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t > > parent, const char *name, > > struct lo_data *lo = lo_data(req); > > struct lo_inode *inode, *dir = lo_inode(req, parent); > > > > + if (!dir) { > > + return EBADF; > > + } > > + > > I worry about that dir will be released or set NULL just after NULL > checking. Or could we use some lock to prevent the simultaneity? Yes, I agree. I haven't audited lo_inode yet, but it needs a refcount and/or lock to ensure accesses are safe. I'll do that and other things in a separate patch series. Stefan signature.asc Description: PGP signature
Re: [Qemu-devel] [Virtio-fs] [PATCH 2/5] virtiofsd: prevent lo_lookup() NULL pointer dereference
Hi Stefan, On 2019/7/26 17:11, Stefan Hajnoczi wrote: > Most lo_do_lookup() have already checked that the parent inode exists. > lo_lookup() hasn't and can therefore hit a NULL pointer dereference when > lo_inode(req, parent) returns NULL. > > Signed-off-by: Stefan Hajnoczi > --- > contrib/virtiofsd/passthrough_ll.c | 4 > 1 file changed, 4 insertions(+) > > diff --git a/contrib/virtiofsd/passthrough_ll.c > b/contrib/virtiofsd/passthrough_ll.c > index 9ae1381618..277a17fc03 100644 > --- a/contrib/virtiofsd/passthrough_ll.c > +++ b/contrib/virtiofsd/passthrough_ll.c > @@ -766,6 +766,10 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t > parent, const char *name, > struct lo_data *lo = lo_data(req); > struct lo_inode *inode, *dir = lo_inode(req, parent); > > + if (!dir) { > + return EBADF; > + } > + I worry about that dir will be released or set NULL just after NULL checking. Or could we use some lock to prevent the simultaneity? Thanks, Jun > memset(e, 0, sizeof(*e)); > e->attr_timeout = lo->timeout; > e->entry_timeout = lo->timeout; >
Re: [Qemu-devel] [Virtio-fs] [PATCH 2/5] virtiofsd: prevent lo_lookup() NULL pointer dereference
On Fri, Jul 26, 2019 at 02:26:14PM -0700, Liu Bo wrote: > On Fri, Jul 26, 2019 at 10:11:00AM +0100, Stefan Hajnoczi wrote: > > Most lo_do_lookup() have already checked that the parent inode exists. > > lo_lookup() hasn't and can therefore hit a NULL pointer dereference when > > lo_inode(req, parent) returns NULL. > > > > Sigh...this one has been fixed by 3 different developers...Me, Pengtao and > Stefan. > > The following one on the ML did the exactly same thing. > --- > Subject: [Virtio-fs] [PATCH] virtiofsd: fix lo_do_lookup panic > > It needs to check for invalid parent dir. > > Signed-off-by: Peng Tao > --- One of the previously posted patches will be merged before mine. It's a shame that work has been duplicated. As a contributor I will send a ping email if there has been no response to a patch after a few days. Stefan signature.asc Description: PGP signature
Re: [Qemu-devel] [Virtio-fs] [PATCH 2/5] virtiofsd: prevent lo_lookup() NULL pointer dereference
Hi Stefan, On 2019/7/26 17:11, Stefan Hajnoczi wrote: > Most lo_do_lookup() have already checked that the parent inode exists. > lo_lookup() hasn't and can therefore hit a NULL pointer dereference when > lo_inode(req, parent) returns NULL. > > Signed-off-by: Stefan Hajnoczi > --- > contrib/virtiofsd/passthrough_ll.c | 4 > 1 file changed, 4 insertions(+) > > diff --git a/contrib/virtiofsd/passthrough_ll.c > b/contrib/virtiofsd/passthrough_ll.c > index 9ae1381618..277a17fc03 100644 > --- a/contrib/virtiofsd/passthrough_ll.c > +++ b/contrib/virtiofsd/passthrough_ll.c > @@ -766,6 +766,10 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t > parent, const char *name, > struct lo_data *lo = lo_data(req); > struct lo_inode *inode, *dir = lo_inode(req, parent); > > + if (!dir) { > + return EBADF; > + } > + I worry about that dir will be released or set NULL just after NULL checking. Or could we use some lock to prevent the simultaneity? Thanks, Jun > memset(e, 0, sizeof(*e)); > e->attr_timeout = lo->timeout; > e->entry_timeout = lo->timeout; >
Re: [Qemu-devel] [Virtio-fs] [PATCH 2/5] virtiofsd: prevent lo_lookup() NULL pointer dereference
On Fri, Jul 26, 2019 at 10:11:00AM +0100, Stefan Hajnoczi wrote: > Most lo_do_lookup() have already checked that the parent inode exists. > lo_lookup() hasn't and can therefore hit a NULL pointer dereference when > lo_inode(req, parent) returns NULL. > Sigh...this one has been fixed by 3 different developers...Me, Pengtao and Stefan. The following one on the ML did the exactly same thing. --- Subject: [Virtio-fs] [PATCH] virtiofsd: fix lo_do_lookup panic It needs to check for invalid parent dir. Signed-off-by: Peng Tao --- thanks, -liubo > Signed-off-by: Stefan Hajnoczi > --- > contrib/virtiofsd/passthrough_ll.c | 4 > 1 file changed, 4 insertions(+) > > diff --git a/contrib/virtiofsd/passthrough_ll.c > b/contrib/virtiofsd/passthrough_ll.c > index 9ae1381618..277a17fc03 100644 > --- a/contrib/virtiofsd/passthrough_ll.c > +++ b/contrib/virtiofsd/passthrough_ll.c > @@ -766,6 +766,10 @@ static int lo_do_lookup(fuse_req_t req, fuse_ino_t > parent, const char *name, > struct lo_data *lo = lo_data(req); > struct lo_inode *inode, *dir = lo_inode(req, parent); > > + if (!dir) { > + return EBADF; > + } > + > memset(e, 0, sizeof(*e)); > e->attr_timeout = lo->timeout; > e->entry_timeout = lo->timeout; > -- > 2.21.0 > > ___ > Virtio-fs mailing list > virtio...@redhat.com > https://www.redhat.com/mailman/listinfo/virtio-fs