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@xxxxxxxx>
SUSE Labs, CR
