linux-kernel - Re: [PATCH v5 2/3] fanotify: define struct members to hold response decision context

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: <Y8b4GHkrw2spVwUX@madcap2.tricolour.ca>
Date:   Tue, 17 Jan 2023 14:33:44 -0500
From:   Richard Guy Briggs <rgb@...hat.com>
To:     Jan Kara <jack@...e.cz>
Cc:     Linux-Audit Mailing List <linux-audit@...hat.com>,
        LKML <linux-kernel@...r.kernel.org>,
        linux-fsdevel@...r.kernel.org, linux-api@...r.kernel.org,
        Paul Moore <paul@...l-moore.com>,
        Eric Paris <eparis@...isplace.org>,
        Steve Grubb <sgrubb@...hat.com>,
        Amir Goldstein <amir73il@...il.com>
Subject: Re: [PATCH v5 2/3] fanotify: define struct members to hold response
 decision context

On 2023-01-17 09:27, Jan Kara wrote:
> On Mon 16-01-23 15:42:29, Richard Guy Briggs wrote:
> > On 2023-01-03 13:42, Jan Kara wrote:
> > > On Thu 22-12-22 15:47:21, Richard Guy Briggs wrote:
> > > > > > +
> > > > > > +	if (info_len != sizeof(*friar))
> > > > > > +		return -EINVAL;
> > > > > > +
> > > > > > +	if (copy_from_user(friar, info, sizeof(*friar)))
> > > > > > +		return -EFAULT;
> > > > > > +
> > > > > > +	if (friar->hdr.type != FAN_RESPONSE_INFO_AUDIT_RULE)
> > > > > > +		return -EINVAL;
> > > > > > +	if (friar->hdr.pad != 0)
> > > > > > +		return -EINVAL;
> > > > > > +	if (friar->hdr.len != sizeof(*friar))
> > > > > > +		return -EINVAL;
> > > > > > +
> > > > > > +	return info_len;
> > > > > > +}
> > > > > > +
> > > > > 
> > > > > ...
> > > > > 
> > > > > > @@ -327,10 +359,18 @@ static int process_access_response(struct fsnotify_group *group,
> > > > > >  		return -EINVAL;
> > > > > >  	}
> > > > > >  
> > > > > > -	if (fd < 0)
> > > > > > +	if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
> > > > > >  		return -EINVAL;
> > > > > >  
> > > > > > -	if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
> > > > > > +	if (response & FAN_INFO) {
> > > > > > +		ret = process_access_response_info(fd, info, info_len, &friar);
> > > > > > +		if (ret < 0)
> > > > > > +			return ret;
> > > > > > +	} else {
> > > > > > +		ret = 0;
> > > > > > +	}
> > > > > > +
> > > > > > +	if (fd < 0)
> > > > > >  		return -EINVAL;
> > > > > 
> > > > > And here I'd do:
> > > > > 
> > > > > 	if (fd == FAN_NOFD)
> > > > > 		return 0;
> > > > > 	if (fd < 0)
> > > > > 		return -EINVAL;
> > > > > 
> > > > > As we talked in previous revisions we'd specialcase FAN_NOFD to just verify
> > > > > extra info is understood by the kernel so that application writing fanotify
> > > > > responses has a way to check which information it can provide to the
> > > > > kernel.
> > > > 
> > > > The reason for including it in process_access_response_info() is to make
> > > > sure that it is included in the FAN_INFO case to detect this extension.
> > > > If it were included here
> > > 
> > > I see what you're getting at now. So the condition
> > > 
> > >  	if (fd == FAN_NOFD)
> > >  		return 0;
> > > 
> > > needs to be moved into 
> > > 
> > > 	if (response & FAN_INFO)
> > > 
> > > branch after process_access_response_info(). I still prefer to keep it
> > > outside of the process_access_response_info() function itself as it looks
> > > more logical to me. Does it address your concerns?
> > 
> > Ok.  Note that this does not return zero to userspace, since this
> > function's return value is added to the size of the struct
> > fanotify_response when there is no error.
> 
> Right, good point. 0 is not a good return value in this case.
> 
> > For that reason, I think it makes more sense to return -ENOENT, or some
> > other unused error code that fits, unless you think it is acceptable to
> > return sizeof(struct fanotify_response) when FAN_INFO is set to indicate
> > this.
> 
> Yeah, my intention was to indicate "success" to userspace so I'd like to
> return whatever we return for the case when struct fanotify_response is
> accepted for a normal file descriptor - looks like info_len is the right
> value. Thanks!

Ok, I hadn't thought of that.  So, to confirm, when FAN_INFO is set, if
FAN_NOFD is also set, return info_len from process_access_response() and
then immediately return sizeof(struct fanotify_response) plus info_len
to userspace without issuing an audit record should indicate support for
FAN_INFO and the specific info type supplied.

Thanks for helping work through this.

> 								Honza
> -- 
> Jan Kara <jack@...e.com>

- RGB

--
Richard Guy Briggs <rgb@...hat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635