From: Vivek Goyal <vgoyal@redhat.com> To: Aditya Pakki <pakki001@umn.edu> Cc: Stefan Hajnoczi <stefanha@redhat.com>, Miklos Szeredi <miklos@szeredi.hu>, virtualization@lists.linux-foundation.org, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] fuse: Avoid potential use after free Date: Wed, 7 Apr 2021 11:50:31 -0400 [thread overview] Message-ID: <20210407155031.GA1014852@redhat.com> (raw) In-Reply-To: <20210406235332.2206460-1-pakki001@umn.edu> 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 >
WARNING: multiple messages have this Message-ID (diff)
From: Vivek Goyal <vgoyal@redhat.com> To: Aditya Pakki <pakki001@umn.edu> Cc: linux-fsdevel@vger.kernel.org, virtualization@lists.linux-foundation.org, linux-kernel@vger.kernel.org, Stefan Hajnoczi <stefanha@redhat.com>, Miklos Szeredi <miklos@szeredi.hu> Subject: Re: [PATCH] fuse: Avoid potential use after free Date: Wed, 7 Apr 2021 11:50:31 -0400 [thread overview] Message-ID: <20210407155031.GA1014852@redhat.com> (raw) In-Reply-To: <20210406235332.2206460-1-pakki001@umn.edu> 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 > _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
next prev parent reply other threads:[~2021-04-07 15:51 UTC|newest] Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-04-06 23:53 [PATCH] fuse: Avoid potential use after free Aditya Pakki 2021-04-07 15:50 ` Vivek Goyal [this message] 2021-04-07 15:50 ` Vivek Goyal 2021-04-21 13:28 ` Krzysztof Kozlowski 2021-04-21 13:28 ` Krzysztof Kozlowski 2021-04-22 0:44 ` Al Viro 2021-04-22 0:44 ` Al Viro
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20210407155031.GA1014852@redhat.com \ --to=vgoyal@redhat.com \ --cc=linux-fsdevel@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=miklos@szeredi.hu \ --cc=pakki001@umn.edu \ --cc=stefanha@redhat.com \ --cc=virtualization@lists.linux-foundation.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.