Re: Re: [PATCH] fuse: Fix a potential double free in virtio_fs_get_tree
> -原始邮件- > 发件人: "Vivek Goyal" > 发送时间: 2021-03-24 01:10:03 (星期三) > 收件人: "Lv Yunlong" > 抄送: stefa...@redhat.com, mik...@szeredi.hu, > virtualizat...@lists.linux-foundation.org, linux-fsde...@vger.kernel.org, > linux-kernel@vger.kernel.org > 主题: Re: [PATCH] fuse: Fix a potential double free in virtio_fs_get_tree > > On Mon, Mar 22, 2021 at 10:18:31PM -0700, Lv Yunlong wrote: > > In virtio_fs_get_tree, fm is allocated by kzalloc() and > > assigned to fsc->s_fs_info by fsc->s_fs_info=fm statement. > > If the kzalloc() failed, it will goto err directly, so that > > fsc->s_fs_info must be non-NULL and fm will be freed. > > sget_fc() will either consume fsc->s_fs_info in case a new super > block is allocated and set fsc->s_fs_info. In that case we don't > free fc or fm. > > Or, sget_fc() will return with fsc->s_fs_info set in case we already > found a super block. In that case we need to free fc and fm. > > In case of error from sget_fc(), fc/fm need to be freed first and > then error needs to be returned to caller. > > if (IS_ERR(sb)) > return PTR_ERR(sb); > > > If we allocated a new super block in sget_fc(), then next step is > to initialize it. > > if (!sb->s_root) { > err = virtio_fs_fill_super(sb, fsc); > } > > If we run into errors here, then fc/fm need to be freed. > > So current code looks fine to me. > > Vivek > > > > > But later fm is freed again when virtio_fs_fill_super() fialed. > > I think the statement if (fsc->s_fs_info) {kfree(fm);} is > > misplaced. > > > > My patch puts this statement in the correct palce to avoid > > double free. > > > > Signed-off-by: Lv Yunlong > > --- > > fs/fuse/virtio_fs.c | 10 ++ > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c > > index 8868ac31a3c0..727cf436828f 100644 > > --- a/fs/fuse/virtio_fs.c > > +++ b/fs/fuse/virtio_fs.c > > @@ -1437,10 +1437,7 @@ static int virtio_fs_get_tree(struct fs_context *fsc) > > > > fsc->s_fs_info = fm; > > sb = sget_fc(fsc, virtio_fs_test_super, set_anon_super_fc); > > - if (fsc->s_fs_info) { > > - fuse_conn_put(fc); > > - kfree(fm); > > - } > > + > > if (IS_ERR(sb)) > > return PTR_ERR(sb); > > > > @@ -1457,6 +1454,11 @@ static int virtio_fs_get_tree(struct fs_context *fsc) > > sb->s_flags |= SB_ACTIVE; > > } > > > > + if (fsc->s_fs_info) { > > + fuse_conn_put(fc); > > + kfree(fm); > > + } > > + > > WARN_ON(fsc->root); > > fsc->root = dget(sb->s_root); > > return 0; > > -- > > 2.25.1 > > > > > Ok, thanks. It should be a false positive.
Re: [PATCH] fuse: Fix a potential double free in virtio_fs_get_tree
On Mon, Mar 22, 2021 at 10:18:31PM -0700, Lv Yunlong wrote: > In virtio_fs_get_tree, fm is allocated by kzalloc() and > assigned to fsc->s_fs_info by fsc->s_fs_info=fm statement. > If the kzalloc() failed, it will goto err directly, so that > fsc->s_fs_info must be non-NULL and fm will be freed. sget_fc() will either consume fsc->s_fs_info in case a new super block is allocated and set fsc->s_fs_info. In that case we don't free fc or fm. Or, sget_fc() will return with fsc->s_fs_info set in case we already found a super block. In that case we need to free fc and fm. In case of error from sget_fc(), fc/fm need to be freed first and then error needs to be returned to caller. if (IS_ERR(sb)) return PTR_ERR(sb); If we allocated a new super block in sget_fc(), then next step is to initialize it. if (!sb->s_root) { err = virtio_fs_fill_super(sb, fsc); } If we run into errors here, then fc/fm need to be freed. So current code looks fine to me. Vivek > > But later fm is freed again when virtio_fs_fill_super() fialed. > I think the statement if (fsc->s_fs_info) {kfree(fm);} is > misplaced. > > My patch puts this statement in the correct palce to avoid > double free. > > Signed-off-by: Lv Yunlong > --- > fs/fuse/virtio_fs.c | 10 ++ > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c > index 8868ac31a3c0..727cf436828f 100644 > --- a/fs/fuse/virtio_fs.c > +++ b/fs/fuse/virtio_fs.c > @@ -1437,10 +1437,7 @@ static int virtio_fs_get_tree(struct fs_context *fsc) > > fsc->s_fs_info = fm; > sb = sget_fc(fsc, virtio_fs_test_super, set_anon_super_fc); > - if (fsc->s_fs_info) { > - fuse_conn_put(fc); > - kfree(fm); > - } > + > if (IS_ERR(sb)) > return PTR_ERR(sb); > > @@ -1457,6 +1454,11 @@ static int virtio_fs_get_tree(struct fs_context *fsc) > sb->s_flags |= SB_ACTIVE; > } > > + if (fsc->s_fs_info) { > + fuse_conn_put(fc); > + kfree(fm); > + } > + > WARN_ON(fsc->root); > fsc->root = dget(sb->s_root); > return 0; > -- > 2.25.1 > >
Re: [PATCH] fuse: Fix a potential double free in virtio_fs_get_tree
On 3/23/21 12:18 AM, Lv Yunlong wrote: In virtio_fs_get_tree, fm is allocated by kzalloc() and assigned to fsc->s_fs_info by fsc->s_fs_info=fm statement. If the kzalloc() failed, it will goto err directly, so that Right, I follow this so far. fsc->s_fs_info must be non-NULL and fm will be freed. But this I don't follow in the context of the stuff that happens in out_err. But later fm is freed again when virtio_fs_fill_super() fialed. I think the statement if (fsc->s_fs_info) {kfree(fm);} is misplaced. I'm not sure this can double free, because: * If fm = kzalloc[..] fails, the function bails early. * If sget_fc() fails, the function cleans up fm and fc and bails early. * If sget_fc() succeeds and allocated a new superblock, fc->s_fs_info pointer is moved to sb->s_fs_info and fc->s_fs_info is set to NULL, so the first free hasn't happened yet. * If sget_fc() succeeds and somehow returns an existing superblock (which I think is tested by checking if fc->s_fs_info is not NULL, since otherwise it'd have been moved to the superblock and set to NULL in sget_fc), I think sb->s_root would not be NULL, therefore the flow of control wouldn't enter the if-block where virtio_fs_fill_super could fail which means the code won't reach the double free. That's just my reading of it though, and I'm wondering if that makes sense to others :-) One last comment inline: My patch puts this statement in the correct palce to avoid double free. Signed-off-by: Lv Yunlong --- fs/fuse/virtio_fs.c | 10 ++ 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c index 8868ac31a3c0..727cf436828f 100644 --- a/fs/fuse/virtio_fs.c +++ b/fs/fuse/virtio_fs.c @@ -1437,10 +1437,7 @@ static int virtio_fs_get_tree(struct fs_context *fsc) fsc->s_fs_info = fm; sb = sget_fc(fsc, virtio_fs_test_super, set_anon_super_fc); - if (fsc->s_fs_info) { - fuse_conn_put(fc); - kfree(fm); - } + if (IS_ERR(sb)) return PTR_ERR(sb); By removing the check from here, it now looks like if sget_fc() fails, then this early return will leak fm's memory and fc's reference. Connor @@ -1457,6 +1454,11 @@ static int virtio_fs_get_tree(struct fs_context *fsc) sb->s_flags |= SB_ACTIVE; } + if (fsc->s_fs_info) { + fuse_conn_put(fc); + kfree(fm); + } + WARN_ON(fsc->root); fsc->root = dget(sb->s_root); return 0;
[PATCH] fuse: Fix a potential double free in virtio_fs_get_tree
In virtio_fs_get_tree, fm is allocated by kzalloc() and assigned to fsc->s_fs_info by fsc->s_fs_info=fm statement. If the kzalloc() failed, it will goto err directly, so that fsc->s_fs_info must be non-NULL and fm will be freed. But later fm is freed again when virtio_fs_fill_super() fialed. I think the statement if (fsc->s_fs_info) {kfree(fm);} is misplaced. My patch puts this statement in the correct palce to avoid double free. Signed-off-by: Lv Yunlong --- fs/fuse/virtio_fs.c | 10 ++ 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c index 8868ac31a3c0..727cf436828f 100644 --- a/fs/fuse/virtio_fs.c +++ b/fs/fuse/virtio_fs.c @@ -1437,10 +1437,7 @@ static int virtio_fs_get_tree(struct fs_context *fsc) fsc->s_fs_info = fm; sb = sget_fc(fsc, virtio_fs_test_super, set_anon_super_fc); - if (fsc->s_fs_info) { - fuse_conn_put(fc); - kfree(fm); - } + if (IS_ERR(sb)) return PTR_ERR(sb); @@ -1457,6 +1454,11 @@ static int virtio_fs_get_tree(struct fs_context *fsc) sb->s_flags |= SB_ACTIVE; } + if (fsc->s_fs_info) { + fuse_conn_put(fc); + kfree(fm); + } + WARN_ON(fsc->root); fsc->root = dget(sb->s_root); return 0; -- 2.25.1