| Message ID | 20240916052128.225475-1-danielyangkang@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] fs/exfat: resolve memory leak from exfat_create_upcase_table() | expand |
On Mon, Sep 16, 2024 at 2:21 PM Daniel Yang <danielyangkang@gmail.com> wrote: > > If exfat_load_upcase_table reaches end and returns -EINVAL, > allocated memory doesn't get freed and while > exfat_load_default_upcase_table allocates more memory, leading to a > memory leak. > > Here's link to syzkaller crash report illustrating this issue: > https://syzkaller.appspot.com/text?tag=CrashReport&x=1406c201980000 > > Signed-off-by: Daniel Yang <danielyangkang@gmail.com> > Reported-by: syzbot+e1c69cadec0f1a078e3d@syzkaller.appspotmail.com > --- > V1 -> V2: Moved the mem free to create_upcase_table > > fs/exfat/nls.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/fs/exfat/nls.c b/fs/exfat/nls.c > index afdf13c34..8828f9d29 100644 > --- a/fs/exfat/nls.c > +++ b/fs/exfat/nls.c > @@ -3,6 +3,7 @@ > * Copyright (C) 2012-2013 Samsung Electronics Co., Ltd. > */ > > +#include <cerrno> Why did you add this? > #include <linux/string.h> > #include <linux/slab.h> > #include <linux/buffer_head.h> > @@ -779,8 +780,13 @@ int exfat_create_upcase_table(struct super_block *sb) > le32_to_cpu(ep->dentry.upcase.checksum)); > > brelse(bh); > - if (ret && ret != -EIO) > + if (ret && ret != -EIO) { > + /* free memory from exfat_load_upcase_table call */ > + if (ret == -EINVAL) { Why did you add this check here? If you consider that ->vol_utbl is NULL, this check is unnecessary. Thanks. > + exfat_free_upcase_table(sbi); > + } > goto load_default; > + } > > /* load successfully */ > return ret; > -- > 2.39.2 >
On Mon, Sep 16, 2024 at 02:58:43PM -0700, Daniel Yang wrote: > In exfat_create_upcase_table, ENOMEM and EINVAL result in a jump to > exfat_load_default_upcase_table where memory is also allocated. Since > ENOMEM doesn't allocate memory, freeing null addresses will result in a > double free. Freeing null address is a no-op. Explicitly guaranteed, for the same reason why C standard guarantees that free(NULL) is a no-op. So you don't need to check if allocation had been done.
diff --git a/fs/exfat/nls.c b/fs/exfat/nls.c index afdf13c34..8828f9d29 100644 --- a/fs/exfat/nls.c +++ b/fs/exfat/nls.c @@ -3,6 +3,7 @@ * Copyright (C) 2012-2013 Samsung Electronics Co., Ltd. */ +#include <cerrno> #include <linux/string.h> #include <linux/slab.h> #include <linux/buffer_head.h> @@ -779,8 +780,13 @@ int exfat_create_upcase_table(struct super_block *sb) le32_to_cpu(ep->dentry.upcase.checksum)); brelse(bh); - if (ret && ret != -EIO) + if (ret && ret != -EIO) { + /* free memory from exfat_load_upcase_table call */ + if (ret == -EINVAL) { + exfat_free_upcase_table(sbi); + } goto load_default; + } /* load successfully */ return ret;
If exfat_load_upcase_table reaches end and returns -EINVAL, allocated memory doesn't get freed and while exfat_load_default_upcase_table allocates more memory, leading to a memory leak. Here's link to syzkaller crash report illustrating this issue: https://syzkaller.appspot.com/text?tag=CrashReport&x=1406c201980000 Signed-off-by: Daniel Yang <danielyangkang@gmail.com> Reported-by: syzbot+e1c69cadec0f1a078e3d@syzkaller.appspotmail.com --- V1 -> V2: Moved the mem free to create_upcase_table fs/exfat/nls.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)