fs: Fix uninitialized value issue in from_kuid

[PATCH] fs: Fix uninitialized value issue in from_kuid

Posted by Alessandro Zanni 1 year, 3 months ago

Fix uninitialized value issue in from_kuid by initializing the newattrs
structure in do_truncate() method.

Fixes: uninit-value in from_kuid reported here
 https://syzkaller.appspot.com/bug?extid=6c55f725d1bdc8c52058
Reported-by: syzbot+6c55f725d1bdc8c52058@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=6c55f725d1bdc8c52058
Signed-off-by: Alessandro Zanni <alessandro.zanni87@gmail.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};
 
 	/* Not pretty: "inode->i_size" shouldn't really be signed. But it is. */
 	if (length < 0)
-- 
2.43.0

Re: [PATCH] fs: Fix uninitialized value issue in from_kuid

Posted by Jan Kara 1 year, 3 months ago

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.

> 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@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=6c55f725d1bdc8c52058
> Signed-off-by: Alessandro Zanni <alessandro.zanni87@gmail.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.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] fs: Fix uninitialized value issue in from_kuid

Posted by Christian Brauner 1 year, 3 months ago

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@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=6c55f725d1bdc8c52058
> > Signed-off-by: Alessandro Zanni <alessandro.zanni87@gmail.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.

Re: [PATCH] fs: Fix uninitialized value issue in from_kuid

Posted by Alessandro Zanni 1 year, 3 months ago

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@syzkaller.appspotmail.com
> > > Closes: https://syzkaller.appspot.com/bug?extid=6c55f725d1bdc8c52058
> > > Signed-off-by: Alessandro Zanni <alessandro.zanni87@gmail.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.

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

Re: [PATCH] fs: Fix uninitialized value issue in from_kuid

Posted by Jan Kara 1 year, 3 months ago

On Thu 17-10-24 11:22:17, Alessandro Zanni wrote:
> On 24/10/16 04:52, Christian Brauner wrote:
> > On Wed, Oct 16, 2024 at 03:23:39PM +0200, Jan Kara wrote:
> > > 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?

I think the easiest fix is like:

        trace_ocfs2_setattr(inode, dentry,
                            (unsigned long long)OCFS2_I(inode)->ip_blkno,
                            dentry->d_name.len, dentry->d_name.name,
-                           attr->ia_valid, attr->ia_mode,
-                           from_kuid(&init_user_ns, attr->ia_uid),
-                           from_kgid(&init_user_ns, attr->ia_gid));
+                           attr->ia_valid,
+			    attr->ia_valid & ATTR_MODE ? attr->ia_mode : 0,
+                           attr->ia_valid & ATTR_UID ? from_kuid(&init_user_ns, attr->ia_uid) : 0,
+                           attr->ia_valid & ATTR_GID ? from_kgid(&init_user_ns, attr->ia_gid) : 0);

Bonus points for fixing up overly long lines I have in my proposal :)

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR