Re: [PATCH] fs: Fix uninitialized value issue in from_kuid
From: Alessandro Zanni
Date: Thu Oct 17 2024 - 05:23:02 EST
On 24/10/16 04:52, Christian Brauner wrote:
> 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.
Ok.
> >
> > > 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.
Ok.
> > > Reported-by: syzbot+6c55f725d1bdc8c52058@xxxxxxxxxxxxxxxxxxxxxxxxx
> > > Closes: https://syzkaller.appspot.com/bug?extid=6c55f725d1bdc8c52058
> > > Signed-off-by: Alessandro Zanni <alessandro.zanni87@xxxxxxxxx>
> > > ---
> > > 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.
Thanks for the valuable comments.
I digged more into the code. I think the two possible fixes are:
i) return 0 from ocfs2_setattr() if ATTR_UID/ATTR_GID are not set
ii) enter in trace_ocfs2_setattr() only if ATTR_UID/ATTR_GID are set
What do you think?
Thanks,
Alessandro