Re: [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents

[Paul Moore]

On Mon, Nov 13, 2017 at 1:30 PM, Steve Grubb  wrote:
> On Thursday, November 9, 2017 3:52:46 PM EST Richard Guy Briggs wrote:
>> > >> > It might be simplest to just apply a corrective patch over top of
>> > >> > this one so that you don't have to muck about with git branches and
>> > >> > commit messages.
>> > >>
>> > >> A quick note on the "corrective patch": given we are just days away
>> > >> from the merge window opening, it is *way* to late for something like
>> > >> that, at this point the only options are to leave it as-is or
>> > >> yank/revert and make another pass during the next development phase.
>> > >
>> > > Then yank it. I think that is overreacting but given the options you
>> > > presented its the only one that avoids changing a critical field
>> > > format.
>> >
>> > It's not overreacting Steve, there is simply no way we can test and
>> > adequately soak new changes in the few days we have left.
>
> Its just moving the output of the information a few lines down further in the
> code. 10 minutes of work, tops.

It's like you don't even bother reading why I write ... it's not about
the amount of time needed to make the change, it's the other stuff I
mentioned.  Regardless, it's a moot point now, the patch is out and it
isn't going up in the currently open merge window.

>> > Event yanks/reverts carry a risk at this stage, but I consider that the
>> > less risky option for these patches.  Neither is a great option, and that
>> > is why I'm rather annoyed.
>>
>> I don't really see that this is my choice to include it or not.  This is
>> the upstream maintainer's decision.
>>
>> I can't say I'd be thrilled to have my name on something that stuffs up
>> the system though.  It still isn't clear to me why an incomplete path
>> from some seemingly random place in the filesystem tree is preferable to
>> something that gives it an anchor point, at least to human interpreters.
>
> The path should stay. Just the file system type needs decoupling and moving.

See my previous comments about relative pathnames, specifially the
part about me not being a fan of them in the PATH records.

-- 
paul moore
www.paul-moore.com

Re: [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents

[Paul Moore]

On Mon, Nov 13, 2017 at 1:30 PM, Steve Grubb  wrote:
> On Thursday, November 9, 2017 3:52:46 PM EST Richard Guy Briggs wrote:
>> > >> > It might be simplest to just apply a corrective patch over top of
>> > >> > this one so that you don't have to muck about with git branches and
>> > >> > commit messages.
>> > >>
>> > >> A quick note on the "corrective patch": given we are just days away
>> > >> from the merge window opening, it is *way* to late for something like
>> > >> that, at this point the only options are to leave it as-is or
>> > >> yank/revert and make another pass during the next development phase.
>> > >
>> > > Then yank it. I think that is overreacting but given the options you
>> > > presented its the only one that avoids changing a critical field
>> > > format.
>> >
>> > It's not overreacting Steve, there is simply no way we can test and
>> > adequately soak new changes in the few days we have left.
>
> Its just moving the output of the information a few lines down further in the
> code. 10 minutes of work, tops.

It's like you don't even bother reading why I write ... it's not about
the amount of time needed to make the change, it's the other stuff I
mentioned.  Regardless, it's a moot point now, the patch is out and it
isn't going up in the currently open merge window.

>> > Event yanks/reverts carry a risk at this stage, but I consider that the
>> > less risky option for these patches.  Neither is a great option, and that
>> > is why I'm rather annoyed.
>>
>> I don't really see that this is my choice to include it or not.  This is
>> the upstream maintainer's decision.
>>
>> I can't say I'd be thrilled to have my name on something that stuffs up
>> the system though.  It still isn't clear to me why an incomplete path
>> from some seemingly random place in the filesystem tree is preferable to
>> something that gives it an anchor point, at least to human interpreters.
>
> The path should stay. Just the file system type needs decoupling and moving.

See my previous comments about relative pathnames, specifially the
part about me not being a fan of them in the PATH records.

-- 
paul moore
www.paul-moore.com

Re: [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents

[Steve Grubb]

On Thursday, November 9, 2017 3:52:46 PM EST Richard Guy Briggs wrote:
> > >> > It might be simplest to just apply a corrective patch over top of
> > >> > this one so that you don't have to muck about with git branches and
> > >> > commit messages.
> > >> 
> > >> A quick note on the "corrective patch": given we are just days away
> > >> from the merge window opening, it is *way* to late for something like
> > >> that, at this point the only options are to leave it as-is or
> > >> yank/revert and make another pass during the next development phase.
> > > 
> > > Then yank it. I think that is overreacting but given the options you
> > > presented its the only one that avoids changing a critical field
> > > format.
> > 
> > It's not overreacting Steve, there is simply no way we can test and
> > adequately soak new changes in the few days we have left.

Its just moving the output of the information a few lines down further in the 
code. 10 minutes of work, tops.

> > Event yanks/reverts carry a risk at this stage, but I consider that the
> > less risky option for these patches.  Neither is a great option, and that
> > is why I'm rather annoyed.
>
> I don't really see that this is my choice to include it or not.  This is
> the upstream maintainer's decision.
> 
> I can't say I'd be thrilled to have my name on something that stuffs up
> the system though.  It still isn't clear to me why an incomplete path
> from some seemingly random place in the filesystem tree is preferable to
> something that gives it an anchor point, at least to human interpreters.

The path should stay. Just the file system type needs decoupling and moving.

> Adding an fstype to the record is an interesting idea, but then creates
> a void for all the rest of the properly formed records that don't need
> it and will need more work to find it, wasting bandwidth with
> "fstype=?". 

I would let it optionally swing in and out at the end of the record. This 
should never show up on a normal system because the rules will suppress 
generating this information by default. So, it really won't be all that 
visible.

> How are the analysis tools stymied by a text prefix to a path that it can't
> find anyways?

Because they do not actually resolve anything in the file system. They take 
the event as ground truth and use that. Also, the tools expect name=value. 
This has been the way since the beginning. We do not lump multiple independent 
values together. And then what if the path has a special character in it? The 
whole value then has to be encoded. And I don't think the patch is using 
untrusted string like it should.

> Since we have a chance to fix it before it goes upstream, I think it
> should either be yanked and respun, or add a corrective patch and submit
> them together.
> 
> > >> As for the objection itself: ungh.  There is really no good reason why
> > >> you couldn't have seen this in the *several* *months* prior to this;
> > >> Richard wrote a nice patch description which *included* sample audit
> > >> events, and you were involved in discussions regarding this patchset.
> > >> To say I'm disappointed would be an understatement.
> > > 
> > > I am also disappointed to find that we are modifying a searchable field
> > > that has been defined since 2005. The "name" field is very important.
> > > It's used in quite a few reports, its used in the text format, it's
> > > searchable, and we have a dictionary that defines exactly what it is.
> > > Fields that are searchable and used in common reports cannot be changed
> > > without a whole lot of coordination. I'm also disappointed to have to
> > > point out that new information should go in its own field. I thought
> > > this was common knowledge. In any event, it was caught and problems can
> > > be avoided.
> 
> So why does this make it unsearchable?

I didn't say it makes in unsearchable, but now that you mention it...it does 
in one case. Searchable fields are more important. They typically are the 
object or subject or some kind of special attribute that is commonly searched 
on to group events. Searchable fields can either be partial or full word 
match. By combining information in the same field, it will change this 
behavior. The path name is the object of the event. By combining information 
that is not the object with it, everyone will have to change and update their 
software to handle this change in behavior.

> I still don't understand any explanations that have been made so far

Try ausearch --text on those events, or aureport --file.

-Steve

Re: [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents

[Steve Grubb]

On Thursday, November 9, 2017 3:52:46 PM EST Richard Guy Briggs wrote:
> > >> > It might be simplest to just apply a corrective patch over top of
> > >> > this one so that you don't have to muck about with git branches and
> > >> > commit messages.
> > >> 
> > >> A quick note on the "corrective patch": given we are just days away
> > >> from the merge window opening, it is *way* to late for something like
> > >> that, at this point the only options are to leave it as-is or
> > >> yank/revert and make another pass during the next development phase.
> > > 
> > > Then yank it. I think that is overreacting but given the options you
> > > presented its the only one that avoids changing a critical field
> > > format.
> > 
> > It's not overreacting Steve, there is simply no way we can test and
> > adequately soak new changes in the few days we have left.

Its just moving the output of the information a few lines down further in the 
code. 10 minutes of work, tops.

> > Event yanks/reverts carry a risk at this stage, but I consider that the
> > less risky option for these patches.  Neither is a great option, and that
> > is why I'm rather annoyed.
>
> I don't really see that this is my choice to include it or not.  This is
> the upstream maintainer's decision.
> 
> I can't say I'd be thrilled to have my name on something that stuffs up
> the system though.  It still isn't clear to me why an incomplete path
> from some seemingly random place in the filesystem tree is preferable to
> something that gives it an anchor point, at least to human interpreters.

The path should stay. Just the file system type needs decoupling and moving.

> Adding an fstype to the record is an interesting idea, but then creates
> a void for all the rest of the properly formed records that don't need
> it and will need more work to find it, wasting bandwidth with
> "fstype=?". 

I would let it optionally swing in and out at the end of the record. This 
should never show up on a normal system because the rules will suppress 
generating this information by default. So, it really won't be all that 
visible.

> How are the analysis tools stymied by a text prefix to a path that it can't
> find anyways?

Because they do not actually resolve anything in the file system. They take 
the event as ground truth and use that. Also, the tools expect name=value. 
This has been the way since the beginning. We do not lump multiple independent 
values together. And then what if the path has a special character in it? The 
whole value then has to be encoded. And I don't think the patch is using 
untrusted string like it should.

> Since we have a chance to fix it before it goes upstream, I think it
> should either be yanked and respun, or add a corrective patch and submit
> them together.
> 
> > >> As for the objection itself: ungh.  There is really no good reason why
> > >> you couldn't have seen this in the *several* *months* prior to this;
> > >> Richard wrote a nice patch description which *included* sample audit
> > >> events, and you were involved in discussions regarding this patchset.
> > >> To say I'm disappointed would be an understatement.
> > > 
> > > I am also disappointed to find that we are modifying a searchable field
> > > that has been defined since 2005. The "name" field is very important.
> > > It's used in quite a few reports, its used in the text format, it's
> > > searchable, and we have a dictionary that defines exactly what it is.
> > > Fields that are searchable and used in common reports cannot be changed
> > > without a whole lot of coordination. I'm also disappointed to have to
> > > point out that new information should go in its own field. I thought
> > > this was common knowledge. In any event, it was caught and problems can
> > > be avoided.
> 
> So why does this make it unsearchable?

I didn't say it makes in unsearchable, but now that you mention it...it does 
in one case. Searchable fields are more important. They typically are the 
object or subject or some kind of special attribute that is commonly searched 
on to group events. Searchable fields can either be partial or full word 
match. By combining information in the same field, it will change this 
behavior. The path name is the object of the event. By combining information 
that is not the object with it, everyone will have to change and update their 
software to handle this change in behavior.

> I still don't understand any explanations that have been made so far

Try ausearch --text on those events, or aureport --file.

-Steve

Re: [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents

[Richard Guy Briggs]

On 2017-11-09 16:47, Paul Moore wrote:
> On Thu, Nov 9, 2017 at 3:52 PM, Richard Guy Briggs  wrote:
> > On 2017-11-09 10:59, Paul Moore wrote:
> >> On Thu, Nov 9, 2017 at 10:31 AM, Steve Grubb  wrote:
> >> > On Thursday, November 9, 2017 10:18:10 AM EST Paul Moore wrote:
> >> >> On Wed, Nov 8, 2017 at 6:29 PM, Steve Grubb  wrote:
> >>
> >> ...
> >>
> >> >> > Late reply...but I just noticed that this changes the format of the 
> >> >> > "name"
> >> >> > field - which is undesirable. Please put the file system type in a 
> >> >> > field
> >> >> > all by itself called "fstype". You can just leave it as the hex magic
> >> >> > number prepended with 0x and user space can do the lookup from there,
> >> >> >
> >> >> > It might be simplest to just apply a corrective patch over top of 
> >> >> > this one
> >> >> > so that you don't have to muck about with git branches and commit
> >> >> > messages.
> >> >>
> >> >> A quick note on the "corrective patch": given we are just days away
> >> >> from the merge window opening, it is *way* to late for something like
> >> >> that, at this point the only options are to leave it as-is or
> >> >> yank/revert and make another pass during the next development phase.
> >> >
> >> > Then yank it. I think that is overreacting but given the options you 
> >> > presented
> >> > its the only one that avoids changing a critical field format.
> >>
> >> It's not overreacting Steve, there is simply no way we can test and
> >> adequately soak new changes in the few days we have left.  Event
> >> yanks/reverts carry a risk at this stage, but I consider that the less
> >> risky option for these patches.  Neither is a great option, and that
> >> is why I'm rather annoyed.
> >
> > I don't really see that this is my choice to include it or not.  This is
> > the upstream maintainer's decision.
> 
> You are right, however, while ultimately it isn't your choice I still
> wanted to hear your opinion on this as you have put a lot of effort
> into this patchset.
> 
> > I can't say I'd be thrilled to have my name on something that stuffs up
> > the system though.  It still isn't clear to me why an incomplete path
> > from some seemingly random place in the filesystem tree is preferable to
> > something that gives it an anchor point, at least to human interpreters.
> 
> That confuses me too.  My current thinking is that a partial, or
> relative, path is not something we want.
> 
> > Adding an fstype to the record is an interesting idea, but then creates
> > a void for all the rest of the properly formed records that don't need
> > it and will need more work to find it, wasting bandwidth with
> > "fstype=?".
> 
> Not to mention we still have the relative path problem in this case.
> 
> > How are the analysis tools stymied by a text prefix to a path that it can't 
> > find anyways?
> 
> I've been wondering the same.  My gut feeling isn't a positive comment
> so I'll refrain from sharing it here.
> 
> > Since we have a chance to fix it before it goes upstream, I think it
> > should either be yanked and respun, or add a corrective patch and submit
> > them together.
> 
> The odds of agreeing upon a corrective patch and getting it tested and
> soaked before the merge window opens is z-e-r-o.  As I said earlier,
> at the very top of my first response, this isn't an option (I'm hoping
> you just missed reading that).

Oh, I read that.  That's what informed my position.  That should help
you make your decision.

> I've been testing audit/next without patch 1/2 this afternoon and it
> is still looking okay; unless I see something arguing against it
> within the next hour or two that's what I'm going to send up to Linus.
> 
> >> >> As for the objection itself: ungh.  There is really no good reason why
> >> >> you couldn't have seen this in the *several* *months* prior to this;
> >> >> Richard wrote a nice patch description which *included* sample audit
> >> >> events, and you were involved in discussions regarding this patchset.
> >> >> To say I'm disappointed would be an understatement.
> >> >
> >> > I am also disappointed to find that we are modifying a searchable field 
> >> > that
> >> > has been defined since 2005. The "name" field is very important. It's 
> >> > used in
> >> > quite a few reports, its used in the text format, it's searchable, and 
> >> > we have
> >> > a dictionary that defines exactly what it is. Fields that are searchable 
> >> > and
> >> > used in common reports cannot be changed without a whole lot of 
> >> > coordination.
> >> > I'm also disappointed to have to point out that new information should 
> >> > go in
> >> > its own field. I thought this was common knowledge. In any event, it was
> >> > caught and problems can be avoided.
> >
> > So why does this make it unsearchable?  I still don't understand any
> > explanations that have been made so far.
> 
> Agree.
> 
> >> There are plenty of things to say about the above comment, 

Re: [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents

[Richard Guy Briggs]

On 2017-11-09 16:47, Paul Moore wrote:
> On Thu, Nov 9, 2017 at 3:52 PM, Richard Guy Briggs  wrote:
> > On 2017-11-09 10:59, Paul Moore wrote:
> >> On Thu, Nov 9, 2017 at 10:31 AM, Steve Grubb  wrote:
> >> > On Thursday, November 9, 2017 10:18:10 AM EST Paul Moore wrote:
> >> >> On Wed, Nov 8, 2017 at 6:29 PM, Steve Grubb  wrote:
> >>
> >> ...
> >>
> >> >> > Late reply...but I just noticed that this changes the format of the 
> >> >> > "name"
> >> >> > field - which is undesirable. Please put the file system type in a 
> >> >> > field
> >> >> > all by itself called "fstype". You can just leave it as the hex magic
> >> >> > number prepended with 0x and user space can do the lookup from there,
> >> >> >
> >> >> > It might be simplest to just apply a corrective patch over top of 
> >> >> > this one
> >> >> > so that you don't have to muck about with git branches and commit
> >> >> > messages.
> >> >>
> >> >> A quick note on the "corrective patch": given we are just days away
> >> >> from the merge window opening, it is *way* to late for something like
> >> >> that, at this point the only options are to leave it as-is or
> >> >> yank/revert and make another pass during the next development phase.
> >> >
> >> > Then yank it. I think that is overreacting but given the options you 
> >> > presented
> >> > its the only one that avoids changing a critical field format.
> >>
> >> It's not overreacting Steve, there is simply no way we can test and
> >> adequately soak new changes in the few days we have left.  Event
> >> yanks/reverts carry a risk at this stage, but I consider that the less
> >> risky option for these patches.  Neither is a great option, and that
> >> is why I'm rather annoyed.
> >
> > I don't really see that this is my choice to include it or not.  This is
> > the upstream maintainer's decision.
> 
> You are right, however, while ultimately it isn't your choice I still
> wanted to hear your opinion on this as you have put a lot of effort
> into this patchset.
> 
> > I can't say I'd be thrilled to have my name on something that stuffs up
> > the system though.  It still isn't clear to me why an incomplete path
> > from some seemingly random place in the filesystem tree is preferable to
> > something that gives it an anchor point, at least to human interpreters.
> 
> That confuses me too.  My current thinking is that a partial, or
> relative, path is not something we want.
> 
> > Adding an fstype to the record is an interesting idea, but then creates
> > a void for all the rest of the properly formed records that don't need
> > it and will need more work to find it, wasting bandwidth with
> > "fstype=?".
> 
> Not to mention we still have the relative path problem in this case.
> 
> > How are the analysis tools stymied by a text prefix to a path that it can't 
> > find anyways?
> 
> I've been wondering the same.  My gut feeling isn't a positive comment
> so I'll refrain from sharing it here.
> 
> > Since we have a chance to fix it before it goes upstream, I think it
> > should either be yanked and respun, or add a corrective patch and submit
> > them together.
> 
> The odds of agreeing upon a corrective patch and getting it tested and
> soaked before the merge window opens is z-e-r-o.  As I said earlier,
> at the very top of my first response, this isn't an option (I'm hoping
> you just missed reading that).

Oh, I read that.  That's what informed my position.  That should help
you make your decision.

> I've been testing audit/next without patch 1/2 this afternoon and it
> is still looking okay; unless I see something arguing against it
> within the next hour or two that's what I'm going to send up to Linus.
> 
> >> >> As for the objection itself: ungh.  There is really no good reason why
> >> >> you couldn't have seen this in the *several* *months* prior to this;
> >> >> Richard wrote a nice patch description which *included* sample audit
> >> >> events, and you were involved in discussions regarding this patchset.
> >> >> To say I'm disappointed would be an understatement.
> >> >
> >> > I am also disappointed to find that we are modifying a searchable field 
> >> > that
> >> > has been defined since 2005. The "name" field is very important. It's 
> >> > used in
> >> > quite a few reports, its used in the text format, it's searchable, and 
> >> > we have
> >> > a dictionary that defines exactly what it is. Fields that are searchable 
> >> > and
> >> > used in common reports cannot be changed without a whole lot of 
> >> > coordination.
> >> > I'm also disappointed to have to point out that new information should 
> >> > go in
> >> > its own field. I thought this was common knowledge. In any event, it was
> >> > caught and problems can be avoided.
> >
> > So why does this make it unsearchable?  I still don't understand any
> > explanations that have been made so far.
> 
> Agree.
> 
> >> There are plenty of things to say about the above comment, but in the
> >> interest of brevity I'm just going to 

Re: [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents

[Paul Moore]

On Thu, Nov 9, 2017 at 3:52 PM, Richard Guy Briggs  wrote:
> On 2017-11-09 10:59, Paul Moore wrote:
>> On Thu, Nov 9, 2017 at 10:31 AM, Steve Grubb  wrote:
>> > On Thursday, November 9, 2017 10:18:10 AM EST Paul Moore wrote:
>> >> On Wed, Nov 8, 2017 at 6:29 PM, Steve Grubb  wrote:
>>
>> ...
>>
>> >> > Late reply...but I just noticed that this changes the format of the 
>> >> > "name"
>> >> > field - which is undesirable. Please put the file system type in a field
>> >> > all by itself called "fstype". You can just leave it as the hex magic
>> >> > number prepended with 0x and user space can do the lookup from there,
>> >> >
>> >> > It might be simplest to just apply a corrective patch over top of this 
>> >> > one
>> >> > so that you don't have to muck about with git branches and commit
>> >> > messages.
>> >>
>> >> A quick note on the "corrective patch": given we are just days away
>> >> from the merge window opening, it is *way* to late for something like
>> >> that, at this point the only options are to leave it as-is or
>> >> yank/revert and make another pass during the next development phase.
>> >
>> > Then yank it. I think that is overreacting but given the options you 
>> > presented
>> > its the only one that avoids changing a critical field format.
>>
>> It's not overreacting Steve, there is simply no way we can test and
>> adequately soak new changes in the few days we have left.  Event
>> yanks/reverts carry a risk at this stage, but I consider that the less
>> risky option for these patches.  Neither is a great option, and that
>> is why I'm rather annoyed.
>
> I don't really see that this is my choice to include it or not.  This is
> the upstream maintainer's decision.

You are right, however, while ultimately it isn't your choice I still
wanted to hear your opinion on this as you have put a lot of effort
into this patchset.

> I can't say I'd be thrilled to have my name on something that stuffs up
> the system though.  It still isn't clear to me why an incomplete path
> from some seemingly random place in the filesystem tree is preferable to
> something that gives it an anchor point, at least to human interpreters.

That confuses me too.  My current thinking is that a partial, or
relative, path is not something we want.

> Adding an fstype to the record is an interesting idea, but then creates
> a void for all the rest of the properly formed records that don't need
> it and will need more work to find it, wasting bandwidth with
> "fstype=?".

Not to mention we still have the relative path problem in this case.

> How are the analysis tools stymied by a text prefix to a path that it can't 
> find anyways?

I've been wondering the same.  My gut feeling isn't a positive comment
so I'll refrain from sharing it here.

> Since we have a chance to fix it before it goes upstream, I think it
> should either be yanked and respun, or add a corrective patch and submit
> them together.

The odds of agreeing upon a corrective patch and getting it tested and
soaked before the merge window opens is z-e-r-o.  As I said earlier,
at the very top of my first response, this isn't an option (I'm hoping
you just missed reading that).

I've been testing audit/next without patch 1/2 this afternoon and it
is still looking okay; unless I see something arguing against it
within the next hour or two that's what I'm going to send up to Linus.

>> >> As for the objection itself: ungh.  There is really no good reason why
>> >> you couldn't have seen this in the *several* *months* prior to this;
>> >> Richard wrote a nice patch description which *included* sample audit
>> >> events, and you were involved in discussions regarding this patchset.
>> >> To say I'm disappointed would be an understatement.
>> >
>> > I am also disappointed to find that we are modifying a searchable field 
>> > that
>> > has been defined since 2005. The "name" field is very important. It's used 
>> > in
>> > quite a few reports, its used in the text format, it's searchable, and we 
>> > have
>> > a dictionary that defines exactly what it is. Fields that are searchable 
>> > and
>> > used in common reports cannot be changed without a whole lot of 
>> > coordination.
>> > I'm also disappointed to have to point out that new information should go 
>> > in
>> > its own field. I thought this was common knowledge. In any event, it was
>> > caught and problems can be avoided.
>
> So why does this make it unsearchable?  I still don't understand any
> explanations that have been made so far.

Agree.

>> There are plenty of things to say about the above comment, but in the
>> interest of brevity I'm just going to leave it at the assumptions and
>> inflexibility in your audit userspace continue to amaze me in all the
>> worst ways.  Regardless, as you say, the problem can likely be avoided
>> this time.
>>
>> >> I need to look at the rest of audit/next to see what a mess things
>> >> would be if I yanked 

Re: [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents

[Paul Moore]

On Thu, Nov 9, 2017 at 3:52 PM, Richard Guy Briggs  wrote:
> On 2017-11-09 10:59, Paul Moore wrote:
>> On Thu, Nov 9, 2017 at 10:31 AM, Steve Grubb  wrote:
>> > On Thursday, November 9, 2017 10:18:10 AM EST Paul Moore wrote:
>> >> On Wed, Nov 8, 2017 at 6:29 PM, Steve Grubb  wrote:
>>
>> ...
>>
>> >> > Late reply...but I just noticed that this changes the format of the 
>> >> > "name"
>> >> > field - which is undesirable. Please put the file system type in a field
>> >> > all by itself called "fstype". You can just leave it as the hex magic
>> >> > number prepended with 0x and user space can do the lookup from there,
>> >> >
>> >> > It might be simplest to just apply a corrective patch over top of this 
>> >> > one
>> >> > so that you don't have to muck about with git branches and commit
>> >> > messages.
>> >>
>> >> A quick note on the "corrective patch": given we are just days away
>> >> from the merge window opening, it is *way* to late for something like
>> >> that, at this point the only options are to leave it as-is or
>> >> yank/revert and make another pass during the next development phase.
>> >
>> > Then yank it. I think that is overreacting but given the options you 
>> > presented
>> > its the only one that avoids changing a critical field format.
>>
>> It's not overreacting Steve, there is simply no way we can test and
>> adequately soak new changes in the few days we have left.  Event
>> yanks/reverts carry a risk at this stage, but I consider that the less
>> risky option for these patches.  Neither is a great option, and that
>> is why I'm rather annoyed.
>
> I don't really see that this is my choice to include it or not.  This is
> the upstream maintainer's decision.

You are right, however, while ultimately it isn't your choice I still
wanted to hear your opinion on this as you have put a lot of effort
into this patchset.

> I can't say I'd be thrilled to have my name on something that stuffs up
> the system though.  It still isn't clear to me why an incomplete path
> from some seemingly random place in the filesystem tree is preferable to
> something that gives it an anchor point, at least to human interpreters.

That confuses me too.  My current thinking is that a partial, or
relative, path is not something we want.

> Adding an fstype to the record is an interesting idea, but then creates
> a void for all the rest of the properly formed records that don't need
> it and will need more work to find it, wasting bandwidth with
> "fstype=?".

Not to mention we still have the relative path problem in this case.

> How are the analysis tools stymied by a text prefix to a path that it can't 
> find anyways?

I've been wondering the same.  My gut feeling isn't a positive comment
so I'll refrain from sharing it here.

> Since we have a chance to fix it before it goes upstream, I think it
> should either be yanked and respun, or add a corrective patch and submit
> them together.

The odds of agreeing upon a corrective patch and getting it tested and
soaked before the merge window opens is z-e-r-o.  As I said earlier,
at the very top of my first response, this isn't an option (I'm hoping
you just missed reading that).

I've been testing audit/next without patch 1/2 this afternoon and it
is still looking okay; unless I see something arguing against it
within the next hour or two that's what I'm going to send up to Linus.

>> >> As for the objection itself: ungh.  There is really no good reason why
>> >> you couldn't have seen this in the *several* *months* prior to this;
>> >> Richard wrote a nice patch description which *included* sample audit
>> >> events, and you were involved in discussions regarding this patchset.
>> >> To say I'm disappointed would be an understatement.
>> >
>> > I am also disappointed to find that we are modifying a searchable field 
>> > that
>> > has been defined since 2005. The "name" field is very important. It's used 
>> > in
>> > quite a few reports, its used in the text format, it's searchable, and we 
>> > have
>> > a dictionary that defines exactly what it is. Fields that are searchable 
>> > and
>> > used in common reports cannot be changed without a whole lot of 
>> > coordination.
>> > I'm also disappointed to have to point out that new information should go 
>> > in
>> > its own field. I thought this was common knowledge. In any event, it was
>> > caught and problems can be avoided.
>
> So why does this make it unsearchable?  I still don't understand any
> explanations that have been made so far.

Agree.

>> There are plenty of things to say about the above comment, but in the
>> interest of brevity I'm just going to leave it at the assumptions and
>> inflexibility in your audit userspace continue to amaze me in all the
>> worst ways.  Regardless, as you say, the problem can likely be avoided
>> this time.
>>
>> >> I need to look at the rest of audit/next to see what a mess things
>> >> would be if I yanked this patch.  I don't expect it to be bad, but
>> >> 

Re: [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents

[Richard Guy Briggs]

On 2017-11-09 10:59, Paul Moore wrote:
> On Thu, Nov 9, 2017 at 10:31 AM, Steve Grubb  wrote:
> > On Thursday, November 9, 2017 10:18:10 AM EST Paul Moore wrote:
> >> On Wed, Nov 8, 2017 at 6:29 PM, Steve Grubb  wrote:
> 
> ...
> 
> >> > Late reply...but I just noticed that this changes the format of the 
> >> > "name"
> >> > field - which is undesirable. Please put the file system type in a field
> >> > all by itself called "fstype". You can just leave it as the hex magic
> >> > number prepended with 0x and user space can do the lookup from there,
> >> >
> >> > It might be simplest to just apply a corrective patch over top of this 
> >> > one
> >> > so that you don't have to muck about with git branches and commit
> >> > messages.
> >>
> >> A quick note on the "corrective patch": given we are just days away
> >> from the merge window opening, it is *way* to late for something like
> >> that, at this point the only options are to leave it as-is or
> >> yank/revert and make another pass during the next development phase.
> >
> > Then yank it. I think that is overreacting but given the options you 
> > presented
> > its the only one that avoids changing a critical field format.
> 
> It's not overreacting Steve, there is simply no way we can test and
> adequately soak new changes in the few days we have left.  Event
> yanks/reverts carry a risk at this stage, but I consider that the less
> risky option for these patches.  Neither is a great option, and that
> is why I'm rather annoyed.

I don't really see that this is my choice to include it or not.  This is
the upstream maintainer's decision.

I can't say I'd be thrilled to have my name on something that stuffs up
the system though.  It still isn't clear to me why an incomplete path
from some seemingly random place in the filesystem tree is preferable to
something that gives it an anchor point, at least to human interpreters.
Adding an fstype to the record is an interesting idea, but then creates
a void for all the rest of the properly formed records that don't need
it and will need more work to find it, wasting bandwidth with
"fstype=?".  How are the analysis tools stymied by a text prefix to a
path that it can't find anyways?

Since we have a chance to fix it before it goes upstream, I think it
should either be yanked and respun, or add a corrective patch and submit
them together.

> >> As for the objection itself: ungh.  There is really no good reason why
> >> you couldn't have seen this in the *several* *months* prior to this;
> >> Richard wrote a nice patch description which *included* sample audit
> >> events, and you were involved in discussions regarding this patchset.
> >> To say I'm disappointed would be an understatement.
> >
> > I am also disappointed to find that we are modifying a searchable field that
> > has been defined since 2005. The "name" field is very important. It's used 
> > in
> > quite a few reports, its used in the text format, it's searchable, and we 
> > have
> > a dictionary that defines exactly what it is. Fields that are searchable and
> > used in common reports cannot be changed without a whole lot of 
> > coordination.
> > I'm also disappointed to have to point out that new information should go in
> > its own field. I thought this was common knowledge. In any event, it was
> > caught and problems can be avoided.

So why does this make it unsearchable?  I still don't understand any
explanations that have been made so far.

> There are plenty of things to say about the above comment, but in the
> interest of brevity I'm just going to leave it at the assumptions and
> inflexibility in your audit userspace continue to amaze me in all the
> worst ways.  Regardless, as you say, the problem can likely be avoided
> this time.
> 
> >> I need to look at the rest of audit/next to see what a mess things
> >> would be if I yanked this patch.  I don't expect it to be bad, but
> >> taking a look will also give Richard a chance to voice his thoughts;
> >> it is his patch after all, it would be nice to see an "OK" from him.
> >> Whatever we do, it needs to happen by the of the day today (Thursday,
> >> November 9th) as we need time to build and test the revised patches.
> 
> FWIW, I just went through audit/next and it looks like yanking patch
> 1/2 isn't going to be too painful; I'm waiting on the build to finish
> now.  Also, as a FYI, Richard's 2/2 filtering patch is going to remain
> in audit/next as that appears unrelated to the pathname objection,
> applies cleanly, and still offers value.

The irony here stuns me.  2/2 was supposed to be the more controvertial
one.

> paul moore

- 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

Re: [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents

[Richard Guy Briggs]

On 2017-11-09 10:59, Paul Moore wrote:
> On Thu, Nov 9, 2017 at 10:31 AM, Steve Grubb  wrote:
> > On Thursday, November 9, 2017 10:18:10 AM EST Paul Moore wrote:
> >> On Wed, Nov 8, 2017 at 6:29 PM, Steve Grubb  wrote:
> 
> ...
> 
> >> > Late reply...but I just noticed that this changes the format of the 
> >> > "name"
> >> > field - which is undesirable. Please put the file system type in a field
> >> > all by itself called "fstype". You can just leave it as the hex magic
> >> > number prepended with 0x and user space can do the lookup from there,
> >> >
> >> > It might be simplest to just apply a corrective patch over top of this 
> >> > one
> >> > so that you don't have to muck about with git branches and commit
> >> > messages.
> >>
> >> A quick note on the "corrective patch": given we are just days away
> >> from the merge window opening, it is *way* to late for something like
> >> that, at this point the only options are to leave it as-is or
> >> yank/revert and make another pass during the next development phase.
> >
> > Then yank it. I think that is overreacting but given the options you 
> > presented
> > its the only one that avoids changing a critical field format.
> 
> It's not overreacting Steve, there is simply no way we can test and
> adequately soak new changes in the few days we have left.  Event
> yanks/reverts carry a risk at this stage, but I consider that the less
> risky option for these patches.  Neither is a great option, and that
> is why I'm rather annoyed.

I don't really see that this is my choice to include it or not.  This is
the upstream maintainer's decision.

I can't say I'd be thrilled to have my name on something that stuffs up
the system though.  It still isn't clear to me why an incomplete path
from some seemingly random place in the filesystem tree is preferable to
something that gives it an anchor point, at least to human interpreters.
Adding an fstype to the record is an interesting idea, but then creates
a void for all the rest of the properly formed records that don't need
it and will need more work to find it, wasting bandwidth with
"fstype=?".  How are the analysis tools stymied by a text prefix to a
path that it can't find anyways?

Since we have a chance to fix it before it goes upstream, I think it
should either be yanked and respun, or add a corrective patch and submit
them together.

> >> As for the objection itself: ungh.  There is really no good reason why
> >> you couldn't have seen this in the *several* *months* prior to this;
> >> Richard wrote a nice patch description which *included* sample audit
> >> events, and you were involved in discussions regarding this patchset.
> >> To say I'm disappointed would be an understatement.
> >
> > I am also disappointed to find that we are modifying a searchable field that
> > has been defined since 2005. The "name" field is very important. It's used 
> > in
> > quite a few reports, its used in the text format, it's searchable, and we 
> > have
> > a dictionary that defines exactly what it is. Fields that are searchable and
> > used in common reports cannot be changed without a whole lot of 
> > coordination.
> > I'm also disappointed to have to point out that new information should go in
> > its own field. I thought this was common knowledge. In any event, it was
> > caught and problems can be avoided.

So why does this make it unsearchable?  I still don't understand any
explanations that have been made so far.

> There are plenty of things to say about the above comment, but in the
> interest of brevity I'm just going to leave it at the assumptions and
> inflexibility in your audit userspace continue to amaze me in all the
> worst ways.  Regardless, as you say, the problem can likely be avoided
> this time.
> 
> >> I need to look at the rest of audit/next to see what a mess things
> >> would be if I yanked this patch.  I don't expect it to be bad, but
> >> taking a look will also give Richard a chance to voice his thoughts;
> >> it is his patch after all, it would be nice to see an "OK" from him.
> >> Whatever we do, it needs to happen by the of the day today (Thursday,
> >> November 9th) as we need time to build and test the revised patches.
> 
> FWIW, I just went through audit/next and it looks like yanking patch
> 1/2 isn't going to be too painful; I'm waiting on the build to finish
> now.  Also, as a FYI, Richard's 2/2 filtering patch is going to remain
> in audit/next as that appears unrelated to the pathname objection,
> applies cleanly, and still offers value.

The irony here stuns me.  2/2 was supposed to be the more controvertial
one.

> paul moore

- 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

Re: [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents

[Paul Moore]

On Thu, Nov 9, 2017 at 10:31 AM, Steve Grubb  wrote:
> On Thursday, November 9, 2017 10:18:10 AM EST Paul Moore wrote:
>> On Wed, Nov 8, 2017 at 6:29 PM, Steve Grubb  wrote:

...

>> > Late reply...but I just noticed that this changes the format of the "name"
>> > field - which is undesirable. Please put the file system type in a field
>> > all by itself called "fstype". You can just leave it as the hex magic
>> > number prepended with 0x and user space can do the lookup from there,
>> >
>> > It might be simplest to just apply a corrective patch over top of this one
>> > so that you don't have to muck about with git branches and commit
>> > messages.
>>
>> A quick note on the "corrective patch": given we are just days away
>> from the merge window opening, it is *way* to late for something like
>> that, at this point the only options are to leave it as-is or
>> yank/revert and make another pass during the next development phase.
>
> Then yank it. I think that is overreacting but given the options you presented
> its the only one that avoids changing a critical field format.

It's not overreacting Steve, there is simply no way we can test and
adequately soak new changes in the few days we have left.  Event
yanks/reverts carry a risk at this stage, but I consider that the less
risky option for these patches.  Neither is a great option, and that
is why I'm rather annoyed.

>> As for the objection itself: ungh.  There is really no good reason why
>> you couldn't have seen this in the *several* *months* prior to this;
>> Richard wrote a nice patch description which *included* sample audit
>> events, and you were involved in discussions regarding this patchset.
>> To say I'm disappointed would be an understatement.
>
> I am also disappointed to find that we are modifying a searchable field that
> has been defined since 2005. The "name" field is very important. It's used in
> quite a few reports, its used in the text format, it's searchable, and we have
> a dictionary that defines exactly what it is. Fields that are searchable and
> used in common reports cannot be changed without a whole lot of coordination.
> I'm also disappointed to have to point out that new information should go in
> its own field. I thought this was common knowledge. In any event, it was
> caught and problems can be avoided.

There are plenty of things to say about the above comment, but in the
interest of brevity I'm just going to leave it at the assumptions and
inflexibility in your audit userspace continue to amaze me in all the
worst ways.  Regardless, as you say, the problem can likely be avoided
this time.

>> I need to look at the rest of audit/next to see what a mess things
>> would be if I yanked this patch.  I don't expect it to be bad, but
>> taking a look will also give Richard a chance to voice his thoughts;
>> it is his patch after all, it would be nice to see an "OK" from him.
>> Whatever we do, it needs to happen by the of the day today (Thursday,
>> November 9th) as we need time to build and test the revised patches.

FWIW, I just went through audit/next and it looks like yanking patch
1/2 isn't going to be too painful; I'm waiting on the build to finish
now.  Also, as a FYI, Richard's 2/2 filtering patch is going to remain
in audit/next as that appears unrelated to the pathname objection,
applies cleanly, and still offers value.

-- 
paul moore
www.paul-moore.com

Re: [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents

[Paul Moore]

On Thu, Nov 9, 2017 at 10:31 AM, Steve Grubb  wrote:
> On Thursday, November 9, 2017 10:18:10 AM EST Paul Moore wrote:
>> On Wed, Nov 8, 2017 at 6:29 PM, Steve Grubb  wrote:

...

>> > Late reply...but I just noticed that this changes the format of the "name"
>> > field - which is undesirable. Please put the file system type in a field
>> > all by itself called "fstype". You can just leave it as the hex magic
>> > number prepended with 0x and user space can do the lookup from there,
>> >
>> > It might be simplest to just apply a corrective patch over top of this one
>> > so that you don't have to muck about with git branches and commit
>> > messages.
>>
>> A quick note on the "corrective patch": given we are just days away
>> from the merge window opening, it is *way* to late for something like
>> that, at this point the only options are to leave it as-is or
>> yank/revert and make another pass during the next development phase.
>
> Then yank it. I think that is overreacting but given the options you presented
> its the only one that avoids changing a critical field format.

It's not overreacting Steve, there is simply no way we can test and
adequately soak new changes in the few days we have left.  Event
yanks/reverts carry a risk at this stage, but I consider that the less
risky option for these patches.  Neither is a great option, and that
is why I'm rather annoyed.

>> As for the objection itself: ungh.  There is really no good reason why
>> you couldn't have seen this in the *several* *months* prior to this;
>> Richard wrote a nice patch description which *included* sample audit
>> events, and you were involved in discussions regarding this patchset.
>> To say I'm disappointed would be an understatement.
>
> I am also disappointed to find that we are modifying a searchable field that
> has been defined since 2005. The "name" field is very important. It's used in
> quite a few reports, its used in the text format, it's searchable, and we have
> a dictionary that defines exactly what it is. Fields that are searchable and
> used in common reports cannot be changed without a whole lot of coordination.
> I'm also disappointed to have to point out that new information should go in
> its own field. I thought this was common knowledge. In any event, it was
> caught and problems can be avoided.

There are plenty of things to say about the above comment, but in the
interest of brevity I'm just going to leave it at the assumptions and
inflexibility in your audit userspace continue to amaze me in all the
worst ways.  Regardless, as you say, the problem can likely be avoided
this time.

>> I need to look at the rest of audit/next to see what a mess things
>> would be if I yanked this patch.  I don't expect it to be bad, but
>> taking a look will also give Richard a chance to voice his thoughts;
>> it is his patch after all, it would be nice to see an "OK" from him.
>> Whatever we do, it needs to happen by the of the day today (Thursday,
>> November 9th) as we need time to build and test the revised patches.

FWIW, I just went through audit/next and it looks like yanking patch
1/2 isn't going to be too painful; I'm waiting on the build to finish
now.  Also, as a FYI, Richard's 2/2 filtering patch is going to remain
in audit/next as that appears unrelated to the pathname objection,
applies cleanly, and still offers value.

-- 
paul moore
www.paul-moore.com

Re: [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents

[Steve Grubb]

On Thursday, November 9, 2017 10:18:10 AM EST Paul Moore wrote:
> On Wed, Nov 8, 2017 at 6:29 PM, Steve Grubb  wrote:
> > On Wednesday, September 20, 2017 12:52:32 PM EST Paul Moore wrote:
> >> On Wed, Aug 23, 2017 at 7:03 AM, Richard Guy Briggs  
wrote:
> >> > Tracefs or debugfs were causing hundreds to thousands of null PATH
> >> > records to be associated with the init_module and finit_module SYSCALL
> >> > records on a few modules when the following rule was in place for
> >> > 
> >> > startup:
> >> > -a always,exit -F arch=x86_64 -S init_module -F key=mod-load
> >> > 
> >> > This happens because the parent inode is not found in the task's
> >> > audit_names list and hence treats it as anonymous.  This gives us no
> >> > information other than a numerical device number that may no longer be
> >> > visible upon log inspeciton, and an inode number.
> >> > 
> >> > Fill in the filesystem type, filesystem magic number and full pathname
> >> > from the filesystem mount point on previously null PATH records from
> >> > entries that have an anonymous parent from the child dentry using
> >> > dentry_path_raw().
> > 
> > Late reply...but I just noticed that this changes the format of the "name"
> > field - which is undesirable. Please put the file system type in a field
> > all by itself called "fstype". You can just leave it as the hex magic
> > number prepended with 0x and user space can do the lookup from there,
> > 
> > It might be simplest to just apply a corrective patch over top of this one
> > so that you don't have to muck about with git branches and commit
> > messages.
>
> A quick note on the "corrective patch": given we are just days away
> from the merge window opening, it is *way* to late for something like
> that, at this point the only options are to leave it as-is or
> yank/revert and make another pass during the next development phase.

Then yank it. I think that is overreacting but given the options you presented 
its the only one that avoids changing a critical field format.
 
> As for the objection itself: ungh.  There is really no good reason why
> you couldn't have seen this in the *several* *months* prior to this;
> Richard wrote a nice patch description which *included* sample audit
> events, and you were involved in discussions regarding this patchset.
> To say I'm disappointed would be an understatement.

I am also disappointed to find that we are modifying a searchable field that 
has been defined since 2005. The "name" field is very important. It's used in 
quite a few reports, its used in the text format, it's searchable, and we have 
a dictionary that defines exactly what it is. Fields that are searchable and 
used in common reports cannot be changed without a whole lot of coordination. 
I'm also disappointed to have to point out that new information should go in 
its own field. I thought this was common knowledge. In any event, it was 
caught and problems can be avoided.

-Steve

> I need to look at the rest of audit/next to see what a mess things
> would be if I yanked this patch.  I don't expect it to be bad, but
> taking a look will also give Richard a chance to voice his thoughts;
> it is his patch after all, it would be nice to see an "OK" from him.
> Whatever we do, it needs to happen by the of the day today (Thursday,
> November 9th) as we need time to build and test the revised patches.

Re: [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents

[Steve Grubb]

On Thursday, November 9, 2017 10:18:10 AM EST Paul Moore wrote:
> On Wed, Nov 8, 2017 at 6:29 PM, Steve Grubb  wrote:
> > On Wednesday, September 20, 2017 12:52:32 PM EST Paul Moore wrote:
> >> On Wed, Aug 23, 2017 at 7:03 AM, Richard Guy Briggs  
wrote:
> >> > Tracefs or debugfs were causing hundreds to thousands of null PATH
> >> > records to be associated with the init_module and finit_module SYSCALL
> >> > records on a few modules when the following rule was in place for
> >> > 
> >> > startup:
> >> > -a always,exit -F arch=x86_64 -S init_module -F key=mod-load
> >> > 
> >> > This happens because the parent inode is not found in the task's
> >> > audit_names list and hence treats it as anonymous.  This gives us no
> >> > information other than a numerical device number that may no longer be
> >> > visible upon log inspeciton, and an inode number.
> >> > 
> >> > Fill in the filesystem type, filesystem magic number and full pathname
> >> > from the filesystem mount point on previously null PATH records from
> >> > entries that have an anonymous parent from the child dentry using
> >> > dentry_path_raw().
> > 
> > Late reply...but I just noticed that this changes the format of the "name"
> > field - which is undesirable. Please put the file system type in a field
> > all by itself called "fstype". You can just leave it as the hex magic
> > number prepended with 0x and user space can do the lookup from there,
> > 
> > It might be simplest to just apply a corrective patch over top of this one
> > so that you don't have to muck about with git branches and commit
> > messages.
>
> A quick note on the "corrective patch": given we are just days away
> from the merge window opening, it is *way* to late for something like
> that, at this point the only options are to leave it as-is or
> yank/revert and make another pass during the next development phase.

Then yank it. I think that is overreacting but given the options you presented 
its the only one that avoids changing a critical field format.
 
> As for the objection itself: ungh.  There is really no good reason why
> you couldn't have seen this in the *several* *months* prior to this;
> Richard wrote a nice patch description which *included* sample audit
> events, and you were involved in discussions regarding this patchset.
> To say I'm disappointed would be an understatement.

I am also disappointed to find that we are modifying a searchable field that 
has been defined since 2005. The "name" field is very important. It's used in 
quite a few reports, its used in the text format, it's searchable, and we have 
a dictionary that defines exactly what it is. Fields that are searchable and 
used in common reports cannot be changed without a whole lot of coordination. 
I'm also disappointed to have to point out that new information should go in 
its own field. I thought this was common knowledge. In any event, it was 
caught and problems can be avoided.

-Steve

> I need to look at the rest of audit/next to see what a mess things
> would be if I yanked this patch.  I don't expect it to be bad, but
> taking a look will also give Richard a chance to voice his thoughts;
> it is his patch after all, it would be nice to see an "OK" from him.
> Whatever we do, it needs to happen by the of the day today (Thursday,
> November 9th) as we need time to build and test the revised patches.

Re: [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents

[Paul Moore]

On Wed, Nov 8, 2017 at 6:29 PM, Steve Grubb  wrote:
> On Wednesday, September 20, 2017 12:52:32 PM EST Paul Moore wrote:
>> On Wed, Aug 23, 2017 at 7:03 AM, Richard Guy Briggs  wrote:
>> > Tracefs or debugfs were causing hundreds to thousands of null PATH
>> > records to be associated with the init_module and finit_module SYSCALL
>> > records on a few modules when the following rule was in place for
>> >
>> > startup:
>> > -a always,exit -F arch=x86_64 -S init_module -F key=mod-load
>> >
>> > This happens because the parent inode is not found in the task's
>> > audit_names list and hence treats it as anonymous.  This gives us no
>> > information other than a numerical device number that may no longer be
>> > visible upon log inspeciton, and an inode number.
>> >
>> > Fill in the filesystem type, filesystem magic number and full pathname
>> > from the filesystem mount point on previously null PATH records from
>> > entries that have an anonymous parent from the child dentry using
>> > dentry_path_raw().
>
> Late reply...but I just noticed that this changes the format of the "name"
> field - which is undesirable. Please put the file system type in a field all
> by itself called "fstype". You can just leave it as the hex magic number
> prepended with 0x and user space can do the lookup from there,
>
> It might be simplest to just apply a corrective patch over top of this one so
> that you don't have to muck about with git branches and commit messages.

A quick note on the "corrective patch": given we are just days away
from the merge window opening, it is *way* to late for something like
that, at this point the only options are to leave it as-is or
yank/revert and make another pass during the next development phase.

As for the objection itself: ungh.  There is really no good reason why
you couldn't have seen this in the *several* *months* prior to this;
Richard wrote a nice patch description which *included* sample audit
events, and you were involved in discussions regarding this patchset.
To say I'm disappointed would be an understatement.

I need to look at the rest of audit/next to see what a mess things
would be if I yanked this patch.  I don't expect it to be bad, but
taking a look will also give Richard a chance to voice his thoughts;
it is his patch after all, it would be nice to see an "OK" from him.
Whatever we do, it needs to happen by the of the day today (Thursday,
November 9th) as we need time to build and test the revised patches.

-- 
paul moore
www.paul-moore.com

Re: [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents

[Paul Moore]

On Wed, Nov 8, 2017 at 6:29 PM, Steve Grubb  wrote:
> On Wednesday, September 20, 2017 12:52:32 PM EST Paul Moore wrote:
>> On Wed, Aug 23, 2017 at 7:03 AM, Richard Guy Briggs  wrote:
>> > Tracefs or debugfs were causing hundreds to thousands of null PATH
>> > records to be associated with the init_module and finit_module SYSCALL
>> > records on a few modules when the following rule was in place for
>> >
>> > startup:
>> > -a always,exit -F arch=x86_64 -S init_module -F key=mod-load
>> >
>> > This happens because the parent inode is not found in the task's
>> > audit_names list and hence treats it as anonymous.  This gives us no
>> > information other than a numerical device number that may no longer be
>> > visible upon log inspeciton, and an inode number.
>> >
>> > Fill in the filesystem type, filesystem magic number and full pathname
>> > from the filesystem mount point on previously null PATH records from
>> > entries that have an anonymous parent from the child dentry using
>> > dentry_path_raw().
>
> Late reply...but I just noticed that this changes the format of the "name"
> field - which is undesirable. Please put the file system type in a field all
> by itself called "fstype". You can just leave it as the hex magic number
> prepended with 0x and user space can do the lookup from there,
>
> It might be simplest to just apply a corrective patch over top of this one so
> that you don't have to muck about with git branches and commit messages.

A quick note on the "corrective patch": given we are just days away
from the merge window opening, it is *way* to late for something like
that, at this point the only options are to leave it as-is or
yank/revert and make another pass during the next development phase.

As for the objection itself: ungh.  There is really no good reason why
you couldn't have seen this in the *several* *months* prior to this;
Richard wrote a nice patch description which *included* sample audit
events, and you were involved in discussions regarding this patchset.
To say I'm disappointed would be an understatement.

I need to look at the rest of audit/next to see what a mess things
would be if I yanked this patch.  I don't expect it to be bad, but
taking a look will also give Richard a chance to voice his thoughts;
it is his patch after all, it would be nice to see an "OK" from him.
Whatever we do, it needs to happen by the of the day today (Thursday,
November 9th) as we need time to build and test the revised patches.

-- 
paul moore
www.paul-moore.com

Re: [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents

[Steve Grubb]

On Wednesday, September 20, 2017 12:52:32 PM EST Paul Moore wrote:
> On Wed, Aug 23, 2017 at 7:03 AM, Richard Guy Briggs  wrote:
> > Tracefs or debugfs were causing hundreds to thousands of null PATH
> > records to be associated with the init_module and finit_module SYSCALL
> > records on a few modules when the following rule was in place for
> > 
> > startup:
> > -a always,exit -F arch=x86_64 -S init_module -F key=mod-load
> > 
> > This happens because the parent inode is not found in the task's
> > audit_names list and hence treats it as anonymous.  This gives us no
> > information other than a numerical device number that may no longer be
> > visible upon log inspeciton, and an inode number.
> > 
> > Fill in the filesystem type, filesystem magic number and full pathname
> > from the filesystem mount point on previously null PATH records from
> > entries that have an anonymous parent from the child dentry using
> > dentry_path_raw().

Late reply...but I just noticed that this changes the format of the "name" 
field - which is undesirable. Please put the file system type in a field all 
by itself called "fstype". You can just leave it as the hex magic number 
prepended with 0x and user space can do the lookup from there,

It might be simplest to just apply a corrective patch over top of this one so 
that you don't have to muck about with git branches and commit messages.

-Steve
 
> > Make the dentry argument of __audit_inode_child() non-const so that we
> > can take a reference to it in the case of an anonymous parent with
> > dget() and dget_parent() to be able to later print a partial path from
> > the host filesystem rather than null.
> > 
> > Since all we are given is an inode of the parent and the dentry of the
> > child, finding the path from the mount point to the root of the
> > filesystem is more challenging that would involve searching all
> > vfsmounts from "/" until a matching dentry is found for that
> > filesystem's root dentry.  Even if one is found, there may be more than
> > one mount point.  At this point the gain seems marginal since
> > knowing the filesystem type and path are a significant help in tracking
> > down the source of the PATH records and being to address them.
> > 
> > Sample output:
> > type=PROCTITLE msg=audit(1488317694.446:143):
> > proctitle=2F7362696E2F6D6F6470726F6265002D71002D2D006E66737634 type=PATH
> > msg=audit(1488317694.446:143): item=797
> > name=tracefs(74726163):/events/nfs4/nfs4_setclientid/format inode=15969
> > dev=00:09 mode=0100444 ouid=0 ogid=0 rdev=00:00
> > obj=system_u:object_r:tracefs_t:s0 nametype=CREATE type=PATH
> > msg=audit(1488317694.446:143): item=796
> > name=tracefs(74726163):/events/nfs4/nfs4_setclientid inode=15964
> > dev=00:09 mode=040755 ouid=0 ogid=0 rdev=00:00
> > obj=system_u:object_r:tracefs_t:s0 nametype=PARENT ...
> > type=PATH msg=audit(1488317694.446:143): item=1
> > name=tracefs(74726163):/events/nfs4 inode=15571 dev=00:09 mode=040755
> > ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0
> > nametype=CREATE type=PATH msg=audit(1488317694.446:143): item=0
> > name=tracefs(74726163):/events inode=119 dev=00:09 mode=040755 ouid=0
> > ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=PARENT
> > type=UNKNOWN[1330] msg=audit(1488317694.446:143): name="nfsv4"
> > type=SYSCALL msg=audit(1488317694.446:143): arch=c03e syscall=313
> > success=yes exit=0 a0=1 a1=55d5a35ce106 a2=0 a3=1 items=798 ppid=6
> > pid=528 auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0
> > fsgid=0 tty=(none) ses=4294967295 comm="modprobe" exe="/usr/bin/kmod"
> > subj=system_u:system_r:insmod_t:s0 key="mod-load"
> > 
> > See: https://github.com/linux-audit/audit-kernel/issues/8
> > Test case: https://github.com/linux-audit/audit-testsuite/issues/42
> > 
> > Signed-off-by: Richard Guy Briggs 
> > 
> > ---
> > 
> > v3:
> >   fix audit_buffer leak and dname error allocation leak audit_log_name
> >   only put audit_name->dentry if it is being replaced
> > 
> > v2:
> >   minor cosmetic changes and support fs filter patch
> > 
> > ---
> > 
> >  include/linux/audit.h |8 
> >  kernel/audit.c|   19 +++
> >  kernel/audit.h|1 +
> >  kernel/auditsc.c  |8 +++-
> >  4 files changed, 31 insertions(+), 5 deletions(-)
> 
> ...
> 
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 59e60e0..d6e6e4e 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -72,6 +72,7 @@
> > 
> >  #include 
> >  #include 
> >  #include 
> > 
> > +#include 
> > 
> >  #include "audit.h"
> > 
> > @@ -2047,6 +2048,10 @@ void audit_copy_inode(struct audit_names *name,
> > const struct dentry *dentry,> 
> > name->gid   = inode->i_gid;
> > name->rdev  = inode->i_rdev;
> > security_inode_getsecid(inode, >osid);
> > 
> > +   if (name->dentry) {
> > +   dput(name->dentry);
> > +   name->dentry = 

Re: [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents

[Steve Grubb]

On Wednesday, September 20, 2017 12:52:32 PM EST Paul Moore wrote:
> On Wed, Aug 23, 2017 at 7:03 AM, Richard Guy Briggs  wrote:
> > Tracefs or debugfs were causing hundreds to thousands of null PATH
> > records to be associated with the init_module and finit_module SYSCALL
> > records on a few modules when the following rule was in place for
> > 
> > startup:
> > -a always,exit -F arch=x86_64 -S init_module -F key=mod-load
> > 
> > This happens because the parent inode is not found in the task's
> > audit_names list and hence treats it as anonymous.  This gives us no
> > information other than a numerical device number that may no longer be
> > visible upon log inspeciton, and an inode number.
> > 
> > Fill in the filesystem type, filesystem magic number and full pathname
> > from the filesystem mount point on previously null PATH records from
> > entries that have an anonymous parent from the child dentry using
> > dentry_path_raw().

Late reply...but I just noticed that this changes the format of the "name" 
field - which is undesirable. Please put the file system type in a field all 
by itself called "fstype". You can just leave it as the hex magic number 
prepended with 0x and user space can do the lookup from there,

It might be simplest to just apply a corrective patch over top of this one so 
that you don't have to muck about with git branches and commit messages.

-Steve
 
> > Make the dentry argument of __audit_inode_child() non-const so that we
> > can take a reference to it in the case of an anonymous parent with
> > dget() and dget_parent() to be able to later print a partial path from
> > the host filesystem rather than null.
> > 
> > Since all we are given is an inode of the parent and the dentry of the
> > child, finding the path from the mount point to the root of the
> > filesystem is more challenging that would involve searching all
> > vfsmounts from "/" until a matching dentry is found for that
> > filesystem's root dentry.  Even if one is found, there may be more than
> > one mount point.  At this point the gain seems marginal since
> > knowing the filesystem type and path are a significant help in tracking
> > down the source of the PATH records and being to address them.
> > 
> > Sample output:
> > type=PROCTITLE msg=audit(1488317694.446:143):
> > proctitle=2F7362696E2F6D6F6470726F6265002D71002D2D006E66737634 type=PATH
> > msg=audit(1488317694.446:143): item=797
> > name=tracefs(74726163):/events/nfs4/nfs4_setclientid/format inode=15969
> > dev=00:09 mode=0100444 ouid=0 ogid=0 rdev=00:00
> > obj=system_u:object_r:tracefs_t:s0 nametype=CREATE type=PATH
> > msg=audit(1488317694.446:143): item=796
> > name=tracefs(74726163):/events/nfs4/nfs4_setclientid inode=15964
> > dev=00:09 mode=040755 ouid=0 ogid=0 rdev=00:00
> > obj=system_u:object_r:tracefs_t:s0 nametype=PARENT ...
> > type=PATH msg=audit(1488317694.446:143): item=1
> > name=tracefs(74726163):/events/nfs4 inode=15571 dev=00:09 mode=040755
> > ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0
> > nametype=CREATE type=PATH msg=audit(1488317694.446:143): item=0
> > name=tracefs(74726163):/events inode=119 dev=00:09 mode=040755 ouid=0
> > ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=PARENT
> > type=UNKNOWN[1330] msg=audit(1488317694.446:143): name="nfsv4"
> > type=SYSCALL msg=audit(1488317694.446:143): arch=c03e syscall=313
> > success=yes exit=0 a0=1 a1=55d5a35ce106 a2=0 a3=1 items=798 ppid=6
> > pid=528 auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0
> > fsgid=0 tty=(none) ses=4294967295 comm="modprobe" exe="/usr/bin/kmod"
> > subj=system_u:system_r:insmod_t:s0 key="mod-load"
> > 
> > See: https://github.com/linux-audit/audit-kernel/issues/8
> > Test case: https://github.com/linux-audit/audit-testsuite/issues/42
> > 
> > Signed-off-by: Richard Guy Briggs 
> > 
> > ---
> > 
> > v3:
> >   fix audit_buffer leak and dname error allocation leak audit_log_name
> >   only put audit_name->dentry if it is being replaced
> > 
> > v2:
> >   minor cosmetic changes and support fs filter patch
> > 
> > ---
> > 
> >  include/linux/audit.h |8 
> >  kernel/audit.c|   19 +++
> >  kernel/audit.h|1 +
> >  kernel/auditsc.c  |8 +++-
> >  4 files changed, 31 insertions(+), 5 deletions(-)
> 
> ...
> 
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 59e60e0..d6e6e4e 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -72,6 +72,7 @@
> > 
> >  #include 
> >  #include 
> >  #include 
> > 
> > +#include 
> > 
> >  #include "audit.h"
> > 
> > @@ -2047,6 +2048,10 @@ void audit_copy_inode(struct audit_names *name,
> > const struct dentry *dentry,> 
> > name->gid   = inode->i_gid;
> > name->rdev  = inode->i_rdev;
> > security_inode_getsecid(inode, >osid);
> > 
> > +   if (name->dentry) {
> > +   dput(name->dentry);
> > +   name->dentry = NULL;
> > +   }
> > 
> > 

Re: [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents

[Richard Guy Briggs]

On 2017-09-20 16:52, Paul Moore wrote:
> On Wed, Aug 23, 2017 at 7:03 AM, Richard Guy Briggs  wrote:
> > Tracefs or debugfs were causing hundreds to thousands of null PATH
> > records to be associated with the init_module and finit_module SYSCALL
> > records on a few modules when the following rule was in place for
> > startup:
> > -a always,exit -F arch=x86_64 -S init_module -F key=mod-load
> >
> > This happens because the parent inode is not found in the task's
> > audit_names list and hence treats it as anonymous.  This gives us no
> > information other than a numerical device number that may no longer be
> > visible upon log inspeciton, and an inode number.
> >
> > Fill in the filesystem type, filesystem magic number and full pathname
> > from the filesystem mount point on previously null PATH records from
> > entries that have an anonymous parent from the child dentry using
> > dentry_path_raw().
> >
> > Make the dentry argument of __audit_inode_child() non-const so that we
> > can take a reference to it in the case of an anonymous parent with
> > dget() and dget_parent() to be able to later print a partial path from
> > the host filesystem rather than null.
> >
> > Since all we are given is an inode of the parent and the dentry of the
> > child, finding the path from the mount point to the root of the
> > filesystem is more challenging that would involve searching all
> > vfsmounts from "/" until a matching dentry is found for that
> > filesystem's root dentry.  Even if one is found, there may be more than
> > one mount point.  At this point the gain seems marginal since
> > knowing the filesystem type and path are a significant help in tracking
> > down the source of the PATH records and being to address them.
> >
> > Sample output:
> > type=PROCTITLE msg=audit(1488317694.446:143): 
> > proctitle=2F7362696E2F6D6F6470726F6265002D71002D2D006E66737634
> > type=PATH msg=audit(1488317694.446:143): item=797 
> > name=tracefs(74726163):/events/nfs4/nfs4_setclientid/format inode=15969 
> > dev=00:09 mode=0100444 ouid=0 ogid=0 rdev=00:00 
> > obj=system_u:object_r:tracefs_t:s0 nametype=CREATE
> > type=PATH msg=audit(1488317694.446:143): item=796 
> > name=tracefs(74726163):/events/nfs4/nfs4_setclientid inode=15964 dev=00:09 
> > mode=040755 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 
> > nametype=PARENT
> > ...
> > type=PATH msg=audit(1488317694.446:143): item=1 
> > name=tracefs(74726163):/events/nfs4 inode=15571 dev=00:09 mode=040755 
> > ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=CREATE
> > type=PATH msg=audit(1488317694.446:143): item=0 
> > name=tracefs(74726163):/events inode=119 dev=00:09 mode=040755 ouid=0 
> > ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=PARENT
> > type=UNKNOWN[1330] msg=audit(1488317694.446:143): name="nfsv4"
> > type=SYSCALL msg=audit(1488317694.446:143): arch=c03e syscall=313 
> > success=yes exit=0 a0=1 a1=55d5a35ce106 a2=0 a3=1 items=798 ppid=6 pid=528 
> > auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 
> > tty=(none) ses=4294967295 comm="modprobe" exe="/usr/bin/kmod" 
> > subj=system_u:system_r:insmod_t:s0 key="mod-load"
> >
> > See: https://github.com/linux-audit/audit-kernel/issues/8
> > Test case: https://github.com/linux-audit/audit-testsuite/issues/42
> >
> > Signed-off-by: Richard Guy Briggs 
> >
> > ---
> > v3:
> >   fix audit_buffer leak and dname error allocation leak audit_log_name
> >   only put audit_name->dentry if it is being replaced
> >
> > v2:
> >   minor cosmetic changes and support fs filter patch
> > ---
> >  include/linux/audit.h |8 
> >  kernel/audit.c|   19 +++
> >  kernel/audit.h|1 +
> >  kernel/auditsc.c  |8 +++-
> >  4 files changed, 31 insertions(+), 5 deletions(-)
> 
> ...
> 
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 59e60e0..d6e6e4e 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -72,6 +72,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >
> >  #include "audit.h"
> >
> > @@ -2047,6 +2048,10 @@ void audit_copy_inode(struct audit_names *name, 
> > const struct dentry *dentry,
> > name->gid   = inode->i_gid;
> > name->rdev  = inode->i_rdev;
> > security_inode_getsecid(inode, >osid);
> > +   if (name->dentry) {
> > +   dput(name->dentry);
> > +   name->dentry = NULL;
> > +   }
> > audit_copy_fcaps(name, dentry);
> >  }
> >
> > @@ -2088,6 +2093,20 @@ void audit_log_name(struct audit_context *context, 
> > struct audit_names *n,
> > audit_log_n_untrustedstring(ab, n->name->name,
> > n->name_len);
> > }
> > +   } else if (n->dentry) {
> > +   char *fullpath;
> > +   const char *fullpathp = NULL;
> > +
> > +   fullpath = 

Re: [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents

[Richard Guy Briggs]

On 2017-09-20 16:52, Paul Moore wrote:
> On Wed, Aug 23, 2017 at 7:03 AM, Richard Guy Briggs  wrote:
> > Tracefs or debugfs were causing hundreds to thousands of null PATH
> > records to be associated with the init_module and finit_module SYSCALL
> > records on a few modules when the following rule was in place for
> > startup:
> > -a always,exit -F arch=x86_64 -S init_module -F key=mod-load
> >
> > This happens because the parent inode is not found in the task's
> > audit_names list and hence treats it as anonymous.  This gives us no
> > information other than a numerical device number that may no longer be
> > visible upon log inspeciton, and an inode number.
> >
> > Fill in the filesystem type, filesystem magic number and full pathname
> > from the filesystem mount point on previously null PATH records from
> > entries that have an anonymous parent from the child dentry using
> > dentry_path_raw().
> >
> > Make the dentry argument of __audit_inode_child() non-const so that we
> > can take a reference to it in the case of an anonymous parent with
> > dget() and dget_parent() to be able to later print a partial path from
> > the host filesystem rather than null.
> >
> > Since all we are given is an inode of the parent and the dentry of the
> > child, finding the path from the mount point to the root of the
> > filesystem is more challenging that would involve searching all
> > vfsmounts from "/" until a matching dentry is found for that
> > filesystem's root dentry.  Even if one is found, there may be more than
> > one mount point.  At this point the gain seems marginal since
> > knowing the filesystem type and path are a significant help in tracking
> > down the source of the PATH records and being to address them.
> >
> > Sample output:
> > type=PROCTITLE msg=audit(1488317694.446:143): 
> > proctitle=2F7362696E2F6D6F6470726F6265002D71002D2D006E66737634
> > type=PATH msg=audit(1488317694.446:143): item=797 
> > name=tracefs(74726163):/events/nfs4/nfs4_setclientid/format inode=15969 
> > dev=00:09 mode=0100444 ouid=0 ogid=0 rdev=00:00 
> > obj=system_u:object_r:tracefs_t:s0 nametype=CREATE
> > type=PATH msg=audit(1488317694.446:143): item=796 
> > name=tracefs(74726163):/events/nfs4/nfs4_setclientid inode=15964 dev=00:09 
> > mode=040755 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 
> > nametype=PARENT
> > ...
> > type=PATH msg=audit(1488317694.446:143): item=1 
> > name=tracefs(74726163):/events/nfs4 inode=15571 dev=00:09 mode=040755 
> > ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=CREATE
> > type=PATH msg=audit(1488317694.446:143): item=0 
> > name=tracefs(74726163):/events inode=119 dev=00:09 mode=040755 ouid=0 
> > ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=PARENT
> > type=UNKNOWN[1330] msg=audit(1488317694.446:143): name="nfsv4"
> > type=SYSCALL msg=audit(1488317694.446:143): arch=c03e syscall=313 
> > success=yes exit=0 a0=1 a1=55d5a35ce106 a2=0 a3=1 items=798 ppid=6 pid=528 
> > auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 
> > tty=(none) ses=4294967295 comm="modprobe" exe="/usr/bin/kmod" 
> > subj=system_u:system_r:insmod_t:s0 key="mod-load"
> >
> > See: https://github.com/linux-audit/audit-kernel/issues/8
> > Test case: https://github.com/linux-audit/audit-testsuite/issues/42
> >
> > Signed-off-by: Richard Guy Briggs 
> >
> > ---
> > v3:
> >   fix audit_buffer leak and dname error allocation leak audit_log_name
> >   only put audit_name->dentry if it is being replaced
> >
> > v2:
> >   minor cosmetic changes and support fs filter patch
> > ---
> >  include/linux/audit.h |8 
> >  kernel/audit.c|   19 +++
> >  kernel/audit.h|1 +
> >  kernel/auditsc.c  |8 +++-
> >  4 files changed, 31 insertions(+), 5 deletions(-)
> 
> ...
> 
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 59e60e0..d6e6e4e 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -72,6 +72,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >
> >  #include "audit.h"
> >
> > @@ -2047,6 +2048,10 @@ void audit_copy_inode(struct audit_names *name, 
> > const struct dentry *dentry,
> > name->gid   = inode->i_gid;
> > name->rdev  = inode->i_rdev;
> > security_inode_getsecid(inode, >osid);
> > +   if (name->dentry) {
> > +   dput(name->dentry);
> > +   name->dentry = NULL;
> > +   }
> > audit_copy_fcaps(name, dentry);
> >  }
> >
> > @@ -2088,6 +2093,20 @@ void audit_log_name(struct audit_context *context, 
> > struct audit_names *n,
> > audit_log_n_untrustedstring(ab, n->name->name,
> > n->name_len);
> > }
> > +   } else if (n->dentry) {
> > +   char *fullpath;
> > +   const char *fullpathp = NULL;
> > +
> > +   fullpath = kmalloc(PATH_MAX, GFP_KERNEL);
> > +  

Re: [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents

[Richard Guy Briggs]

On 2017-09-20 12:52, Paul Moore wrote:
> On Wed, Aug 23, 2017 at 7:03 AM, Richard Guy Briggs  wrote:
> > Tracefs or debugfs were causing hundreds to thousands of null PATH
> > records to be associated with the init_module and finit_module SYSCALL
> > records on a few modules when the following rule was in place for
> > startup:
> > -a always,exit -F arch=x86_64 -S init_module -F key=mod-load
> >
> > This happens because the parent inode is not found in the task's
> > audit_names list and hence treats it as anonymous.  This gives us no
> > information other than a numerical device number that may no longer be
> > visible upon log inspeciton, and an inode number.
> >
> > Fill in the filesystem type, filesystem magic number and full pathname
> > from the filesystem mount point on previously null PATH records from
> > entries that have an anonymous parent from the child dentry using
> > dentry_path_raw().
> >
> > Make the dentry argument of __audit_inode_child() non-const so that we
> > can take a reference to it in the case of an anonymous parent with
> > dget() and dget_parent() to be able to later print a partial path from
> > the host filesystem rather than null.
> >
> > Since all we are given is an inode of the parent and the dentry of the
> > child, finding the path from the mount point to the root of the
> > filesystem is more challenging that would involve searching all
> > vfsmounts from "/" until a matching dentry is found for that
> > filesystem's root dentry.  Even if one is found, there may be more than
> > one mount point.  At this point the gain seems marginal since
> > knowing the filesystem type and path are a significant help in tracking
> > down the source of the PATH records and being to address them.
> >
> > Sample output:
> > type=PROCTITLE msg=audit(1488317694.446:143): 
> > proctitle=2F7362696E2F6D6F6470726F6265002D71002D2D006E66737634
> > type=PATH msg=audit(1488317694.446:143): item=797 
> > name=tracefs(74726163):/events/nfs4/nfs4_setclientid/format inode=15969 
> > dev=00:09 mode=0100444 ouid=0 ogid=0 rdev=00:00 
> > obj=system_u:object_r:tracefs_t:s0 nametype=CREATE
> > type=PATH msg=audit(1488317694.446:143): item=796 
> > name=tracefs(74726163):/events/nfs4/nfs4_setclientid inode=15964 dev=00:09 
> > mode=040755 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 
> > nametype=PARENT
> > ...
> > type=PATH msg=audit(1488317694.446:143): item=1 
> > name=tracefs(74726163):/events/nfs4 inode=15571 dev=00:09 mode=040755 
> > ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=CREATE
> > type=PATH msg=audit(1488317694.446:143): item=0 
> > name=tracefs(74726163):/events inode=119 dev=00:09 mode=040755 ouid=0 
> > ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=PARENT
> > type=UNKNOWN[1330] msg=audit(1488317694.446:143): name="nfsv4"
> > type=SYSCALL msg=audit(1488317694.446:143): arch=c03e syscall=313 
> > success=yes exit=0 a0=1 a1=55d5a35ce106 a2=0 a3=1 items=798 ppid=6 pid=528 
> > auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 
> > tty=(none) ses=4294967295 comm="modprobe" exe="/usr/bin/kmod" 
> > subj=system_u:system_r:insmod_t:s0 key="mod-load"
> >
> > See: https://github.com/linux-audit/audit-kernel/issues/8
> > Test case: https://github.com/linux-audit/audit-testsuite/issues/42
> >
> > Signed-off-by: Richard Guy Briggs 
> >
> > ---
> > v3:
> >   fix audit_buffer leak and dname error allocation leak audit_log_name
> >   only put audit_name->dentry if it is being replaced
> >
> > v2:
> >   minor cosmetic changes and support fs filter patch
> > ---
> >  include/linux/audit.h |8 
> >  kernel/audit.c|   19 +++
> >  kernel/audit.h|1 +
> >  kernel/auditsc.c  |8 +++-
> >  4 files changed, 31 insertions(+), 5 deletions(-)
> 
> ...
> 
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 59e60e0..d6e6e4e 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -72,6 +72,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >
> >  #include "audit.h"
> >
> > @@ -2047,6 +2048,10 @@ void audit_copy_inode(struct audit_names *name, 
> > const struct dentry *dentry,
> > name->gid   = inode->i_gid;
> > name->rdev  = inode->i_rdev;
> > security_inode_getsecid(inode, >osid);
> > +   if (name->dentry) {
> > +   dput(name->dentry);
> > +   name->dentry = NULL;
> > +   }
> > audit_copy_fcaps(name, dentry);
> >  }
> >
> > @@ -2088,6 +2093,20 @@ void audit_log_name(struct audit_context *context, 
> > struct audit_names *n,
> > audit_log_n_untrustedstring(ab, n->name->name,
> > n->name_len);
> > }
> > +   } else if (n->dentry) {
> > +   char *fullpath;
> > +   const char *fullpathp = NULL;
> > +
> > +   fullpath = 

Re: [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents

[Richard Guy Briggs]

On 2017-09-20 12:52, Paul Moore wrote:
> On Wed, Aug 23, 2017 at 7:03 AM, Richard Guy Briggs  wrote:
> > Tracefs or debugfs were causing hundreds to thousands of null PATH
> > records to be associated with the init_module and finit_module SYSCALL
> > records on a few modules when the following rule was in place for
> > startup:
> > -a always,exit -F arch=x86_64 -S init_module -F key=mod-load
> >
> > This happens because the parent inode is not found in the task's
> > audit_names list and hence treats it as anonymous.  This gives us no
> > information other than a numerical device number that may no longer be
> > visible upon log inspeciton, and an inode number.
> >
> > Fill in the filesystem type, filesystem magic number and full pathname
> > from the filesystem mount point on previously null PATH records from
> > entries that have an anonymous parent from the child dentry using
> > dentry_path_raw().
> >
> > Make the dentry argument of __audit_inode_child() non-const so that we
> > can take a reference to it in the case of an anonymous parent with
> > dget() and dget_parent() to be able to later print a partial path from
> > the host filesystem rather than null.
> >
> > Since all we are given is an inode of the parent and the dentry of the
> > child, finding the path from the mount point to the root of the
> > filesystem is more challenging that would involve searching all
> > vfsmounts from "/" until a matching dentry is found for that
> > filesystem's root dentry.  Even if one is found, there may be more than
> > one mount point.  At this point the gain seems marginal since
> > knowing the filesystem type and path are a significant help in tracking
> > down the source of the PATH records and being to address them.
> >
> > Sample output:
> > type=PROCTITLE msg=audit(1488317694.446:143): 
> > proctitle=2F7362696E2F6D6F6470726F6265002D71002D2D006E66737634
> > type=PATH msg=audit(1488317694.446:143): item=797 
> > name=tracefs(74726163):/events/nfs4/nfs4_setclientid/format inode=15969 
> > dev=00:09 mode=0100444 ouid=0 ogid=0 rdev=00:00 
> > obj=system_u:object_r:tracefs_t:s0 nametype=CREATE
> > type=PATH msg=audit(1488317694.446:143): item=796 
> > name=tracefs(74726163):/events/nfs4/nfs4_setclientid inode=15964 dev=00:09 
> > mode=040755 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 
> > nametype=PARENT
> > ...
> > type=PATH msg=audit(1488317694.446:143): item=1 
> > name=tracefs(74726163):/events/nfs4 inode=15571 dev=00:09 mode=040755 
> > ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=CREATE
> > type=PATH msg=audit(1488317694.446:143): item=0 
> > name=tracefs(74726163):/events inode=119 dev=00:09 mode=040755 ouid=0 
> > ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=PARENT
> > type=UNKNOWN[1330] msg=audit(1488317694.446:143): name="nfsv4"
> > type=SYSCALL msg=audit(1488317694.446:143): arch=c03e syscall=313 
> > success=yes exit=0 a0=1 a1=55d5a35ce106 a2=0 a3=1 items=798 ppid=6 pid=528 
> > auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 
> > tty=(none) ses=4294967295 comm="modprobe" exe="/usr/bin/kmod" 
> > subj=system_u:system_r:insmod_t:s0 key="mod-load"
> >
> > See: https://github.com/linux-audit/audit-kernel/issues/8
> > Test case: https://github.com/linux-audit/audit-testsuite/issues/42
> >
> > Signed-off-by: Richard Guy Briggs 
> >
> > ---
> > v3:
> >   fix audit_buffer leak and dname error allocation leak audit_log_name
> >   only put audit_name->dentry if it is being replaced
> >
> > v2:
> >   minor cosmetic changes and support fs filter patch
> > ---
> >  include/linux/audit.h |8 
> >  kernel/audit.c|   19 +++
> >  kernel/audit.h|1 +
> >  kernel/auditsc.c  |8 +++-
> >  4 files changed, 31 insertions(+), 5 deletions(-)
> 
> ...
> 
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 59e60e0..d6e6e4e 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -72,6 +72,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >
> >  #include "audit.h"
> >
> > @@ -2047,6 +2048,10 @@ void audit_copy_inode(struct audit_names *name, 
> > const struct dentry *dentry,
> > name->gid   = inode->i_gid;
> > name->rdev  = inode->i_rdev;
> > security_inode_getsecid(inode, >osid);
> > +   if (name->dentry) {
> > +   dput(name->dentry);
> > +   name->dentry = NULL;
> > +   }
> > audit_copy_fcaps(name, dentry);
> >  }
> >
> > @@ -2088,6 +2093,20 @@ void audit_log_name(struct audit_context *context, 
> > struct audit_names *n,
> > audit_log_n_untrustedstring(ab, n->name->name,
> > n->name_len);
> > }
> > +   } else if (n->dentry) {
> > +   char *fullpath;
> > +   const char *fullpathp = NULL;
> > +
> > +   fullpath = kmalloc(PATH_MAX, GFP_KERNEL);
> > +  

Re: [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents

[Paul Moore]

On Wed, Aug 23, 2017 at 7:03 AM, Richard Guy Briggs  wrote:
> Tracefs or debugfs were causing hundreds to thousands of null PATH
> records to be associated with the init_module and finit_module SYSCALL
> records on a few modules when the following rule was in place for
> startup:
> -a always,exit -F arch=x86_64 -S init_module -F key=mod-load
>
> This happens because the parent inode is not found in the task's
> audit_names list and hence treats it as anonymous.  This gives us no
> information other than a numerical device number that may no longer be
> visible upon log inspeciton, and an inode number.
>
> Fill in the filesystem type, filesystem magic number and full pathname
> from the filesystem mount point on previously null PATH records from
> entries that have an anonymous parent from the child dentry using
> dentry_path_raw().
>
> Make the dentry argument of __audit_inode_child() non-const so that we
> can take a reference to it in the case of an anonymous parent with
> dget() and dget_parent() to be able to later print a partial path from
> the host filesystem rather than null.
>
> Since all we are given is an inode of the parent and the dentry of the
> child, finding the path from the mount point to the root of the
> filesystem is more challenging that would involve searching all
> vfsmounts from "/" until a matching dentry is found for that
> filesystem's root dentry.  Even if one is found, there may be more than
> one mount point.  At this point the gain seems marginal since
> knowing the filesystem type and path are a significant help in tracking
> down the source of the PATH records and being to address them.
>
> Sample output:
> type=PROCTITLE msg=audit(1488317694.446:143): 
> proctitle=2F7362696E2F6D6F6470726F6265002D71002D2D006E66737634
> type=PATH msg=audit(1488317694.446:143): item=797 
> name=tracefs(74726163):/events/nfs4/nfs4_setclientid/format inode=15969 
> dev=00:09 mode=0100444 ouid=0 ogid=0 rdev=00:00 
> obj=system_u:object_r:tracefs_t:s0 nametype=CREATE
> type=PATH msg=audit(1488317694.446:143): item=796 
> name=tracefs(74726163):/events/nfs4/nfs4_setclientid inode=15964 dev=00:09 
> mode=040755 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 
> nametype=PARENT
> ...
> type=PATH msg=audit(1488317694.446:143): item=1 
> name=tracefs(74726163):/events/nfs4 inode=15571 dev=00:09 mode=040755 ouid=0 
> ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=CREATE
> type=PATH msg=audit(1488317694.446:143): item=0 
> name=tracefs(74726163):/events inode=119 dev=00:09 mode=040755 ouid=0 ogid=0 
> rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=PARENT
> type=UNKNOWN[1330] msg=audit(1488317694.446:143): name="nfsv4"
> type=SYSCALL msg=audit(1488317694.446:143): arch=c03e syscall=313 
> success=yes exit=0 a0=1 a1=55d5a35ce106 a2=0 a3=1 items=798 ppid=6 pid=528 
> auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 
> tty=(none) ses=4294967295 comm="modprobe" exe="/usr/bin/kmod" 
> subj=system_u:system_r:insmod_t:s0 key="mod-load"
>
> See: https://github.com/linux-audit/audit-kernel/issues/8
> Test case: https://github.com/linux-audit/audit-testsuite/issues/42
>
> Signed-off-by: Richard Guy Briggs 
>
> ---
> v3:
>   fix audit_buffer leak and dname error allocation leak audit_log_name
>   only put audit_name->dentry if it is being replaced
>
> v2:
>   minor cosmetic changes and support fs filter patch
> ---
>  include/linux/audit.h |8 
>  kernel/audit.c|   19 +++
>  kernel/audit.h|1 +
>  kernel/auditsc.c  |8 +++-
>  4 files changed, 31 insertions(+), 5 deletions(-)

...

> diff --git a/kernel/audit.c b/kernel/audit.c
> index 59e60e0..d6e6e4e 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -72,6 +72,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  #include "audit.h"
>
> @@ -2047,6 +2048,10 @@ void audit_copy_inode(struct audit_names *name, const 
> struct dentry *dentry,
> name->gid   = inode->i_gid;
> name->rdev  = inode->i_rdev;
> security_inode_getsecid(inode, >osid);
> +   if (name->dentry) {
> +   dput(name->dentry);
> +   name->dentry = NULL;
> +   }
> audit_copy_fcaps(name, dentry);
>  }
>
> @@ -2088,6 +2093,20 @@ void audit_log_name(struct audit_context *context, 
> struct audit_names *n,
> audit_log_n_untrustedstring(ab, n->name->name,
> n->name_len);
> }
> +   } else if (n->dentry) {
> +   char *fullpath;
> +   const char *fullpathp = NULL;
> +
> +   fullpath = kmalloc(PATH_MAX, GFP_KERNEL);
> +   if (fullpath)
> +   fullpathp = dentry_path_raw(n->dentry, fullpath, 
> PATH_MAX);
> +   if (IS_ERR(fullpathp)) {
> +   fullpathp = NULL;
> +   

Re: [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents

[Paul Moore]

On Wed, Aug 23, 2017 at 7:03 AM, Richard Guy Briggs  wrote:
> Tracefs or debugfs were causing hundreds to thousands of null PATH
> records to be associated with the init_module and finit_module SYSCALL
> records on a few modules when the following rule was in place for
> startup:
> -a always,exit -F arch=x86_64 -S init_module -F key=mod-load
>
> This happens because the parent inode is not found in the task's
> audit_names list and hence treats it as anonymous.  This gives us no
> information other than a numerical device number that may no longer be
> visible upon log inspeciton, and an inode number.
>
> Fill in the filesystem type, filesystem magic number and full pathname
> from the filesystem mount point on previously null PATH records from
> entries that have an anonymous parent from the child dentry using
> dentry_path_raw().
>
> Make the dentry argument of __audit_inode_child() non-const so that we
> can take a reference to it in the case of an anonymous parent with
> dget() and dget_parent() to be able to later print a partial path from
> the host filesystem rather than null.
>
> Since all we are given is an inode of the parent and the dentry of the
> child, finding the path from the mount point to the root of the
> filesystem is more challenging that would involve searching all
> vfsmounts from "/" until a matching dentry is found for that
> filesystem's root dentry.  Even if one is found, there may be more than
> one mount point.  At this point the gain seems marginal since
> knowing the filesystem type and path are a significant help in tracking
> down the source of the PATH records and being to address them.
>
> Sample output:
> type=PROCTITLE msg=audit(1488317694.446:143): 
> proctitle=2F7362696E2F6D6F6470726F6265002D71002D2D006E66737634
> type=PATH msg=audit(1488317694.446:143): item=797 
> name=tracefs(74726163):/events/nfs4/nfs4_setclientid/format inode=15969 
> dev=00:09 mode=0100444 ouid=0 ogid=0 rdev=00:00 
> obj=system_u:object_r:tracefs_t:s0 nametype=CREATE
> type=PATH msg=audit(1488317694.446:143): item=796 
> name=tracefs(74726163):/events/nfs4/nfs4_setclientid inode=15964 dev=00:09 
> mode=040755 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 
> nametype=PARENT
> ...
> type=PATH msg=audit(1488317694.446:143): item=1 
> name=tracefs(74726163):/events/nfs4 inode=15571 dev=00:09 mode=040755 ouid=0 
> ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=CREATE
> type=PATH msg=audit(1488317694.446:143): item=0 
> name=tracefs(74726163):/events inode=119 dev=00:09 mode=040755 ouid=0 ogid=0 
> rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=PARENT
> type=UNKNOWN[1330] msg=audit(1488317694.446:143): name="nfsv4"
> type=SYSCALL msg=audit(1488317694.446:143): arch=c03e syscall=313 
> success=yes exit=0 a0=1 a1=55d5a35ce106 a2=0 a3=1 items=798 ppid=6 pid=528 
> auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 
> tty=(none) ses=4294967295 comm="modprobe" exe="/usr/bin/kmod" 
> subj=system_u:system_r:insmod_t:s0 key="mod-load"
>
> See: https://github.com/linux-audit/audit-kernel/issues/8
> Test case: https://github.com/linux-audit/audit-testsuite/issues/42
>
> Signed-off-by: Richard Guy Briggs 
>
> ---
> v3:
>   fix audit_buffer leak and dname error allocation leak audit_log_name
>   only put audit_name->dentry if it is being replaced
>
> v2:
>   minor cosmetic changes and support fs filter patch
> ---
>  include/linux/audit.h |8 
>  kernel/audit.c|   19 +++
>  kernel/audit.h|1 +
>  kernel/auditsc.c  |8 +++-
>  4 files changed, 31 insertions(+), 5 deletions(-)

...

> diff --git a/kernel/audit.c b/kernel/audit.c
> index 59e60e0..d6e6e4e 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -72,6 +72,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  #include "audit.h"
>
> @@ -2047,6 +2048,10 @@ void audit_copy_inode(struct audit_names *name, const 
> struct dentry *dentry,
> name->gid   = inode->i_gid;
> name->rdev  = inode->i_rdev;
> security_inode_getsecid(inode, >osid);
> +   if (name->dentry) {
> +   dput(name->dentry);
> +   name->dentry = NULL;
> +   }
> audit_copy_fcaps(name, dentry);
>  }
>
> @@ -2088,6 +2093,20 @@ void audit_log_name(struct audit_context *context, 
> struct audit_names *n,
> audit_log_n_untrustedstring(ab, n->name->name,
> n->name_len);
> }
> +   } else if (n->dentry) {
> +   char *fullpath;
> +   const char *fullpathp = NULL;
> +
> +   fullpath = kmalloc(PATH_MAX, GFP_KERNEL);
> +   if (fullpath)
> +   fullpathp = dentry_path_raw(n->dentry, fullpath, 
> PATH_MAX);
> +   if (IS_ERR(fullpathp)) {
> +   fullpathp = NULL;
> +   

[PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents

[Richard Guy Briggs]

Tracefs or debugfs were causing hundreds to thousands of null PATH
records to be associated with the init_module and finit_module SYSCALL
records on a few modules when the following rule was in place for
startup:
-a always,exit -F arch=x86_64 -S init_module -F key=mod-load

This happens because the parent inode is not found in the task's
audit_names list and hence treats it as anonymous.  This gives us no
information other than a numerical device number that may no longer be
visible upon log inspeciton, and an inode number.

Fill in the filesystem type, filesystem magic number and full pathname
from the filesystem mount point on previously null PATH records from
entries that have an anonymous parent from the child dentry using
dentry_path_raw().

Make the dentry argument of __audit_inode_child() non-const so that we
can take a reference to it in the case of an anonymous parent with
dget() and dget_parent() to be able to later print a partial path from
the host filesystem rather than null.

Since all we are given is an inode of the parent and the dentry of the
child, finding the path from the mount point to the root of the
filesystem is more challenging that would involve searching all
vfsmounts from "/" until a matching dentry is found for that
filesystem's root dentry.  Even if one is found, there may be more than
one mount point.  At this point the gain seems marginal since
knowing the filesystem type and path are a significant help in tracking
down the source of the PATH records and being to address them.

Sample output:
type=PROCTITLE msg=audit(1488317694.446:143): 
proctitle=2F7362696E2F6D6F6470726F6265002D71002D2D006E66737634
type=PATH msg=audit(1488317694.446:143): item=797 
name=tracefs(74726163):/events/nfs4/nfs4_setclientid/format inode=15969 
dev=00:09 mode=0100444 ouid=0 ogid=0 rdev=00:00 
obj=system_u:object_r:tracefs_t:s0 nametype=CREATE
type=PATH msg=audit(1488317694.446:143): item=796 
name=tracefs(74726163):/events/nfs4/nfs4_setclientid inode=15964 dev=00:09 
mode=040755 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 
nametype=PARENT
...
type=PATH msg=audit(1488317694.446:143): item=1 
name=tracefs(74726163):/events/nfs4 inode=15571 dev=00:09 mode=040755 ouid=0 
ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=CREATE
type=PATH msg=audit(1488317694.446:143): item=0 name=tracefs(74726163):/events 
inode=119 dev=00:09 mode=040755 ouid=0 ogid=0 rdev=00:00 
obj=system_u:object_r:tracefs_t:s0 nametype=PARENT
type=UNKNOWN[1330] msg=audit(1488317694.446:143): name="nfsv4"
type=SYSCALL msg=audit(1488317694.446:143): arch=c03e syscall=313 
success=yes exit=0 a0=1 a1=55d5a35ce106 a2=0 a3=1 items=798 ppid=6 pid=528 
auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 
tty=(none) ses=4294967295 comm="modprobe" exe="/usr/bin/kmod" 
subj=system_u:system_r:insmod_t:s0 key="mod-load"

See: https://github.com/linux-audit/audit-kernel/issues/8
Test case: https://github.com/linux-audit/audit-testsuite/issues/42

Signed-off-by: Richard Guy Briggs 

---
v3:
  fix audit_buffer leak and dname error allocation leak audit_log_name
  only put audit_name->dentry if it is being replaced

v2:
  minor cosmetic changes and support fs filter patch
---
 include/linux/audit.h |8 
 kernel/audit.c|   19 +++
 kernel/audit.h|1 +
 kernel/auditsc.c  |8 +++-
 4 files changed, 31 insertions(+), 5 deletions(-)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index 2150bdc..1ef4ec8 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -240,7 +240,7 @@ extern void __audit_inode(struct filename *name, const 
struct dentry *dentry,
unsigned int flags);
 extern void __audit_file(const struct file *);
 extern void __audit_inode_child(struct inode *parent,
-   const struct dentry *dentry,
+   struct dentry *dentry,
const unsigned char type);
 extern void __audit_seccomp(unsigned long syscall, long signr, int code);
 extern void __audit_ptrace(struct task_struct *t);
@@ -305,7 +305,7 @@ static inline void audit_inode_parent_hidden(struct 
filename *name,
AUDIT_INODE_PARENT | AUDIT_INODE_HIDDEN);
 }
 static inline void audit_inode_child(struct inode *parent,
-const struct dentry *dentry,
+struct dentry *dentry,
 const unsigned char type) {
if (unlikely(!audit_dummy_context()))
__audit_inode_child(parent, dentry, type);
@@ -486,7 +486,7 @@ static inline void __audit_inode(struct filename *name,
unsigned int flags)
 { }
 static inline void __audit_inode_child(struct inode *parent,
-   const struct dentry *dentry,
+ 

[PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents

[Richard Guy Briggs]

Tracefs or debugfs were causing hundreds to thousands of null PATH
records to be associated with the init_module and finit_module SYSCALL
records on a few modules when the following rule was in place for
startup:
-a always,exit -F arch=x86_64 -S init_module -F key=mod-load

This happens because the parent inode is not found in the task's
audit_names list and hence treats it as anonymous.  This gives us no
information other than a numerical device number that may no longer be
visible upon log inspeciton, and an inode number.

Fill in the filesystem type, filesystem magic number and full pathname
from the filesystem mount point on previously null PATH records from
entries that have an anonymous parent from the child dentry using
dentry_path_raw().

Make the dentry argument of __audit_inode_child() non-const so that we
can take a reference to it in the case of an anonymous parent with
dget() and dget_parent() to be able to later print a partial path from
the host filesystem rather than null.

Since all we are given is an inode of the parent and the dentry of the
child, finding the path from the mount point to the root of the
filesystem is more challenging that would involve searching all
vfsmounts from "/" until a matching dentry is found for that
filesystem's root dentry.  Even if one is found, there may be more than
one mount point.  At this point the gain seems marginal since
knowing the filesystem type and path are a significant help in tracking
down the source of the PATH records and being to address them.

Sample output:
type=PROCTITLE msg=audit(1488317694.446:143): 
proctitle=2F7362696E2F6D6F6470726F6265002D71002D2D006E66737634
type=PATH msg=audit(1488317694.446:143): item=797 
name=tracefs(74726163):/events/nfs4/nfs4_setclientid/format inode=15969 
dev=00:09 mode=0100444 ouid=0 ogid=0 rdev=00:00 
obj=system_u:object_r:tracefs_t:s0 nametype=CREATE
type=PATH msg=audit(1488317694.446:143): item=796 
name=tracefs(74726163):/events/nfs4/nfs4_setclientid inode=15964 dev=00:09 
mode=040755 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 
nametype=PARENT
...
type=PATH msg=audit(1488317694.446:143): item=1 
name=tracefs(74726163):/events/nfs4 inode=15571 dev=00:09 mode=040755 ouid=0 
ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=CREATE
type=PATH msg=audit(1488317694.446:143): item=0 name=tracefs(74726163):/events 
inode=119 dev=00:09 mode=040755 ouid=0 ogid=0 rdev=00:00 
obj=system_u:object_r:tracefs_t:s0 nametype=PARENT
type=UNKNOWN[1330] msg=audit(1488317694.446:143): name="nfsv4"
type=SYSCALL msg=audit(1488317694.446:143): arch=c03e syscall=313 
success=yes exit=0 a0=1 a1=55d5a35ce106 a2=0 a3=1 items=798 ppid=6 pid=528 
auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 
tty=(none) ses=4294967295 comm="modprobe" exe="/usr/bin/kmod" 
subj=system_u:system_r:insmod_t:s0 key="mod-load"

See: https://github.com/linux-audit/audit-kernel/issues/8
Test case: https://github.com/linux-audit/audit-testsuite/issues/42

Signed-off-by: Richard Guy Briggs 

---
v3:
  fix audit_buffer leak and dname error allocation leak audit_log_name
  only put audit_name->dentry if it is being replaced

v2:
  minor cosmetic changes and support fs filter patch
---
 include/linux/audit.h |8 
 kernel/audit.c|   19 +++
 kernel/audit.h|1 +
 kernel/auditsc.c  |8 +++-
 4 files changed, 31 insertions(+), 5 deletions(-)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index 2150bdc..1ef4ec8 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -240,7 +240,7 @@ extern void __audit_inode(struct filename *name, const 
struct dentry *dentry,
unsigned int flags);
 extern void __audit_file(const struct file *);
 extern void __audit_inode_child(struct inode *parent,
-   const struct dentry *dentry,
+   struct dentry *dentry,
const unsigned char type);
 extern void __audit_seccomp(unsigned long syscall, long signr, int code);
 extern void __audit_ptrace(struct task_struct *t);
@@ -305,7 +305,7 @@ static inline void audit_inode_parent_hidden(struct 
filename *name,
AUDIT_INODE_PARENT | AUDIT_INODE_HIDDEN);
 }
 static inline void audit_inode_child(struct inode *parent,
-const struct dentry *dentry,
+struct dentry *dentry,
 const unsigned char type) {
if (unlikely(!audit_dummy_context()))
__audit_inode_child(parent, dentry, type);
@@ -486,7 +486,7 @@ static inline void __audit_inode(struct filename *name,
unsigned int flags)
 { }
 static inline void __audit_inode_child(struct inode *parent,
-   const struct dentry *dentry,
+