Re: [PATCH v4 3/4] fanotify,audit: Allow audit to use the full permission event response
On 2022-09-09 10:55, Steve Grubb wrote: > On Friday, September 9, 2022 10:38:46 AM EDT Richard Guy Briggs wrote: > > > Richard, add subj_trust and obj_trust. These can be 0|1|2 for no, yes, > > > unknown. > > > > type? bitfield? My gut would say that "0" should be "unset"/"unknown", > > but that is counterintuitive to the values represented. > > > > Or "trust" with sub-fields "subj" and "obj"? > > No. just make them separate and u32. subj_trust and obj_trust - no sub > fields. > If we have sub-fields, that probably means bit mapping and that wasn't wanted. Ack. > -Steve - RGB -- Richard Guy Briggs 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 -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v4 3/4] fanotify, audit: Allow audit to use the full permission event response
On Friday, September 9, 2022 10:38:46 AM EDT Richard Guy Briggs wrote: > > Richard, add subj_trust and obj_trust. These can be 0|1|2 for no, yes, > > unknown. > > type? bitfield? My gut would say that "0" should be "unset"/"unknown", > but that is counterintuitive to the values represented. > > Or "trust" with sub-fields "subj" and "obj"? No. just make them separate and u32. subj_trust and obj_trust - no sub fields. If we have sub-fields, that probably means bit mapping and that wasn't wanted. -Steve -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v4 3/4] fanotify,audit: Allow audit to use the full permission event response
On 2022-09-09 10:22, Steve Grubb wrote:
> On Friday, September 9, 2022 7:09:44 AM EDT Jan Kara wrote:
> > Hello Steve!
> >
> > On Fri 09-09-22 00:03:53, Steve Grubb wrote:
> > > On Thursday, September 8, 2022 10:41:44 PM EDT Richard Guy Briggs wrote:
> > > > > I'm trying to abide by what was suggested by the fs-devel folks. I
> > > > > can
> > > > > live with it. But if you want to make something non-generic for all
> > > > > users of fanotify, call the new field "trusted". This would decern
> > > > > when
> > > > > a decision was made because the file was untrusted or access denied
> > > > > for
> > > > > another reason.
> > > >
> > > > So, "u32 trusted;" ? How would you like that formatted?
> > > > "fan_trust={0|1}"
> > >
> > > So how does this play out if there is another user? Do they want a num=
> > > and trust= if not, then the AUDIT_FANOTIFY record will have multiple
> > > formats which is not good. I'd rather suggest something generic that can
> > > be interpreted based on who's attached to fanotify. IOW we have a
> > > fan_type=0 and then followed by info0= info1= the interpretation of
> > > those solely depend on fan_type. If the fan_type does not need both,
> > > then any interpretation skips what it doesn't need. If fan_type=1, then
> > > it follows what arg0= and arg1= is for that format. But make this pivot
> > > on fan_type and not actual names.
> > So I think there is some misunderstanding so let me maybe spell out in
> > detail how I see things so that we can get on the same page:
> >
> > It was a requirement from me (and probably Amir) that there is a generic
> > way to attach additional info to a response to fanotify permission event.
> > This is achieved by defining:
> >
> > struct fanotify_response_info_header {
> >__u8 type;
> >__u8 pad;
> >__u16 len;
> > };
> >
> > which is a generic header and kernel can based on 'len' field decide how
> > large the response structure is (to safely copy it from userspace) and
> > based on 'type' field it can decide who should be the recipient of this
> > extra information (or generally what to do with it). So any additional
> > info needs to start with this header.
> >
> > Then there is:
> >
> > struct fanotify_response_info_audit_rule {
> >struct fanotify_response_info_header hdr;
> >__u32 audit_rule;
> > };
> >
> > which properly starts with the header and hdr.type is expected to be
> > FAN_RESPONSE_INFO_AUDIT_RULE. What happens after the header with type
> > FAN_RESPONSE_INFO_AUDIT_RULE until length hdr.len is fully within *audit*
> > subsystem's responsibility. Fanotify code will just pass this as an opaque
> > blob to the audit subsystem.
> >
> > So if you know audit subsystem will also need some other field together
> > with 'audit_rule' now is a good time to add it and it doesn't have to be
> > useful for anybody else besides audit. If someone else will need other
> > information passed along with the response, he will append structure with
> > another header with different 'type' field. In principle, there can be
> > multiple structures appended to fanotify response like
> >
> > ...
> >
> > and fanotify subsystem will just pass them to different receivers based
> > on the type in 'hdr' field.
> >
> > Also if audit needs to pass even more information along with the respose,
> > we can define a new 'type' for it. But the 'type' space is not infinite so
> > I'd prefer this does not happen too often...
> >
> > I hope this clears out things a bit.
>
> Yes. Thank you.
>
> Richard, add subj_trust and obj_trust. These can be 0|1|2 for no, yes,
> unknown.
type? bitfield? My gut would say that "0" should be "unset"/"unknown",
but that is counterintuitive to the values represented.
Or "trust" with sub-fields "subj" and "obj"?
> -Steve
- RGB
--
Richard Guy Briggs
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
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v4 3/4] fanotify, audit: Allow audit to use the full permission event response
On Friday, September 9, 2022 7:09:44 AM EDT Jan Kara wrote:
> Hello Steve!
>
> On Fri 09-09-22 00:03:53, Steve Grubb wrote:
> > On Thursday, September 8, 2022 10:41:44 PM EDT Richard Guy Briggs wrote:
> > > > I'm trying to abide by what was suggested by the fs-devel folks. I
> > > > can
> > > > live with it. But if you want to make something non-generic for all
> > > > users of fanotify, call the new field "trusted". This would decern
> > > > when
> > > > a decision was made because the file was untrusted or access denied
> > > > for
> > > > another reason.
> > >
> > > So, "u32 trusted;" ? How would you like that formatted?
> > > "fan_trust={0|1}"
> >
> > So how does this play out if there is another user? Do they want a num=
> > and trust= if not, then the AUDIT_FANOTIFY record will have multiple
> > formats which is not good. I'd rather suggest something generic that can
> > be interpreted based on who's attached to fanotify. IOW we have a
> > fan_type=0 and then followed by info0= info1= the interpretation of
> > those solely depend on fan_type. If the fan_type does not need both,
> > then any interpretation skips what it doesn't need. If fan_type=1, then
> > it follows what arg0= and arg1= is for that format. But make this pivot
> > on fan_type and not actual names.
> So I think there is some misunderstanding so let me maybe spell out in
> detail how I see things so that we can get on the same page:
>
> It was a requirement from me (and probably Amir) that there is a generic
> way to attach additional info to a response to fanotify permission event.
> This is achieved by defining:
>
> struct fanotify_response_info_header {
>__u8 type;
>__u8 pad;
>__u16 len;
> };
>
> which is a generic header and kernel can based on 'len' field decide how
> large the response structure is (to safely copy it from userspace) and
> based on 'type' field it can decide who should be the recipient of this
> extra information (or generally what to do with it). So any additional
> info needs to start with this header.
>
> Then there is:
>
> struct fanotify_response_info_audit_rule {
>struct fanotify_response_info_header hdr;
>__u32 audit_rule;
> };
>
> which properly starts with the header and hdr.type is expected to be
> FAN_RESPONSE_INFO_AUDIT_RULE. What happens after the header with type
> FAN_RESPONSE_INFO_AUDIT_RULE until length hdr.len is fully within *audit*
> subsystem's responsibility. Fanotify code will just pass this as an opaque
> blob to the audit subsystem.
>
> So if you know audit subsystem will also need some other field together
> with 'audit_rule' now is a good time to add it and it doesn't have to be
> useful for anybody else besides audit. If someone else will need other
> information passed along with the response, he will append structure with
> another header with different 'type' field. In principle, there can be
> multiple structures appended to fanotify response like
>
> ...
>
> and fanotify subsystem will just pass them to different receivers based
> on the type in 'hdr' field.
>
> Also if audit needs to pass even more information along with the respose,
> we can define a new 'type' for it. But the 'type' space is not infinite so
> I'd prefer this does not happen too often...
>
> I hope this clears out things a bit.
Yes. Thank you.
Richard, add subj_trust and obj_trust. These can be 0|1|2 for no, yes,
unknown.
-Steve
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v4 3/4] fanotify,audit: Allow audit to use the full permission event response
Hello Steve!
On Fri 09-09-22 00:03:53, Steve Grubb wrote:
> On Thursday, September 8, 2022 10:41:44 PM EDT Richard Guy Briggs wrote:
> > > I'm trying to abide by what was suggested by the fs-devel folks. I can
> > > live with it. But if you want to make something non-generic for all
> > > users of fanotify, call the new field "trusted". This would decern when
> > > a decision was made because the file was untrusted or access denied for
> > > another reason.
> >
> > So, "u32 trusted;" ? How would you like that formatted?
> > "fan_trust={0|1}"
>
> So how does this play out if there is another user? Do they want a num= and
> trust= if not, then the AUDIT_FANOTIFY record will have multiple formats
> which is not good. I'd rather suggest something generic that can be
> interpreted based on who's attached to fanotify. IOW we have a fan_type=0 and
> then followed by info0= info1= the interpretation of those solely depend on
> fan_type. If the fan_type does not need both, then any interpretation skips
> what it doesn't need. If fan_type=1, then it follows what arg0= and arg1= is
> for that format. But make this pivot on fan_type and not actual names.
So I think there is some misunderstanding so let me maybe spell out in
detail how I see things so that we can get on the same page:
It was a requirement from me (and probably Amir) that there is a generic
way to attach additional info to a response to fanotify permission event.
This is achieved by defining:
struct fanotify_response_info_header {
__u8 type;
__u8 pad;
__u16 len;
};
which is a generic header and kernel can based on 'len' field decide how
large the response structure is (to safely copy it from userspace) and
based on 'type' field it can decide who should be the recipient of this
extra information (or generally what to do with it). So any additional
info needs to start with this header.
Then there is:
struct fanotify_response_info_audit_rule {
struct fanotify_response_info_header hdr;
__u32 audit_rule;
};
which properly starts with the header and hdr.type is expected to be
FAN_RESPONSE_INFO_AUDIT_RULE. What happens after the header with type
FAN_RESPONSE_INFO_AUDIT_RULE until length hdr.len is fully within *audit*
subsystem's responsibility. Fanotify code will just pass this as an opaque
blob to the audit subsystem.
So if you know audit subsystem will also need some other field together
with 'audit_rule' now is a good time to add it and it doesn't have to be
useful for anybody else besides audit. If someone else will need other
information passed along with the response, he will append structure with
another header with different 'type' field. In principle, there can be
multiple structures appended to fanotify response like
...
and fanotify subsystem will just pass them to different receivers based
on the type in 'hdr' field.
Also if audit needs to pass even more information along with the respose,
we can define a new 'type' for it. But the 'type' space is not infinite so
I'd prefer this does not happen too often...
I hope this clears out things a bit.
Honza
--
Jan Kara
SUSE Labs, CR
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v4 3/4] fanotify, audit: Allow audit to use the full permission event response
On Thursday, September 8, 2022 10:41:44 PM EDT Richard Guy Briggs wrote:
> > I'm trying to abide by what was suggested by the fs-devel folks. I can
> > live with it. But if you want to make something non-generic for all
> > users of fanotify, call the new field "trusted". This would decern when
> > a decision was made because the file was untrusted or access denied for
> > another reason.
>
> So, "u32 trusted;" ? How would you like that formatted?
> "fan_trust={0|1}"
So how does this play out if there is another user? Do they want a num= and
trust= if not, then the AUDIT_FANOTIFY record will have multiple formats
which is not good. I'd rather suggest something generic that can be
interpreted based on who's attached to fanotify. IOW we have a fan_type=0 and
then followed by info0= info1= the interpretation of those solely depend on
fan_type. If the fan_type does not need both, then any interpretation skips
what it doesn't need. If fan_type=1, then it follows what arg0= and arg1= is
for that format. But make this pivot on fan_type and not actual names.
> > > You mention that you know what you want to put in the struct, why not
> > > share the details with all of us so we are all on the same page and
> > > can have a proper discussion.
> >
> > Because I want to abide by the original agreement and not impose
> > opinionated requirements that serve no one else. I'd rather have
> > something anyone can use. I want to play nice.
>
> If someone else wants to use something, why not give them a type of
> their own other than FAN_RESPONSE_INFO_AUDIT_RULE that they can shape
> however they like?
Please, let's keep AUDIT_FANOTIFY normalized but pivot on fan_type.
-Steve
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v4 3/4] fanotify,audit: Allow audit to use the full permission event response
On Thu, Sep 8, 2022 at 10:41 PM Richard Guy Briggs wrote:
> On 2022-09-08 22:20, Steve Grubb wrote:
> > On Thursday, September 8, 2022 5:22:15 PM EDT Paul Moore wrote:
> > > On Thu, Sep 8, 2022 at 5:14 PM Steve Grubb wrote:
> > > > On Wednesday, September 7, 2022 4:23:49 PM EDT Paul Moore wrote:
> > > > > On Wed, Sep 7, 2022 at 4:11 PM Steve Grubb wrote:
> > > > > > On Wednesday, September 7, 2022 2:43:54 PM EDT Richard Guy Briggs
> > wrote:
> > > > > > > > > Ultimately I guess I'll leave it upto audit subsystem what it
> > > > > > > > > wants
> > > > > > > > > to
> > > > > > > > > have in its struct fanotify_response_info_audit_rule because
> > > > > > > > > for
> > > > > > > > > fanotify subsystem, it is just an opaque blob it is passing.
> > > > > > > >
> > > > > > > > In that case, let's stick with leveraging the type/len fields in
> > > > > > > > the
> > > > > > > > fanotify_response_info_header struct, that should give us all
> > > > > > > > the
> > > > > > > > flexibility we need.
> > > > > > > >
> > > > > > > > Richard and Steve, it sounds like Steve is already aware of
> > > > > > > > additional
> > > > > > > > information that he wants to send via the
> > > > > > > > fanotify_response_info_audit_rule struct, please include that in
> > > > > > > > the
> > > > > > > > next revision of this patchset. I don't want to get this merged
> > > > > > > > and
> > > > > > > > then soon after have to hack in additional info.
> > > > > > >
> > > > > > > Steve, please define the type and name of this additional field.
> > > > > >
> > > > > > Maybe extra_data, app_data, or extra_info. Something generic that
> > > > > > can
> > > > > > be
> > > > > > reused by any application. Default to 0 if not present.
> > > > >
> > > > > I think the point is being missed ... The idea is to not speculate on
> > > > > additional fields, as discussed we have ways to handle that, the issue
> > > > > was that Steve implied that he already had ideas for "things" he
> > > > > wanted to add. If there are "things" that need to be added, let's do
> > > > > that now, however if there is just speculation that maybe someday we
> > > > > might need to add something else we can leave that until later.
> > > >
> > > > This is not speculation. I know what I want to put there. I know you
> > > > want
> > > > to pin it down to exactly what it is. However, when this started a
> > > > couple years back, one of the concerns was that we're building something
> > > > specific to 1 user of fanotify. And that it would be better for all
> > > > future users to have a generic facility that everyone could use if they
> > > > wanted to. That's why I'm suggesting something generic, its so this is
> > > > not special purpose that doesn't fit any other use case.
> > >
> > > Well, we are talking specifically about fanotify in this thread and
> > > dealing with data structures that are specific to fanotify. I can
> > > understand wanting to future proof things, but based on what we've
> > > seen in this thread I think we are all set in this regard.
> >
> > I'm trying to abide by what was suggested by the fs-devel folks. I can live
> > with it. But if you want to make something non-generic for all users of
> > fanotify, call the new field "trusted". This would decern when a decision
> > was
> > made because the file was untrusted or access denied for another reason.
>
> So, "u32 trusted;" ? How would you like that formatted?
> "fan_trust={0|1}"
>
> > > You mention that you know what you want to put in the struct, why not
> > > share the details with all of us so we are all on the same page and
> > > can have a proper discussion.
> >
> > Because I want to abide by the original agreement and not impose opinionated
> > requirements that serve no one else. I'd rather have something anyone can
> > use. I want to play nice.
>
> If someone else wants to use something, why not give them a type of
> their own other than FAN_RESPONSE_INFO_AUDIT_RULE that they can shape
> however they like?
Yes, exactly. The struct is very clearly specific to both fanotify
and audit, I see no reason why it needs to be made generic for use by
other subsystems when other mechanisms exist to support them.
--
paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v4 3/4] fanotify,audit: Allow audit to use the full permission event response
On 2022-09-08 22:20, Steve Grubb wrote:
> On Thursday, September 8, 2022 5:22:15 PM EDT Paul Moore wrote:
> > On Thu, Sep 8, 2022 at 5:14 PM Steve Grubb wrote:
> > > On Wednesday, September 7, 2022 4:23:49 PM EDT Paul Moore wrote:
> > > > On Wed, Sep 7, 2022 at 4:11 PM Steve Grubb wrote:
> > > > > On Wednesday, September 7, 2022 2:43:54 PM EDT Richard Guy Briggs
> wrote:
> > > > > > > > Ultimately I guess I'll leave it upto audit subsystem what it
> > > > > > > > wants
> > > > > > > > to
> > > > > > > > have in its struct fanotify_response_info_audit_rule because
> > > > > > > > for
> > > > > > > > fanotify subsystem, it is just an opaque blob it is passing.
> > > > > > >
> > > > > > > In that case, let's stick with leveraging the type/len fields in
> > > > > > > the
> > > > > > > fanotify_response_info_header struct, that should give us all the
> > > > > > > flexibility we need.
> > > > > > >
> > > > > > > Richard and Steve, it sounds like Steve is already aware of
> > > > > > > additional
> > > > > > > information that he wants to send via the
> > > > > > > fanotify_response_info_audit_rule struct, please include that in
> > > > > > > the
> > > > > > > next revision of this patchset. I don't want to get this merged
> > > > > > > and
> > > > > > > then soon after have to hack in additional info.
> > > > > >
> > > > > > Steve, please define the type and name of this additional field.
> > > > >
> > > > > Maybe extra_data, app_data, or extra_info. Something generic that can
> > > > > be
> > > > > reused by any application. Default to 0 if not present.
> > > >
> > > > I think the point is being missed ... The idea is to not speculate on
> > > > additional fields, as discussed we have ways to handle that, the issue
> > > > was that Steve implied that he already had ideas for "things" he
> > > > wanted to add. If there are "things" that need to be added, let's do
> > > > that now, however if there is just speculation that maybe someday we
> > > > might need to add something else we can leave that until later.
> > >
> > > This is not speculation. I know what I want to put there. I know you want
> > > to pin it down to exactly what it is. However, when this started a
> > > couple years back, one of the concerns was that we're building something
> > > specific to 1 user of fanotify. And that it would be better for all
> > > future users to have a generic facility that everyone could use if they
> > > wanted to. That's why I'm suggesting something generic, its so this is
> > > not special purpose that doesn't fit any other use case.
> >
> > Well, we are talking specifically about fanotify in this thread and
> > dealing with data structures that are specific to fanotify. I can
> > understand wanting to future proof things, but based on what we've
> > seen in this thread I think we are all set in this regard.
>
> I'm trying to abide by what was suggested by the fs-devel folks. I can live
> with it. But if you want to make something non-generic for all users of
> fanotify, call the new field "trusted". This would decern when a decision was
> made because the file was untrusted or access denied for another reason.
So, "u32 trusted;" ? How would you like that formatted?
"fan_trust={0|1}"
> > You mention that you know what you want to put in the struct, why not
> > share the details with all of us so we are all on the same page and
> > can have a proper discussion.
>
> Because I want to abide by the original agreement and not impose opinionated
> requirements that serve no one else. I'd rather have something anyone can
> use. I want to play nice.
If someone else wants to use something, why not give them a type of
their own other than FAN_RESPONSE_INFO_AUDIT_RULE that they can shape
however they like?
> -Steve
- RGB
--
Richard Guy Briggs
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
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v4 3/4] fanotify, audit: Allow audit to use the full permission event response
On Thursday, September 8, 2022 5:22:15 PM EDT Paul Moore wrote: > On Thu, Sep 8, 2022 at 5:14 PM Steve Grubb wrote: > > On Wednesday, September 7, 2022 4:23:49 PM EDT Paul Moore wrote: > > > On Wed, Sep 7, 2022 at 4:11 PM Steve Grubb wrote: > > > > On Wednesday, September 7, 2022 2:43:54 PM EDT Richard Guy Briggs wrote: > > > > > > > Ultimately I guess I'll leave it upto audit subsystem what it > > > > > > > wants > > > > > > > to > > > > > > > have in its struct fanotify_response_info_audit_rule because > > > > > > > for > > > > > > > fanotify subsystem, it is just an opaque blob it is passing. > > > > > > > > > > > > In that case, let's stick with leveraging the type/len fields in > > > > > > the > > > > > > fanotify_response_info_header struct, that should give us all the > > > > > > flexibility we need. > > > > > > > > > > > > Richard and Steve, it sounds like Steve is already aware of > > > > > > additional > > > > > > information that he wants to send via the > > > > > > fanotify_response_info_audit_rule struct, please include that in > > > > > > the > > > > > > next revision of this patchset. I don't want to get this merged > > > > > > and > > > > > > then soon after have to hack in additional info. > > > > > > > > > > Steve, please define the type and name of this additional field. > > > > > > > > Maybe extra_data, app_data, or extra_info. Something generic that can > > > > be > > > > reused by any application. Default to 0 if not present. > > > > > > I think the point is being missed ... The idea is to not speculate on > > > additional fields, as discussed we have ways to handle that, the issue > > > was that Steve implied that he already had ideas for "things" he > > > wanted to add. If there are "things" that need to be added, let's do > > > that now, however if there is just speculation that maybe someday we > > > might need to add something else we can leave that until later. > > > > This is not speculation. I know what I want to put there. I know you want > > to pin it down to exactly what it is. However, when this started a > > couple years back, one of the concerns was that we're building something > > specific to 1 user of fanotify. And that it would be better for all > > future users to have a generic facility that everyone could use if they > > wanted to. That's why I'm suggesting something generic, its so this is > > not special purpose that doesn't fit any other use case. > > Well, we are talking specifically about fanotify in this thread and > dealing with data structures that are specific to fanotify. I can > understand wanting to future proof things, but based on what we've > seen in this thread I think we are all set in this regard. I'm trying to abide by what was suggested by the fs-devel folks. I can live with it. But if you want to make something non-generic for all users of fanotify, call the new field "trusted". This would decern when a decision was made because the file was untrusted or access denied for another reason. > You mention that you know what you want to put in the struct, why not > share the details with all of us so we are all on the same page and > can have a proper discussion. Because I want to abide by the original agreement and not impose opinionated requirements that serve no one else. I'd rather have something anyone can use. I want to play nice. -Steve -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v4 3/4] fanotify,audit: Allow audit to use the full permission event response
On Thu, Sep 8, 2022 at 5:14 PM Steve Grubb wrote: > On Wednesday, September 7, 2022 4:23:49 PM EDT Paul Moore wrote: > > On Wed, Sep 7, 2022 at 4:11 PM Steve Grubb wrote: > > > On Wednesday, September 7, 2022 2:43:54 PM EDT Richard Guy Briggs wrote: > > > > > > Ultimately I guess I'll leave it upto audit subsystem what it wants > > > > > > to > > > > > > have in its struct fanotify_response_info_audit_rule because for > > > > > > fanotify subsystem, it is just an opaque blob it is passing. > > > > > > > > > > In that case, let's stick with leveraging the type/len fields in the > > > > > fanotify_response_info_header struct, that should give us all the > > > > > flexibility we need. > > > > > > > > > > Richard and Steve, it sounds like Steve is already aware of > > > > > additional > > > > > information that he wants to send via the > > > > > fanotify_response_info_audit_rule struct, please include that in the > > > > > next revision of this patchset. I don't want to get this merged and > > > > > then soon after have to hack in additional info. > > > > > > > > Steve, please define the type and name of this additional field. > > > > > > Maybe extra_data, app_data, or extra_info. Something generic that can be > > > reused by any application. Default to 0 if not present. > > > > I think the point is being missed ... The idea is to not speculate on > > additional fields, as discussed we have ways to handle that, the issue > > was that Steve implied that he already had ideas for "things" he > > wanted to add. If there are "things" that need to be added, let's do > > that now, however if there is just speculation that maybe someday we > > might need to add something else we can leave that until later. > > This is not speculation. I know what I want to put there. I know you want to > pin it down to exactly what it is. However, when this started a couple years > back, one of the concerns was that we're building something specific to 1 user > of fanotify. And that it would be better for all future users to have a > generic facility that everyone could use if they wanted to. That's why I'm > suggesting something generic, its so this is not special purpose that doesn't > fit any other use case. Well, we are talking specifically about fanotify in this thread and dealing with data structures that are specific to fanotify. I can understand wanting to future proof things, but based on what we've seen in this thread I think we are all set in this regard. You mention that you know what you want to put in the struct, why not share the details with all of us so we are all on the same page and can have a proper discussion. -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v4 3/4] fanotify, audit: Allow audit to use the full permission event response
On Wednesday, September 7, 2022 4:23:49 PM EDT Paul Moore wrote: > On Wed, Sep 7, 2022 at 4:11 PM Steve Grubb wrote: > > On Wednesday, September 7, 2022 2:43:54 PM EDT Richard Guy Briggs wrote: > > > > > Ultimately I guess I'll leave it upto audit subsystem what it wants > > > > > to > > > > > have in its struct fanotify_response_info_audit_rule because for > > > > > fanotify subsystem, it is just an opaque blob it is passing. > > > > > > > > In that case, let's stick with leveraging the type/len fields in the > > > > fanotify_response_info_header struct, that should give us all the > > > > flexibility we need. > > > > > > > > Richard and Steve, it sounds like Steve is already aware of > > > > additional > > > > information that he wants to send via the > > > > fanotify_response_info_audit_rule struct, please include that in the > > > > next revision of this patchset. I don't want to get this merged and > > > > then soon after have to hack in additional info. > > > > > > Steve, please define the type and name of this additional field. > > > > Maybe extra_data, app_data, or extra_info. Something generic that can be > > reused by any application. Default to 0 if not present. > > I think the point is being missed ... The idea is to not speculate on > additional fields, as discussed we have ways to handle that, the issue > was that Steve implied that he already had ideas for "things" he > wanted to add. If there are "things" that need to be added, let's do > that now, however if there is just speculation that maybe someday we > might need to add something else we can leave that until later. This is not speculation. I know what I want to put there. I know you want to pin it down to exactly what it is. However, when this started a couple years back, one of the concerns was that we're building something specific to 1 user of fanotify. And that it would be better for all future users to have a generic facility that everyone could use if they wanted to. That's why I'm suggesting something generic, its so this is not special purpose that doesn't fit any other use case. -Steve -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v4 3/4] fanotify,audit: Allow audit to use the full permission event response
On Wed, Sep 7, 2022 at 4:11 PM Steve Grubb wrote: > On Wednesday, September 7, 2022 2:43:54 PM EDT Richard Guy Briggs wrote: > > > > Ultimately I guess I'll leave it upto audit subsystem what it wants to > > > > have in its struct fanotify_response_info_audit_rule because for > > > > fanotify subsystem, it is just an opaque blob it is passing. > > > > > > In that case, let's stick with leveraging the type/len fields in the > > > fanotify_response_info_header struct, that should give us all the > > > flexibility we need. > > > > > > Richard and Steve, it sounds like Steve is already aware of additional > > > information that he wants to send via the > > > fanotify_response_info_audit_rule struct, please include that in the > > > next revision of this patchset. I don't want to get this merged and > > > then soon after have to hack in additional info. > > > > Steve, please define the type and name of this additional field. > > Maybe extra_data, app_data, or extra_info. Something generic that can be > reused by any application. Default to 0 if not present. I think the point is being missed ... The idea is to not speculate on additional fields, as discussed we have ways to handle that, the issue was that Steve implied that he already had ideas for "things" he wanted to add. If there are "things" that need to be added, let's do that now, however if there is just speculation that maybe someday we might need to add something else we can leave that until later. -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v4 3/4] fanotify, audit: Allow audit to use the full permission event response
On Wednesday, September 7, 2022 2:43:54 PM EDT Richard Guy Briggs wrote: > > > Ultimately I guess I'll leave it upto audit subsystem what it wants to > > > have in its struct fanotify_response_info_audit_rule because for > > > fanotify subsystem, it is just an opaque blob it is passing. > > > > In that case, let's stick with leveraging the type/len fields in the > > fanotify_response_info_header struct, that should give us all the > > flexibility we need. > > > > Richard and Steve, it sounds like Steve is already aware of additional > > information that he wants to send via the > > fanotify_response_info_audit_rule struct, please include that in the > > next revision of this patchset. I don't want to get this merged and > > then soon after have to hack in additional info. > > Steve, please define the type and name of this additional field. Maybe extra_data, app_data, or extra_info. Something generic that can be reused by any application. Default to 0 if not present. Thanks, -Steve -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v4 3/4] fanotify,audit: Allow audit to use the full permission event response
On 2022-09-01 14:31, Paul Moore wrote:
> On Thu, Sep 1, 2022 at 3:52 AM Jan Kara wrote:
> > On Wed 31-08-22 21:47:09, Paul Moore wrote:
> > > On Wed, Aug 31, 2022 at 7:55 PM Steve Grubb wrote:
> > > > On Wednesday, August 31, 2022 6:19:40 PM EDT Richard Guy Briggs wrote:
> > > > > On 2022-08-31 17:25, Steve Grubb wrote:
> > > > > > On Wednesday, August 31, 2022 5:07:25 PM EDT Richard Guy Briggs
> > > > > > wrote:
> > > > > > > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > > > > > > > index 433418d73584..f000fec52360 100644
> > > > > > > > > --- a/kernel/auditsc.c
> > > > > > > > > +++ b/kernel/auditsc.c
> > > > > > > > > @@ -64,6 +64,7 @@
> > > > > > > > > #include
> > > > > > > > > #include
> > > > > > > > > #include // struct open_how
> > > > > > > > > +#include
> > > > > > > > >
> > > > > > > > > #include "audit.h"
> > > > > > > > >
> > > > > > > > > @@ -2899,10 +2900,34 @@ void __audit_log_kern_module(char
> > > > > > > > > *name)
> > > > > > > > > context->type = AUDIT_KERN_MODULE;
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > > -void __audit_fanotify(u32 response)
> > > > > > > > > +void __audit_fanotify(u32 response, size_t len, char *buf)
> > > > > > > > > {
> > > > > > > > > - audit_log(audit_context(), GFP_KERNEL,
> > > > > > > > > - AUDIT_FANOTIFY, "resp=%u", response);
> > > > > > > > > + struct fanotify_response_info_audit_rule *friar;
> > > > > > > > > + size_t c = len;
> > > > > > > > > + char *ib = buf;
> > > > > > > > > +
> > > > > > > > > + if (!(len && buf)) {
> > > > > > > > > + audit_log(audit_context(), GFP_KERNEL,
> > > > > > > > > AUDIT_FANOTIFY,
> > > > > > > > > + "resp=%u fan_type=0 fan_info=?",
> > > > > > > > > response);
> > > > > > > > > + return;
> > > > > > > > > + }
> > > > > > > > > + while (c >= sizeof(struct
> > > > > > > > > fanotify_response_info_header)) {
> > > > > > > > > + friar = (struct
> > > > > > > > > fanotify_response_info_audit_rule
> > > > > > > > > *)buf;
> > > > > > > >
> > > > > > > > Since the only use of this at the moment is the
> > > > > > > > fanotify_response_info_rule, why not pass the
> > > > > > > > fanotify_response_info_rule struct directly into this function?
> > > > > > > > We
> > > > > > > > can always change it if we need to in the future without
> > > > > > > > affecting
> > > > > > > > userspace, and it would simplify the code.
> > > > > > >
> > > > > > > Steve, would it make any sense for there to be more than one
> > > > > > > FAN_RESPONSE_INFO_AUDIT_RULE header in a message? Could there be
> > > > > > > more
> > > > > > > than one rule that contributes to a notify reason? If not, would
> > > > > > > it be
> > > > > > > reasonable to return -EINVAL if there is more than one?
> > > > > >
> > > > > > I don't see a reason for sending more than one header. What is more
> > > > > > probable is the need to send additional data in that header. I was
> > > > > > thinking of maybe bit mapping it in the rule number. But I'd suggest
> > > > > > padding the struct just in case it needs expanding some day.
> > > > >
> > > > > This doesn't exactly answer my question about multiple rules
> > > > > contributing to one decision.
> > > >
> > > > I don't forsee that.
> > > >
> > > > > The need for more as yet undefined information sounds like a good
> > > > > reason
> > > > > to define a new header if that happens.
> > > >
> > > > It's much better to pad the struct so that the size doesn't change.
> > > >
> > > > > At this point, is it reasonable to throw an error if more than one
> > > > > RULE
> > > > > header appears in a message?
> > > >
> > > > It is a write syscall. I'd silently discard everything else and
> > > > document that
> > > > in the man pages. But the fanotify maintainers should really weigh in on
> > > > this.
> > > >
> > > > > The way I had coded this last patchset was to allow for more than one
> > > > > RULE
> > > > > header and each one would get its own record in the event.
> > > >
> > > > I do not forsee a need for this.
> > > >
> > > > > How many rules total are likely to exist?
> > > >
> > > > Could be a thousand. But I already know some missing information we'd
> > > > like to
> > > > return to user space in an audit event, so the bit mapping on the rule
> > > > number
> > > > might happen. I'd suggest padding one u32 for future use.
> > >
> > > A better way to handle an expansion like that would be to have a
> > > length/version field at the top of the struct that could be used to
> > > determine the size and layout of the struct.
> >
> > We already do have the 'type' and 'len' fields in
> > struct fanotify_response_info_header. So if audit needs to pass more
> > information, we can define a new 'type' and either make it replace the
> > current struct fanotify_response_info_audit_rule or make it expand the
> > information in it. At least this is how we handle
Re: [PATCH v4 3/4] fanotify,audit: Allow audit to use the full permission event response
On Thu, Sep 1, 2022 at 3:52 AM Jan Kara wrote:
> On Wed 31-08-22 21:47:09, Paul Moore wrote:
> > On Wed, Aug 31, 2022 at 7:55 PM Steve Grubb wrote:
> > > On Wednesday, August 31, 2022 6:19:40 PM EDT Richard Guy Briggs wrote:
> > > > On 2022-08-31 17:25, Steve Grubb wrote:
> > > > > On Wednesday, August 31, 2022 5:07:25 PM EDT Richard Guy Briggs wrote:
> > > > > > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > > > > > > index 433418d73584..f000fec52360 100644
> > > > > > > > --- a/kernel/auditsc.c
> > > > > > > > +++ b/kernel/auditsc.c
> > > > > > > > @@ -64,6 +64,7 @@
> > > > > > > > #include
> > > > > > > > #include
> > > > > > > > #include // struct open_how
> > > > > > > > +#include
> > > > > > > >
> > > > > > > > #include "audit.h"
> > > > > > > >
> > > > > > > > @@ -2899,10 +2900,34 @@ void __audit_log_kern_module(char *name)
> > > > > > > > context->type = AUDIT_KERN_MODULE;
> > > > > > > > }
> > > > > > > >
> > > > > > > > -void __audit_fanotify(u32 response)
> > > > > > > > +void __audit_fanotify(u32 response, size_t len, char *buf)
> > > > > > > > {
> > > > > > > > - audit_log(audit_context(), GFP_KERNEL,
> > > > > > > > - AUDIT_FANOTIFY, "resp=%u", response);
> > > > > > > > + struct fanotify_response_info_audit_rule *friar;
> > > > > > > > + size_t c = len;
> > > > > > > > + char *ib = buf;
> > > > > > > > +
> > > > > > > > + if (!(len && buf)) {
> > > > > > > > + audit_log(audit_context(), GFP_KERNEL,
> > > > > > > > AUDIT_FANOTIFY,
> > > > > > > > + "resp=%u fan_type=0 fan_info=?",
> > > > > > > > response);
> > > > > > > > + return;
> > > > > > > > + }
> > > > > > > > + while (c >= sizeof(struct
> > > > > > > > fanotify_response_info_header)) {
> > > > > > > > + friar = (struct
> > > > > > > > fanotify_response_info_audit_rule
> > > > > > > > *)buf;
> > > > > > >
> > > > > > > Since the only use of this at the moment is the
> > > > > > > fanotify_response_info_rule, why not pass the
> > > > > > > fanotify_response_info_rule struct directly into this function?
> > > > > > > We
> > > > > > > can always change it if we need to in the future without affecting
> > > > > > > userspace, and it would simplify the code.
> > > > > >
> > > > > > Steve, would it make any sense for there to be more than one
> > > > > > FAN_RESPONSE_INFO_AUDIT_RULE header in a message? Could there be
> > > > > > more
> > > > > > than one rule that contributes to a notify reason? If not, would
> > > > > > it be
> > > > > > reasonable to return -EINVAL if there is more than one?
> > > > >
> > > > > I don't see a reason for sending more than one header. What is more
> > > > > probable is the need to send additional data in that header. I was
> > > > > thinking of maybe bit mapping it in the rule number. But I'd suggest
> > > > > padding the struct just in case it needs expanding some day.
> > > >
> > > > This doesn't exactly answer my question about multiple rules
> > > > contributing to one decision.
> > >
> > > I don't forsee that.
> > >
> > > > The need for more as yet undefined information sounds like a good reason
> > > > to define a new header if that happens.
> > >
> > > It's much better to pad the struct so that the size doesn't change.
> > >
> > > > At this point, is it reasonable to throw an error if more than one RULE
> > > > header appears in a message?
> > >
> > > It is a write syscall. I'd silently discard everything else and document
> > > that
> > > in the man pages. But the fanotify maintainers should really weigh in on
> > > this.
> > >
> > > > The way I had coded this last patchset was to allow for more than one
> > > > RULE
> > > > header and each one would get its own record in the event.
> > >
> > > I do not forsee a need for this.
> > >
> > > > How many rules total are likely to exist?
> > >
> > > Could be a thousand. But I already know some missing information we'd
> > > like to
> > > return to user space in an audit event, so the bit mapping on the rule
> > > number
> > > might happen. I'd suggest padding one u32 for future use.
> >
> > A better way to handle an expansion like that would be to have a
> > length/version field at the top of the struct that could be used to
> > determine the size and layout of the struct.
>
> We already do have the 'type' and 'len' fields in
> struct fanotify_response_info_header. So if audit needs to pass more
> information, we can define a new 'type' and either make it replace the
> current struct fanotify_response_info_audit_rule or make it expand the
> information in it. At least this is how we handle similar situation when
> fanotify wants to report some new bits of information to userspace.
Perfect, I didn't know that was an option from the fanotify side; I
agree that's the right approach.
> That being said if audit wants to have u32 pad in its struct
> fanotify_response_info_audit_rule for future "optional"
Re: [PATCH v4 3/4] fanotify,audit: Allow audit to use the full permission event response
On Wed 31-08-22 21:47:09, Paul Moore wrote:
> On Wed, Aug 31, 2022 at 7:55 PM Steve Grubb wrote:
> > On Wednesday, August 31, 2022 6:19:40 PM EDT Richard Guy Briggs wrote:
> > > On 2022-08-31 17:25, Steve Grubb wrote:
> > > > On Wednesday, August 31, 2022 5:07:25 PM EDT Richard Guy Briggs wrote:
> > > > > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > > > > > index 433418d73584..f000fec52360 100644
> > > > > > > --- a/kernel/auditsc.c
> > > > > > > +++ b/kernel/auditsc.c
> > > > > > > @@ -64,6 +64,7 @@
> > > > > > > #include
> > > > > > > #include
> > > > > > > #include // struct open_how
> > > > > > > +#include
> > > > > > >
> > > > > > > #include "audit.h"
> > > > > > >
> > > > > > > @@ -2899,10 +2900,34 @@ void __audit_log_kern_module(char *name)
> > > > > > > context->type = AUDIT_KERN_MODULE;
> > > > > > > }
> > > > > > >
> > > > > > > -void __audit_fanotify(u32 response)
> > > > > > > +void __audit_fanotify(u32 response, size_t len, char *buf)
> > > > > > > {
> > > > > > > - audit_log(audit_context(), GFP_KERNEL,
> > > > > > > - AUDIT_FANOTIFY, "resp=%u", response);
> > > > > > > + struct fanotify_response_info_audit_rule *friar;
> > > > > > > + size_t c = len;
> > > > > > > + char *ib = buf;
> > > > > > > +
> > > > > > > + if (!(len && buf)) {
> > > > > > > + audit_log(audit_context(), GFP_KERNEL,
> > > > > > > AUDIT_FANOTIFY,
> > > > > > > + "resp=%u fan_type=0 fan_info=?",
> > > > > > > response);
> > > > > > > + return;
> > > > > > > + }
> > > > > > > + while (c >= sizeof(struct fanotify_response_info_header))
> > > > > > > {
> > > > > > > + friar = (struct fanotify_response_info_audit_rule
> > > > > > > *)buf;
> > > > > >
> > > > > > Since the only use of this at the moment is the
> > > > > > fanotify_response_info_rule, why not pass the
> > > > > > fanotify_response_info_rule struct directly into this function? We
> > > > > > can always change it if we need to in the future without affecting
> > > > > > userspace, and it would simplify the code.
> > > > >
> > > > > Steve, would it make any sense for there to be more than one
> > > > > FAN_RESPONSE_INFO_AUDIT_RULE header in a message? Could there be more
> > > > > than one rule that contributes to a notify reason? If not, would it
> > > > > be
> > > > > reasonable to return -EINVAL if there is more than one?
> > > >
> > > > I don't see a reason for sending more than one header. What is more
> > > > probable is the need to send additional data in that header. I was
> > > > thinking of maybe bit mapping it in the rule number. But I'd suggest
> > > > padding the struct just in case it needs expanding some day.
> > >
> > > This doesn't exactly answer my question about multiple rules
> > > contributing to one decision.
> >
> > I don't forsee that.
> >
> > > The need for more as yet undefined information sounds like a good reason
> > > to define a new header if that happens.
> >
> > It's much better to pad the struct so that the size doesn't change.
> >
> > > At this point, is it reasonable to throw an error if more than one RULE
> > > header appears in a message?
> >
> > It is a write syscall. I'd silently discard everything else and document
> > that
> > in the man pages. But the fanotify maintainers should really weigh in on
> > this.
> >
> > > The way I had coded this last patchset was to allow for more than one RULE
> > > header and each one would get its own record in the event.
> >
> > I do not forsee a need for this.
> >
> > > How many rules total are likely to exist?
> >
> > Could be a thousand. But I already know some missing information we'd like
> > to
> > return to user space in an audit event, so the bit mapping on the rule
> > number
> > might happen. I'd suggest padding one u32 for future use.
>
> A better way to handle an expansion like that would be to have a
> length/version field at the top of the struct that could be used to
> determine the size and layout of the struct.
We already do have the 'type' and 'len' fields in
struct fanotify_response_info_header. So if audit needs to pass more
information, we can define a new 'type' and either make it replace the
current struct fanotify_response_info_audit_rule or make it expand the
information in it. At least this is how we handle similar situation when
fanotify wants to report some new bits of information to userspace.
That being said if audit wants to have u32 pad in its struct
fanotify_response_info_audit_rule for future "optional" expansion I'm not
strictly opposed to that but I don't think it is a good idea. It is tricky
to safely start using the new field. Audit subsystem can define that the
kernel currently just ignores the field. And new kernel could start using
the passed information in the additional field but that is somewhat risky
because until that moment userspace can be passing random garbage in that
unused field and thus
Re: [PATCH v4 3/4] fanotify,audit: Allow audit to use the full permission event response
On Wed, Aug 31, 2022 at 7:55 PM Steve Grubb wrote:
> On Wednesday, August 31, 2022 6:19:40 PM EDT Richard Guy Briggs wrote:
> > On 2022-08-31 17:25, Steve Grubb wrote:
> > > On Wednesday, August 31, 2022 5:07:25 PM EDT Richard Guy Briggs wrote:
> > > > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > > > > index 433418d73584..f000fec52360 100644
> > > > > > --- a/kernel/auditsc.c
> > > > > > +++ b/kernel/auditsc.c
> > > > > > @@ -64,6 +64,7 @@
> > > > > > #include
> > > > > > #include
> > > > > > #include // struct open_how
> > > > > > +#include
> > > > > >
> > > > > > #include "audit.h"
> > > > > >
> > > > > > @@ -2899,10 +2900,34 @@ void __audit_log_kern_module(char *name)
> > > > > > context->type = AUDIT_KERN_MODULE;
> > > > > > }
> > > > > >
> > > > > > -void __audit_fanotify(u32 response)
> > > > > > +void __audit_fanotify(u32 response, size_t len, char *buf)
> > > > > > {
> > > > > > - audit_log(audit_context(), GFP_KERNEL,
> > > > > > - AUDIT_FANOTIFY, "resp=%u", response);
> > > > > > + struct fanotify_response_info_audit_rule *friar;
> > > > > > + size_t c = len;
> > > > > > + char *ib = buf;
> > > > > > +
> > > > > > + if (!(len && buf)) {
> > > > > > + audit_log(audit_context(), GFP_KERNEL,
> > > > > > AUDIT_FANOTIFY,
> > > > > > + "resp=%u fan_type=0 fan_info=?",
> > > > > > response);
> > > > > > + return;
> > > > > > + }
> > > > > > + while (c >= sizeof(struct fanotify_response_info_header)) {
> > > > > > + friar = (struct fanotify_response_info_audit_rule
> > > > > > *)buf;
> > > > >
> > > > > Since the only use of this at the moment is the
> > > > > fanotify_response_info_rule, why not pass the
> > > > > fanotify_response_info_rule struct directly into this function? We
> > > > > can always change it if we need to in the future without affecting
> > > > > userspace, and it would simplify the code.
> > > >
> > > > Steve, would it make any sense for there to be more than one
> > > > FAN_RESPONSE_INFO_AUDIT_RULE header in a message? Could there be more
> > > > than one rule that contributes to a notify reason? If not, would it be
> > > > reasonable to return -EINVAL if there is more than one?
> > >
> > > I don't see a reason for sending more than one header. What is more
> > > probable is the need to send additional data in that header. I was
> > > thinking of maybe bit mapping it in the rule number. But I'd suggest
> > > padding the struct just in case it needs expanding some day.
> >
> > This doesn't exactly answer my question about multiple rules
> > contributing to one decision.
>
> I don't forsee that.
>
> > The need for more as yet undefined information sounds like a good reason
> > to define a new header if that happens.
>
> It's much better to pad the struct so that the size doesn't change.
>
> > At this point, is it reasonable to throw an error if more than one RULE
> > header appears in a message?
>
> It is a write syscall. I'd silently discard everything else and document that
> in the man pages. But the fanotify maintainers should really weigh in on
> this.
>
> > The way I had coded this last patchset was to allow for more than one RULE
> > header and each one would get its own record in the event.
>
> I do not forsee a need for this.
>
> > How many rules total are likely to exist?
>
> Could be a thousand. But I already know some missing information we'd like to
> return to user space in an audit event, so the bit mapping on the rule number
> might happen. I'd suggest padding one u32 for future use.
A better way to handle an expansion like that would be to have a
length/version field at the top of the struct that could be used to
determine the size and layout of the struct.
However, to be clear, my original suggestion of passing the
fanotify_response_info_rule struct internally didn't require any
additional future proofing as it is an internal implementation detail
and not something that is exposed to userspace; the function arguments
could be changed in the future and not break userspace. I'm not quite
sure how we ended up on this topic ...
--
paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v4 3/4] fanotify, audit: Allow audit to use the full permission event response
On Wednesday, August 31, 2022 6:19:40 PM EDT Richard Guy Briggs wrote:
> On 2022-08-31 17:25, Steve Grubb wrote:
> > On Wednesday, August 31, 2022 5:07:25 PM EDT Richard Guy Briggs wrote:
> > > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > > > index 433418d73584..f000fec52360 100644
> > > > > --- a/kernel/auditsc.c
> > > > > +++ b/kernel/auditsc.c
> > > > > @@ -64,6 +64,7 @@
> > > > > #include
> > > > > #include
> > > > > #include // struct open_how
> > > > > +#include
> > > > >
> > > > > #include "audit.h"
> > > > >
> > > > > @@ -2899,10 +2900,34 @@ void __audit_log_kern_module(char *name)
> > > > > context->type = AUDIT_KERN_MODULE;
> > > > > }
> > > > >
> > > > > -void __audit_fanotify(u32 response)
> > > > > +void __audit_fanotify(u32 response, size_t len, char *buf)
> > > > > {
> > > > > - audit_log(audit_context(), GFP_KERNEL,
> > > > > - AUDIT_FANOTIFY, "resp=%u", response);
> > > > > + struct fanotify_response_info_audit_rule *friar;
> > > > > + size_t c = len;
> > > > > + char *ib = buf;
> > > > > +
> > > > > + if (!(len && buf)) {
> > > > > + audit_log(audit_context(), GFP_KERNEL,
> > > > > AUDIT_FANOTIFY,
> > > > > + "resp=%u fan_type=0 fan_info=?",
> > > > > response);
> > > > > + return;
> > > > > + }
> > > > > + while (c >= sizeof(struct fanotify_response_info_header)) {
> > > > > + friar = (struct fanotify_response_info_audit_rule
> > > > > *)buf;
> > > >
> > > > Since the only use of this at the moment is the
> > > > fanotify_response_info_rule, why not pass the
> > > > fanotify_response_info_rule struct directly into this function? We
> > > > can always change it if we need to in the future without affecting
> > > > userspace, and it would simplify the code.
> > >
> > > Steve, would it make any sense for there to be more than one
> > > FAN_RESPONSE_INFO_AUDIT_RULE header in a message? Could there be more
> > > than one rule that contributes to a notify reason? If not, would it be
> > > reasonable to return -EINVAL if there is more than one?
> >
> > I don't see a reason for sending more than one header. What is more
> > probable is the need to send additional data in that header. I was
> > thinking of maybe bit mapping it in the rule number. But I'd suggest
> > padding the struct just in case it needs expanding some day.
>
> This doesn't exactly answer my question about multiple rules
> contributing to one decision.
I don't forsee that.
> The need for more as yet undefined information sounds like a good reason
> to define a new header if that happens.
It's much better to pad the struct so that the size doesn't change.
> At this point, is it reasonable to throw an error if more than one RULE
> header appears in a message?
It is a write syscall. I'd silently discard everything else and document that
in the man pages. But the fanotify maintainers should really weigh in on
this.
> The way I had coded this last patchset was to allow for more than one RULE
> header and each one would get its own record in the event.
I do not forsee a need for this.
> How many rules total are likely to exist?
Could be a thousand. But I already know some missing information we'd like to
return to user space in an audit event, so the bit mapping on the rule number
might happen. I'd suggest padding one u32 for future use.
-Steve
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v4 3/4] fanotify,audit: Allow audit to use the full permission event response
On 2022-08-31 17:25, Steve Grubb wrote:
> On Wednesday, August 31, 2022 5:07:25 PM EDT Richard Guy Briggs wrote:
> > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > > index 433418d73584..f000fec52360 100644
> > > > --- a/kernel/auditsc.c
> > > > +++ b/kernel/auditsc.c
> > > > @@ -64,6 +64,7 @@
> > > > #include
> > > > #include
> > > > #include // struct open_how
> > > > +#include
> > > >
> > > > #include "audit.h"
> > > >
> > > > @@ -2899,10 +2900,34 @@ void __audit_log_kern_module(char *name)
> > > > context->type = AUDIT_KERN_MODULE;
> > > > }
> > > >
> > > > -void __audit_fanotify(u32 response)
> > > > +void __audit_fanotify(u32 response, size_t len, char *buf)
> > > > {
> > > > - audit_log(audit_context(), GFP_KERNEL,
> > > > - AUDIT_FANOTIFY, "resp=%u", response);
> > > > + struct fanotify_response_info_audit_rule *friar;
> > > > + size_t c = len;
> > > > + char *ib = buf;
> > > > +
> > > > + if (!(len && buf)) {
> > > > + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
> > > > + "resp=%u fan_type=0 fan_info=?", response);
> > > > + return;
> > > > + }
> > > > + while (c >= sizeof(struct fanotify_response_info_header)) {
> > > > + friar = (struct fanotify_response_info_audit_rule
> > > > *)buf;
> > >
> > > Since the only use of this at the moment is the
> > > fanotify_response_info_rule, why not pass the
> > > fanotify_response_info_rule struct directly into this function? We
> > > can always change it if we need to in the future without affecting
> > > userspace, and it would simplify the code.
> >
> > Steve, would it make any sense for there to be more than one
> > FAN_RESPONSE_INFO_AUDIT_RULE header in a message? Could there be more
> > than one rule that contributes to a notify reason? If not, would it be
> > reasonable to return -EINVAL if there is more than one?
>
> I don't see a reason for sending more than one header. What is more probable
> is the need to send additional data in that header. I was thinking of maybe
> bit mapping it in the rule number. But I'd suggest padding the struct just in
> case it needs expanding some day.
This doesn't exactly answer my question about multiple rules
contributing to one decision.
The need for more as yet undefined information sounds like a good reason
to define a new header if that happens.
At this point, is it reasonable to throw an error if more than one RULE
header appears in a message? The way I had coded this last patchset was
to allow for more than one RULE header and each one would get its own
record in the event.
How many rules total are likely to exist?
> -Steev
- RGB
--
Richard Guy Briggs
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
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v4 3/4] fanotify,audit: Allow audit to use the full permission event response
On 2022-08-15 20:22, Paul Moore wrote:
> On Tue, Aug 9, 2022 at 1:23 PM Richard Guy Briggs wrote:
> >
> > This patch passes the full value so that the audit function can use all
> > of it. The audit function was updated to log the additional information in
> > the AUDIT_FANOTIFY record. The following is an example of the new record
> > format:
> >
> > type=FANOTIFY msg=audit(1600385147.372:590): resp=2 fan_type=1 fan_info=17
> >
> > Suggested-by: Steve Grubb
> > Link: https://lore.kernel.org/r/3075502.aeNJFYEL58@x2
> > Signed-off-by: Richard Guy Briggs
> > ---
> > fs/notify/fanotify/fanotify.c | 3 ++-
> > include/linux/audit.h | 9 +
> > kernel/auditsc.c | 31 ---
> > 3 files changed, 35 insertions(+), 8 deletions(-)
>
> You've hopefully already seen the kernel test robot build warning, so
> I won't bring that up again, but a few comments below ...
Yes, dealt with...
...
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index 433418d73584..f000fec52360 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -64,6 +64,7 @@
> > #include
> > #include
> > #include // struct open_how
> > +#include
> >
> > #include "audit.h"
> >
> > @@ -2899,10 +2900,34 @@ void __audit_log_kern_module(char *name)
> > context->type = AUDIT_KERN_MODULE;
> > }
> >
> > -void __audit_fanotify(u32 response)
> > +void __audit_fanotify(u32 response, size_t len, char *buf)
> > {
> > - audit_log(audit_context(), GFP_KERNEL,
> > - AUDIT_FANOTIFY, "resp=%u", response);
> > + struct fanotify_response_info_audit_rule *friar;
> > + size_t c = len;
> > + char *ib = buf;
> > +
> > + if (!(len && buf)) {
> > + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
> > + "resp=%u fan_type=0 fan_info=?", response);
> > + return;
> > + }
> > + while (c >= sizeof(struct fanotify_response_info_header)) {
> > + friar = (struct fanotify_response_info_audit_rule *)buf;
>
> Since the only use of this at the moment is the
> fanotify_response_info_rule, why not pass the
> fanotify_response_info_rule struct directly into this function? We
> can always change it if we need to in the future without affecting
> userspace, and it would simplify the code.
Steve, would it make any sense for there to be more than one
FAN_RESPONSE_INFO_AUDIT_RULE header in a message? Could there be more
than one rule that contributes to a notify reason? If not, would it be
reasonable to return -EINVAL if there is more than one?
> > + switch (friar->hdr.type) {
> > + case FAN_RESPONSE_INFO_AUDIT_RULE:
> > + if (friar->hdr.len < sizeof(*friar)) {
> > + audit_log(audit_context(), GFP_KERNEL,
> > AUDIT_FANOTIFY,
> > + "resp=%u fan_type=%u
> > fan_info=(incomplete)",
> > + response, friar->hdr.type);
> > + return;
> > + }
> > + audit_log(audit_context(), GFP_KERNEL,
> > AUDIT_FANOTIFY,
> > + "resp=%u fan_type=%u fan_info=%u",
> > + response, friar->hdr.type,
> > friar->audit_rule);
> > + }
> > + c -= friar->hdr.len;
> > + ib += friar->hdr.len;
> > + }
> > }
> >
> > void __audit_tk_injoffset(struct timespec64 offset)
>
> paul-moore.com
- RGB
--
Richard Guy Briggs
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
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v4 3/4] fanotify, audit: Allow audit to use the full permission event response
On Wednesday, August 31, 2022 5:07:25 PM EDT Richard Guy Briggs wrote:
> > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > index 433418d73584..f000fec52360 100644
> > > --- a/kernel/auditsc.c
> > > +++ b/kernel/auditsc.c
> > > @@ -64,6 +64,7 @@
> > > #include
> > > #include
> > > #include // struct open_how
> > > +#include
> > >
> > > #include "audit.h"
> > >
> > > @@ -2899,10 +2900,34 @@ void __audit_log_kern_module(char *name)
> > > context->type = AUDIT_KERN_MODULE;
> > > }
> > >
> > > -void __audit_fanotify(u32 response)
> > > +void __audit_fanotify(u32 response, size_t len, char *buf)
> > > {
> > > - audit_log(audit_context(), GFP_KERNEL,
> > > - AUDIT_FANOTIFY, "resp=%u", response);
> > > + struct fanotify_response_info_audit_rule *friar;
> > > + size_t c = len;
> > > + char *ib = buf;
> > > +
> > > + if (!(len && buf)) {
> > > + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
> > > + "resp=%u fan_type=0 fan_info=?", response);
> > > + return;
> > > + }
> > > + while (c >= sizeof(struct fanotify_response_info_header)) {
> > > + friar = (struct fanotify_response_info_audit_rule
> > > *)buf;
> >
> > Since the only use of this at the moment is the
> > fanotify_response_info_rule, why not pass the
> > fanotify_response_info_rule struct directly into this function? We
> > can always change it if we need to in the future without affecting
> > userspace, and it would simplify the code.
>
> Steve, would it make any sense for there to be more than one
> FAN_RESPONSE_INFO_AUDIT_RULE header in a message? Could there be more
> than one rule that contributes to a notify reason? If not, would it be
> reasonable to return -EINVAL if there is more than one?
I don't see a reason for sending more than one header. What is more probable
is the need to send additional data in that header. I was thinking of maybe
bit mapping it in the rule number. But I'd suggest padding the struct just in
case it needs expanding some day.
-Steev
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v4 3/4] fanotify,audit: Allow audit to use the full permission event response
On Tue, Aug 9, 2022 at 1:23 PM Richard Guy Briggs wrote:
>
> This patch passes the full value so that the audit function can use all
> of it. The audit function was updated to log the additional information in
> the AUDIT_FANOTIFY record. The following is an example of the new record
> format:
>
> type=FANOTIFY msg=audit(1600385147.372:590): resp=2 fan_type=1 fan_info=17
>
> Suggested-by: Steve Grubb
> Link: https://lore.kernel.org/r/3075502.aeNJFYEL58@x2
> Signed-off-by: Richard Guy Briggs
> ---
> fs/notify/fanotify/fanotify.c | 3 ++-
> include/linux/audit.h | 9 +
> kernel/auditsc.c | 31 ---
> 3 files changed, 35 insertions(+), 8 deletions(-)
You've hopefully already seen the kernel test robot build warning, so
I won't bring that up again, but a few comments below ...
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index 0f36062521f4..36c3ed1af085 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -276,7 +276,8 @@ static int fanotify_get_response(struct fsnotify_group
> *group,
>
> /* Check if the response should be audited */
> if (event->response & FAN_AUDIT)
> - audit_fanotify(event->response & ~FAN_AUDIT);
> + audit_fanotify(event->response & ~FAN_AUDIT,
> + event->info_len, event->info_buf);
>
> pr_debug("%s: group=%p event=%p about to return ret=%d\n", __func__,
> group, event, ret);
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 3ea198a2cd59..c69efdba12ca 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -14,6 +14,7 @@
> #include
> #include
> #include
> +#include
>
> #define AUDIT_INO_UNSET ((unsigned long)-1)
> #define AUDIT_DEV_UNSET ((dev_t)-1)
> @@ -417,7 +418,7 @@ extern void __audit_log_capset(const struct cred *new,
> const struct cred *old);
> extern void __audit_mmap_fd(int fd, int flags);
> extern void __audit_openat2_how(struct open_how *how);
> extern void __audit_log_kern_module(char *name);
> -extern void __audit_fanotify(u32 response);
> +extern void __audit_fanotify(u32 response, size_t len, char *buf);
> extern void __audit_tk_injoffset(struct timespec64 offset);
> extern void __audit_ntp_log(const struct audit_ntp_data *ad);
> extern void __audit_log_nfcfg(const char *name, u8 af, unsigned int nentries,
> @@ -524,10 +525,10 @@ static inline void audit_log_kern_module(char *name)
> __audit_log_kern_module(name);
> }
>
> -static inline void audit_fanotify(u32 response)
> +static inline void audit_fanotify(u32 response, size_t len, char *buf)
> {
> if (!audit_dummy_context())
> - __audit_fanotify(response);
> + __audit_fanotify(response, len, buf);
> }
>
> static inline void audit_tk_injoffset(struct timespec64 offset)
> @@ -684,7 +685,7 @@ static inline void audit_log_kern_module(char *name)
> {
> }
>
> -static inline void audit_fanotify(u32 response)
> +static inline void audit_fanotify(u32 response, size_t len, char *buf)
> { }
>
> static inline void audit_tk_injoffset(struct timespec64 offset)
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 433418d73584..f000fec52360 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -64,6 +64,7 @@
> #include
> #include
> #include // struct open_how
> +#include
>
> #include "audit.h"
>
> @@ -2899,10 +2900,34 @@ void __audit_log_kern_module(char *name)
> context->type = AUDIT_KERN_MODULE;
> }
>
> -void __audit_fanotify(u32 response)
> +void __audit_fanotify(u32 response, size_t len, char *buf)
> {
> - audit_log(audit_context(), GFP_KERNEL,
> - AUDIT_FANOTIFY, "resp=%u", response);
> + struct fanotify_response_info_audit_rule *friar;
> + size_t c = len;
> + char *ib = buf;
> +
> + if (!(len && buf)) {
> + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
> + "resp=%u fan_type=0 fan_info=?", response);
> + return;
> + }
> + while (c >= sizeof(struct fanotify_response_info_header)) {
> + friar = (struct fanotify_response_info_audit_rule *)buf;
Since the only use of this at the moment is the
fanotify_response_info_rule, why not pass the
fanotify_response_info_rule struct directly into this function? We
can always change it if we need to in the future without affecting
userspace, and it would simplify the code.
> + switch (friar->hdr.type) {
> + case FAN_RESPONSE_INFO_AUDIT_RULE:
> + if (friar->hdr.len < sizeof(*friar)) {
> + audit_log(audit_context(), GFP_KERNEL,
> AUDIT_FANOTIFY,
> + "resp=%u fan_type=%u
> fan_info=(incomplete)",
> + response, friar->hdr.type);
> +
Re: [PATCH v4 3/4] fanotify,audit: Allow audit to use the full permission event response
Hi Richard,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on jack-fs/fsnotify]
[also build test WARNING on pcmoore-audit/next linus/master v5.19 next-20220810]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url:
https://github.com/intel-lab-lkp/linux/commits/Richard-Guy-Briggs/fanotify-Allow-user-space-to-pass-back-additional-audit-info/20220810-012825
base: https://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git
fsnotify
config: x86_64-randconfig-a016
(https://download.01.org/0day-ci/archive/20220811/202208110406.lb3onrcp-...@intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project
5f1c7e2cc5a3c07cbc2412e851a7283c1841f520)
reproduce (this is a W=1 build):
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
#
https://github.com/intel-lab-lkp/linux/commit/bee8cac0b7796a753948c83b403a152f8c6acb8c
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review
Richard-Guy-Briggs/fanotify-Allow-user-space-to-pass-back-additional-audit-info/20220810-012825
git checkout bee8cac0b7796a753948c83b403a152f8c6acb8c
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1
O=build_dir ARCH=x86_64 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot
All warnings (new ones prefixed by >>):
>> kernel/auditsc.c:2907:8: warning: variable 'ib' set but not used
>> [-Wunused-but-set-variable]
char *ib = buf;
^
1 warning generated.
vim +/ib +2907 kernel/auditsc.c
2902
2903 void __audit_fanotify(u32 response, size_t len, char *buf)
2904 {
2905 struct fanotify_response_info_audit_rule *friar;
2906 size_t c = len;
> 2907 char *ib = buf;
2908
2909 if (!(len && buf)) {
2910 audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
2911"resp=%u fan_type=0 fan_info=?", response);
2912 return;
2913 }
2914 while (c >= sizeof(struct fanotify_response_info_header)) {
2915 friar = (struct fanotify_response_info_audit_rule *)buf;
2916 switch (friar->hdr.type) {
2917 case FAN_RESPONSE_INFO_AUDIT_RULE:
2918 if (friar->hdr.len < sizeof(*friar)) {
2919 audit_log(audit_context(), GFP_KERNEL,
AUDIT_FANOTIFY,
2920"resp=%u fan_type=%u
fan_info=(incomplete)",
2921response, friar->hdr.type);
2922 return;
2923 }
2924 audit_log(audit_context(), GFP_KERNEL,
AUDIT_FANOTIFY,
2925"resp=%u fan_type=%u fan_info=%u",
2926response, friar->hdr.type,
friar->audit_rule);
2927 }
2928 c -= friar->hdr.len;
2929 ib += friar->hdr.len;
2930 }
2931 }
2932
--
0-DAY CI Kernel Test Service
https://01.org/lkp
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
