Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 22 Dec 2022 15:47:21 -0500
From:   Richard Guy Briggs <rgb@redhat.com>
To:     Jan Kara <jack@suse.cz>
Cc:     Linux-Audit Mailing List <linux-audit@redhat.com>,
        LKML <linux-kernel@vger.kernel.org>,
        linux-fsdevel@vger.kernel.org, linux-api@vger.kernel.org,
        Paul Moore <paul@paul-moore.com>,
        Eric Paris <eparis@parisplace.org>,
        Steve Grubb <sgrubb@redhat.com>,
        Amir Goldstein <amir73il@gmail.com>
Subject: Re: [PATCH v5 2/3] fanotify: define struct members to hold response
 decision context
Message-ID: <Y6TCWe4/nR957pFh@madcap2.tricolour.ca>
References: <cover.1670606054.git.rgb@redhat.com>
 <45da8423b9b1e8fc7abd68cd2269acff8cf9022a.1670606054.git.rgb@redhat.com>
 <20221216164342.ojcbdifdmafq5njw@quack3>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221216164342.ojcbdifdmafq5njw@quack3>
Precedence: bulk

On 2022-12-16 17:43, Jan Kara wrote:
> On Mon 12-12-22 09:06:10, 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, to audit a 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@redhat.com>
> > Link: https://lore.kernel.org/r/2745105.e9J7NaK4W3@x2
> > Suggested-by: Jan Kara <jack@suse.cz>
> > Link: https://lore.kernel.org/r/20201001101219.GE17860@quack2.suse.cz
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> 
> Thanks for the patches. They look very good to me. Just two nits below. I
> can do the small updates on commit if there would be no other changes. But
> I'd like to get some review from audit guys for patch 3/3 before I commit
> this.

I'd prefer to send a followup patch based on your recommendations rather
than have you modify it.  It does save some back and forth though...

> > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> > index caa1211bac8c..cf3584351e00 100644
> > --- a/fs/notify/fanotify/fanotify_user.c
> > +++ b/fs/notify/fanotify/fanotify_user.c
> > @@ -283,19 +283,44 @@ static int create_fd(struct fsnotify_group *group, const struct path *path,
> >  	return client_fd;
> >  }
> >  
> > +static int process_access_response_info(int fd, const char __user *info, size_t info_len,
> > +					struct fanotify_response_info_audit_rule *friar)
> 
> I prefer to keep lines within 80 columns, unless there is really good
> reason (like with strings) to have them longer.

Sure.  In this case, it buys us little since the last line is lined up
with the arguments openning bracket and it one long struct name unless I
unalign that argument and back up the indent by one.

> BTW, why do you call the info structure 'friar'? I feel some language twist
> escapes me ;)

Fanotify_Response_Info_Audit_Rule, it is a pronounceable word, and
besides they have a long reputation for making good beer.  :-D

> > +{
> > +	if (fd == FAN_NOFD)
> > +		return -ENOENT;
> 
> I would not test 'fd' in this function at all. After all it is not part of
> the response info structure and you do check it in
> process_access_response() anyway.

I wrestled with that.  I was even tempted to swallow the following fd
check too, but the flow would not have made as much sense for the
non-INFO case.

My understanding from Amir was that FAN_NOFD was only to be sent in in
conjuction with FAN_INFO to test if a newer kernel was present.

I presumed that if FAN_NOFD was present without FAN_INFO that was an
invalid input to an old kernel.

> > +
> > +	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

> 								Honza
> 
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR

- RGB

--
Richard Guy Briggs <rgb@redhat.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