Re: [PATCH v6 3/3] fanotify,audit: Allow audit to use the full permission event response

[Paul Moore]

On Wed, Jan 25, 2023 at 5:06 PM Richard Guy Briggs <rgb@xxxxxxxxxx> wrote:
> On 2023-01-20 13:52, Paul Moore wrote:
> > On Wed, Jan 18, 2023 at 1:34 PM Steve Grubb <sgrubb@xxxxxxxxxx> wrote:
> > > Hello Richard,
> > >
> > > I built a new kernel and tested this with old and new user space. It is
> > > working as advertised. The only thing I'm wondering about is why we have 3F
> > > as the default value when no additional info was sent? Would it be better to
> > > just make it 0?
> >
> > ...
> >
> > > On Tuesday, January 17, 2023 4:14:07 PM EST Richard Guy Briggs wrote:
> > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > > index d1fb821de104..3133c4175c15 100644
> > > > --- a/kernel/auditsc.c
> > > > +++ b/kernel/auditsc.c
> > > > @@ -2877,10 +2878,19 @@ void __audit_log_kern_module(char *name)
> > > >       context->type = AUDIT_KERN_MODULE;
> > > >  }
> > > >
> > > > -void __audit_fanotify(u32 response)
> > > > +void __audit_fanotify(u32 response, struct
> > > > fanotify_response_info_audit_rule *friar) {
> > > > -     audit_log(audit_context(), GFP_KERNEL,
> > > > -             AUDIT_FANOTIFY, "resp=%u", response);
> > > > +     /* {subj,obj}_trust values are {0,1,2}: no,yes,unknown */
> > > > +     if (friar->hdr.type == FAN_RESPONSE_INFO_NONE) {
> > > > +             audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
> > > > +                       "resp=%u fan_type=%u fan_info=3F subj_trust=2
> > > obj_trust=2",
> > > > +                       response, FAN_RESPONSE_INFO_NONE);
> > > > +             return;
> > > > +     }
> >
> > (I'm working under the assumption that the "fan_info=3F" in the record
> > above is what Steve was referring to in his comment.)
> >
> > I vaguely recall Richard commenting on this in the past, although
> > maybe not ... my thought is that the "3F" is simply the hex encoded
> > "?" character in ASCII ('man 7 ascii' is your friend).  I suppose the
> > question is what to do in the FAN_RESPONSE_INFO_NONE case.
> >
> > Historically when we had a missing field we would follow the "field=?"
> > pattern, but I don't recall doing that for a field which was
> > potentially hex encoded, is there an existing case where we use "?"
> > for a field that is hex encoded?  If so, we can swap out the "3F" for
> > a more obvious "?".
>
> I was presuming encoding the zero: "30"

I'm sorry, but you've lost me here.

> > However, another option might be to simply output the current
> > AUDIT_FANOTIFY record format in the FAN_RESPONSE_INFO_NONE case, e.g.
> > only "resp=%u".  This is a little against the usual guidance of
> > "fields should not disappear from a record", but considering that
> > userspace will always need to support the original resp-only format
> > for compatibility reasons this may be an option.
>
> I don't have a strong opinion.

I'm not sure I care too much either.  I will admit that the "3F" seems
to be bordering on the "bit too clever" side of things, but it's easy
to argue it is in keeping with the general idea of using "?" to denote
absent/unknown fields.

As Steve was the one who raised the question in this latest round, and
he knows his userspace tools the best, it seems wise to get his input
on this.

-- 
paul-moore.com