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

From: Jan Kara
Date: Wed Oct 16 2024 - 09:23:57 EST


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@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.

Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR