[PATCH] fs: quota: fix array-index-out-of-bounds bug by passing correct argument to vfs_cleanup_quota_inode()

[PATCH] fs: quota: fix array-index-out-of-bounds bug by passing correct argument to vfs_cleanup_quota_inode()

[Anant Thazhemadam]

When dquot_resume() was last updated, the argument that got passed
to vfs_cleanup_quota_inode was incorrectly set.

If type = -1 and dquot_load_quota_sb() returns a negative value,
then vfs_cleanup_quota_inode() gets called with -1 passed as an
argument, and this leads to an array-index-out-of-bounds bug.

Fix this issue by correctly passing the arguments.

Fixes: ae45f07d47cc ("quota: Simplify dquot_resume()")
Reported-by: syzbot+2643e825238d7aabb37f@syzkaller.appspotmail.com
Tested-by: syzbot+2643e825238d7aabb37f@syzkaller.appspotmail.com
Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com>
---
If type = -1 is passed as an argument to vfs_cleanup_quota_inode(),
it causes an array-index-out-of-bounds error since dqopt->files[-1]
can be potentially attempted to be accessed.
Before the bisected commit introduced this bug, vfs_load_quota_inode()
was being directly called in dquot_resume(), and subsequently 
vfs_cleanup_quota_inode() was called with the cnt value as argument.

 fs/quota/dquot.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
index bb02989d92b6..4f1373463766 100644
--- a/fs/quota/dquot.c
+++ b/fs/quota/dquot.c
@@ -2455,7 +2455,7 @@ int dquot_resume(struct super_block *sb, int type)
 		ret = dquot_load_quota_sb(sb, cnt, dqopt->info[cnt].dqi_fmt_id,
 					  flags);
 		if (ret < 0)
-			vfs_cleanup_quota_inode(sb, type);
+			vfs_cleanup_quota_inode(sb, cnt);
 	}
 
 	return ret;
-- 
2.25.1

Re: [PATCH] fs: quota: fix array-index-out-of-bounds bug by passing correct argument to vfs_cleanup_quota_inode()

[Jan Kara]

On Wed 09-12-20 01:13:38, Anant Thazhemadam wrote:
> When dquot_resume() was last updated, the argument that got passed
> to vfs_cleanup_quota_inode was incorrectly set.
> 
> If type = -1 and dquot_load_quota_sb() returns a negative value,
> then vfs_cleanup_quota_inode() gets called with -1 passed as an
> argument, and this leads to an array-index-out-of-bounds bug.
> 
> Fix this issue by correctly passing the arguments.
> 
> Fixes: ae45f07d47cc ("quota: Simplify dquot_resume()")
> Reported-by: syzbot+2643e825238d7aabb37f@syzkaller.appspotmail.com
> Tested-by: syzbot+2643e825238d7aabb37f@syzkaller.appspotmail.com
> Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com>

Thanks for the fix! I've just queued the very same fix I wrote yesterday to
my tree. But yours has better changelog so let me pick your patch instead
;)

For next time, how can we avoid collisions like this? Did you work on the fix
based on the syzbot email sent to the list so if I actually reply to the
syzbot email that I'm working on / already have a fix you'd see it?

								Honza

> ---
> If type = -1 is passed as an argument to vfs_cleanup_quota_inode(),
> it causes an array-index-out-of-bounds error since dqopt->files[-1]
> can be potentially attempted to be accessed.
> Before the bisected commit introduced this bug, vfs_load_quota_inode()
> was being directly called in dquot_resume(), and subsequently 
> vfs_cleanup_quota_inode() was called with the cnt value as argument.
> 
>  fs/quota/dquot.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
> index bb02989d92b6..4f1373463766 100644
> --- a/fs/quota/dquot.c
> +++ b/fs/quota/dquot.c
> @@ -2455,7 +2455,7 @@ int dquot_resume(struct super_block *sb, int type)
>  		ret = dquot_load_quota_sb(sb, cnt, dqopt->info[cnt].dqi_fmt_id,
>  					  flags);
>  		if (ret < 0)
> -			vfs_cleanup_quota_inode(sb, type);
> +			vfs_cleanup_quota_inode(sb, cnt);
>  	}
>  
>  	return ret;
> -- 
> 2.25.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] fs: quota: fix array-index-out-of-bounds bug by passing correct argument to vfs_cleanup_quota_inode()

[Anant Thazhemadam]

On 09/12/20 2:37 pm, Jan Kara wrote:
> On Wed 09-12-20 01:13:38, Anant Thazhemadam wrote:
>> When dquot_resume() was last updated, the argument that got passed
>> to vfs_cleanup_quota_inode was incorrectly set.
>>
>> If type = -1 and dquot_load_quota_sb() returns a negative value,
>> then vfs_cleanup_quota_inode() gets called with -1 passed as an
>> argument, and this leads to an array-index-out-of-bounds bug.
>>
>> Fix this issue by correctly passing the arguments.
>>
>> Fixes: ae45f07d47cc ("quota: Simplify dquot_resume()")
>> Reported-by: syzbot+2643e825238d7aabb37f@syzkaller.appspotmail.com
>> Tested-by: syzbot+2643e825238d7aabb37f@syzkaller.appspotmail.com
>> Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com>
> Thanks for the fix! I've just queued the very same fix I wrote yesterday to
> my tree. But yours has better changelog so let me pick your patch instead
> ;)

Glad to hear that. Thank you! :D

> For next time, how can we avoid collisions like this? Did you work on the fix
> based on the syzbot email sent to the list so if I actually reply to the
> syzbot email that I'm working on / already have a fix you'd see it?

I came across the bug on the syzbot dashboard, and not through the mailing list.
But even if I did come across this on the mailing list, there is the still a fair chance
that I could've come across this bug, and started working on it before replied to
the syzbot email, right?
I can't speak for everyone, but even if I see a bug on the mailing list, I go over to
the dashboard, and get the apt .config and reproducer from there, and try to work
on it; almost never checking that initial syzbot mail again.

However, iirc there have been previous discussions regarding this on
the mailing lists (although I'm not sure where I came across them :/ ).
For this reason I've Cc-ed Dmitry onto this reply, and hopefully he'll be able to direct
you to those conversations, and also validate any new ideas you might have.
I'd be more than happy to contribute too if I can add any value to the discussion
around that, and to whatever ideas you may have, since this is a issue that has
been around for quite a while now. :)

Hope this helps,
Anant