Re: [PATCH v4 2/4] fanotify: define struct members to hold response decision context

[Jan Kara]

On Fri 12-08-22 10:23:13, Matthew Bobrowski wrote:
> On Tue, Aug 09, 2022 at 01:22:53PM -0400, Richard Guy Briggs wrote:
> > This patch adds a flag, FAN_INFO and an extensible buffer to provide
> > additional information about response decisions.  The buffer contains
> > one or more headers defining the information type and the length of the
> > following information.  The patch defines one additional information
> > type, FAN_RESPONSE_INFO_AUDIT_RULE, an audit rule number.  This will
> > allow for the creation of other information types in the future if other
> > users of the API identify different needs.
> > 
> > Suggested-by: Steve Grubb <sgrubb@xxxxxxxxxx>
> > Link: https://lore.kernel.org/r/2745105.e9J7NaK4W3@x2
> > Suggested-by: Jan Kara <jack@xxxxxxx>
> > Link: https://lore.kernel.org/r/20201001101219.GE17860@xxxxxxxxxxxxxx
> > Signed-off-by: Richard Guy Briggs <rgb@xxxxxxxxxx>
> > ---

...

> > @@ -827,26 +877,33 @@ static ssize_t fanotify_read(struct file *file, char __user *buf,
> >  
> >  static ssize_t fanotify_write(struct file *file, const char __user *buf, size_t count, loff_t *pos)
> >  {
> > -	struct fanotify_response response = { .fd = -1, .response = -1 };
> > +	struct fanotify_response response;
> >  	struct fsnotify_group *group;
> >  	int ret;
> > +	const char __user *info_buf = buf + sizeof(struct fanotify_response);
> > +	size_t c;
> 
> Can we rename this to something like len or info_len instead? I
> dislike single character variable names outside of the scope of things
> like loops.
> 
> >  	if (!IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS))
> >  		return -EINVAL;
> >  
> >  	group = file->private_data;
> >  
> > -	if (count < sizeof(response))
> > -		return -EINVAL;
> > -
> > -	count = sizeof(response);
> > -
> >  	pr_debug("%s: group=%p count=%zu\n", __func__, group, count);
> >  
> > -	if (copy_from_user(&response, buf, count))
> > +	if (count < sizeof(response))
> > +		return -EINVAL;
> > +	if (copy_from_user(&response, buf, sizeof(response)))
> >  		return -EFAULT;
> >  
> > -	ret = process_access_response(group, &response);
> > +	c = count - sizeof(response);
> > +	if (response.response & FAN_INFO) {
> > +		if (c < sizeof(struct fanotify_response_info_header))
> > +			return -EINVAL;
> > +	} else {
> > +		if (c != 0)
> > +			return -EINVAL;
> 
> Hm, prior to this change we truncated the copy operation to the
> sizeof(struct fanotify_response) and didn't care if there maybe was
> extra data supplied in the buf or count > sizeof(struct
> fanotify_response). This leaves me wondering whether this check is
> needed for cases that are not (FAN_INFO | FAN_AUDIT)? The buf may
> still hold a valid fanotify_response despite buf/count possibly being
> larger than sizeof(struct fanotify_response)... I can see why you'd
> want to enforce this, but I'm wondering if it might break things if
> event listeners are responding to the permission events in an awkward
> way i.e. by calculating and supplying count incorrectly.
> 
> Also, if we do decide to keep this check around, then maybe it can be
> simplified into an else if instead?

So the check for (c != 0) in case FAN_INFO is not set is definitely asking
for userspace regression because before we have been just silently ignoring
additional bytes beyond standard reply. So please keep the old behavior of
ignoring extra bytes if FAN_INFO flag is not set. Thanks!

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