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] [day] [month] [year] [list]

Message-ID: <20241017095836.ebrfj2323h4sn5xx@quack3>
Date: Thu, 17 Oct 2024 11:58:36 +0200
From: Jan Kara <jack@...e.cz>
To: Alessandro Zanni <alessandro.zanni87@...il.com>
Cc: Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
	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 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@...e.com>
SUSE Labs, CR