linux-kernel - Re: [PATCH] fs: Fix uninitialized value issue in from

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241016-einpacken-ebnen-bcd0924480e1@brauner>
Date: Wed, 16 Oct 2024 16:52:59 +0200
From: Christian Brauner <brauner@...nel.org>
To: Jan Kara <jack@...e.cz>
Cc: Alessandro Zanni <alessandro.zanni87@...il.com>, 
	viro@...iv.linux.org.uk, linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org, 
	skhan@...uxfoundation.org, anupnewsmail@...il.com, alessandrozanni.dev@...il.com, 
	syzbot+6c55f725d1bdc8c52058@...kaller.appspotmail.com
Subject: Re: [PATCH] fs: Fix uninitialized value issue in from_kuid

On Wed, Oct 16, 2024 at 03:23:39PM +0200, Jan Kara wrote:
> On Wed 16-10-24 14:37:19, Alessandro Zanni wrote:
> > Fix uninitialized value issue in from_kuid by initializing the newattrs
> > structure in do_truncate() method.
> 
> Thanks for the fix. It would be helpful to provide a bit more information
> in the changelog so that one doesn't have to open the referenced syzbot
> report to understand the problem. In this case I'd write something like:
> 
> ocfs2_setattr() uses attr->ia_uid in a trace point even though ATTR_UID
> isn't set. Initialize all fields of newattrs to avoid uninitialized
> variable use.
> 
> But see below as I don't think this is really the right fix.

Agreed.

> 
> > Fixes: uninit-value in from_kuid reported here
> >  https://syzkaller.appspot.com/bug?extid=6c55f725d1bdc8c52058
> 
> Fixes tag should reference some preexisting commit this patch is fixing. As
> such this tag is not really applicable here. Keeping the syzbot reference
> in Reported-by and Closes (or possibly change that to References) is good
> enough.
> 
> > Reported-by: syzbot+6c55f725d1bdc8c52058@...kaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=6c55f725d1bdc8c52058
> > Signed-off-by: Alessandro Zanni <alessandro.zanni87@...il.com>
> > ---
> >  fs/open.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/open.c b/fs/open.c
> > index acaeb3e25c88..57c298b1db2c 100644
> > --- a/fs/open.c
> > +++ b/fs/open.c
> > @@ -40,7 +40,7 @@ int do_truncate(struct mnt_idmap *idmap, struct dentry *dentry,
> >  		loff_t length, unsigned int time_attrs, struct file *filp)
> >  {
> >  	int ret;
> > -	struct iattr newattrs;
> > +	struct iattr newattrs = {0};
> 
> We usually perform such initialization as:
> 	struct iattr newattrs = {};
> 
> That being said there are many more places calling notify_change() and none
> of them is doing the initialization so this patch only fixes that one
> particular syzbot reproducer but doesn't really deal with the problem.
> Looking at the bigger picture I think the right solution really is to fix
> ocfs2_setattr() to not touch attr->ia_uid when ATTR_UID isn't set and
> similarly for attr->ia_gid and ATTR_GID.

Yes, that's what we did for similar bugs.