| Message ID | 20210406235332.2206460-1-pakki001@umn.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fuse: Avoid potential use after free | expand |
On Tue, Apr 06, 2021 at 06:53:32PM -0500, Aditya Pakki wrote: > In virtio_fs_get_tree, after fm is freed, it is again freed in case > s_root is NULL and virtio_fs_fill_super() returns an error. To avoid > a double free, set fm to NULL. > > Signed-off-by: Aditya Pakki <pakki001@umn.edu> > --- > fs/fuse/virtio_fs.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c > index 4ee6f734ba83..a7484c1539bf 100644 > --- a/fs/fuse/virtio_fs.c > +++ b/fs/fuse/virtio_fs.c > @@ -1447,6 +1447,7 @@ static int virtio_fs_get_tree(struct fs_context *fsc) > if (fsc->s_fs_info) { > fuse_conn_put(fc); > kfree(fm); > + fm = NULL; I think both the code paths are mutually exclusive and that's why we don't double free it. sget_fc(), can either return existing super block which is already initialized, or it can create a new super block which need to initialize further. If if get an existing super block, in that case fs->s_fs_info will still be set and we need to free fm (as we did not use it). But in that case this super block is already initialized so sb->s_root should be non-null and we will not call virtio_fs_fill_super() on this. And hence we will not get into kfree(fm) again. Same applies to fuse_conn_put(fc) call as well. So I think this patch is not needed. I think sget_fc() semantics are not obvious and that confuses the reader of the code. Thanks Vivek > } > if (IS_ERR(sb)) > return PTR_ERR(sb); > -- > 2.25.1 >
On Wed, 7 Apr 2021 at 23:25, Vivek Goyal <vgoyal@redhat.com> wrote: > > On Tue, Apr 06, 2021 at 06:53:32PM -0500, Aditya Pakki wrote: > > In virtio_fs_get_tree, after fm is freed, it is again freed in case > > s_root is NULL and virtio_fs_fill_super() returns an error. To avoid > > a double free, set fm to NULL. > > > > Signed-off-by: Aditya Pakki <pakki001@umn.edu> > > --- > > fs/fuse/virtio_fs.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c > > index 4ee6f734ba83..a7484c1539bf 100644 > > --- a/fs/fuse/virtio_fs.c > > +++ b/fs/fuse/virtio_fs.c > > @@ -1447,6 +1447,7 @@ static int virtio_fs_get_tree(struct fs_context *fsc) > > if (fsc->s_fs_info) { > > fuse_conn_put(fc); > > kfree(fm); > > + fm = NULL; > > I think both the code paths are mutually exclusive and that's why we > don't double free it. > > sget_fc(), can either return existing super block which is already > initialized, or it can create a new super block which need to > initialize further. > > If if get an existing super block, in that case fs->s_fs_info will > still be set and we need to free fm (as we did not use it). But in > that case this super block is already initialized so sb->s_root > should be non-null and we will not call virtio_fs_fill_super() > on this. And hence we will not get into kfree(fm) again. > > Same applies to fuse_conn_put(fc) call as well. > > So I think this patch is not needed. I think sget_fc() semantics are > not obvious and that confuses the reader of the code. This patch might be harmful, might be not. Probably should be skipped due to uncertain intentions: https://lore.kernel.org/linux-nfs/YH+7ZydHv4+Y1hlx@kroah.com/ Best regards, Krzysztof
On Tue, Apr 06, 2021 at 06:53:32PM -0500, Aditya Pakki wrote: > In virtio_fs_get_tree, after fm is freed, it is again freed in case > s_root is NULL and virtio_fs_fill_super() returns an error. To avoid > a double free, set fm to NULL. > > Signed-off-by: Aditya Pakki <pakki001@umn.edu> > --- > fs/fuse/virtio_fs.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c > index 4ee6f734ba83..a7484c1539bf 100644 > --- a/fs/fuse/virtio_fs.c > +++ b/fs/fuse/virtio_fs.c > @@ -1447,6 +1447,7 @@ static int virtio_fs_get_tree(struct fs_context *fsc) > if (fsc->s_fs_info) { > fuse_conn_put(fc); > kfree(fm); > + fm = NULL; > } > if (IS_ERR(sb)) > return PTR_ERR(sb); NAK. The only cases when sget_fc() returns without having ->s_fs_info zeroed are when it has successfull grabbed a reference to existing live superblock or when it has failed. In the former case we proceed straight to fsc->root = dget(sb->s_root); return 0; and in the latter we bugger off on IS_ERR(sb). No double-free in either case. Said that, the logics in there (especially around the cleanups on virtio_fs_fill_super() failures) is bloody convoluted, but sorting that out would take a lot more RTFS than I'm willing to start right now. In any case, this patch does not fix any bugs and does not make the thing easier to follow, so... NAKed-by: Al Viro <viro@zeniv.linux.org.uk>
diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c index 4ee6f734ba83..a7484c1539bf 100644 --- a/fs/fuse/virtio_fs.c +++ b/fs/fuse/virtio_fs.c @@ -1447,6 +1447,7 @@ static int virtio_fs_get_tree(struct fs_context *fsc) if (fsc->s_fs_info) { fuse_conn_put(fc); kfree(fm); + fm = NULL; } if (IS_ERR(sb)) return PTR_ERR(sb);
In virtio_fs_get_tree, after fm is freed, it is again freed in case s_root is NULL and virtio_fs_fill_super() returns an error. To avoid a double free, set fm to NULL. Signed-off-by: Aditya Pakki <pakki001@umn.edu> --- fs/fuse/virtio_fs.c | 1 + 1 file changed, 1 insertion(+)