[GIT PULL] Audit fixes for 3.19 #2

[GIT PULL] Audit fixes for 3.19 #2

[Paul Moore]

Hi Linus,

One audit patch to resolve a panic/oops when recording filenames in the audit 
log, see the mail archive link below.  The fix isn't as nice as I would like, 
as it involves an allocate/copy of the filename, but it solves the problem and 
the overhead should only affect users who have configured audit rules 
involving file names.  We'll revisit this issue with future kernels in an 
attempt to make this suck less, but in the meantime I think this fix should go 
into the next release of v3.19-rcX.

 * https://marc.info/?t=141986927600001&r=1&w=2

Thanks,
-Paul

---
The following changes since commit 041d7b98ffe59c59fdd639931dea7d74f9aa9a59:

  audit: restore AUDIT_LOGINUID unset ABI (2014-12-23 16:40:18 -0500)

are available in the git repository at:

  git://git.infradead.org/users/pcmoore/audit upstream

for you to fetch changes up to fcf22d8267ad2601fe9b6c549d1be96401c23e0b:

  audit: create private file name copies when auditing inodes (2014-12-30 
         09:26:21 -0500)

----------------------------------------------------------------
Paul Moore (1):
      audit: create private file name copies when auditing inodes

 kernel/auditsc.c | 49 ++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 40 insertions(+), 9 deletions(-)

Re: [GIT PULL] Audit fixes for 3.19 #2

[Linus Torvalds]

On Wed, Dec 31, 2014 at 12:33 PM, Paul Moore <paul@paul-moore.com> wrote:
>
> One audit patch to resolve a panic/oops when recording filenames in the audit
> log, see the mail archive link below.  The fix isn't as nice as I would like,
> as it involves an allocate/copy of the filename, but it solves the problem and
> the overhead should only affect users who have configured audit rules
> involving file names.

This fix looks wrong.

The kernel "getname()" function already has hacks explicitly for this
audit usage. Why aren't those hacks working? See the whole
"audit_getname()" and "audit_putname()" thing in fs/namei.c.

So why does audit now need to copy the name *again*, when the whole -
and only - point of the current fs/namei.c audit hackery is exactly so
that audit can control the lifetime of the pathnames?

Hmm? Alternatively, could we just remove the fs/namei.c hackery
entirely, and rely on audit always copying the filenames for its own
use?

                      Linus

Re: [GIT PULL] Audit fixes for 3.19 #2

[Paul Moore]

On Wednesday, December 31, 2014 01:23:14 PM Linus Torvalds wrote:
> On Wed, Dec 31, 2014 at 12:33 PM, Paul Moore <paul@paul-moore.com> wrote:
> > One audit patch to resolve a panic/oops when recording filenames in the
> > audit log, see the mail archive link below.  The fix isn't as nice as I
> > would like, as it involves an allocate/copy of the filename, but it
> > solves the problem and the overhead should only affect users who have
> > configured audit rules involving file names.
> 
> This fix looks wrong.
> 
> The kernel "getname()" function already has hacks explicitly for this
> audit usage. Why aren't those hacks working? See the whole
> "audit_getname()" and "audit_putname()" thing in fs/namei.c.
> 
> So why does audit now need to copy the name *again*, when the whole -
> and only - point of the current fs/namei.c audit hackery is exactly so
> that audit can control the lifetime of the pathnames?

The getname/putname hacks work in the normal file case, but it falls apart 
when you start talking about AF_UNIX socket files where the filename string 
doesn't go through the getname/putname refcount tricks.  In the past (no idea 
how far back this goes off the top of my head) this wasn't an issue since the 
code which recorded the filenames in the audit records was broken, but since 
we just "fixed" that problem, the AF_UNIX socket problem is now making an 
appearance.

At least that is how it looks to me right now, if I'm wrong about this and I'm 
missing an obvious fix I'm all ears/eyes/etc.

> Hmm? Alternatively, could we just remove the fs/namei.c hackery
> entirely, and rely on audit always copying the filenames for its own
> use?

I'm still coming up to speed on this mess of a subsystem, so I can't say I'm 
well versed in all the audit design decisions up to this point, but ... I'd 
hate to see us lose the getname/putname hacks if we can find a way to 
differentiate between normal files and things like AF_UNIX.  I've got some 
ideas but I wanted to get you a patch soonish since v3.19-rc2 pukes all over 
itself if you configure audit in a particular way (evidently the Gentoo 
default config triggers the problem).  If you're okay with waiting a bit 
longer I can work on this a bit more and try to find a more elegant solution; 
I'm already working this on anyway for v3.20 (or whatever it happens to be 
when the patch is ready).

-- 
paul moore
www.paul-moore.com

Re: [GIT PULL] Audit fixes for 3.19 #2

[Linus Torvalds]

On Wed, Dec 31, 2014 at 2:08 PM, Paul Moore <paul@paul-moore.com> wrote:
>
> The getname/putname hacks work in the normal file case, but it falls apart
> when you start talking about AF_UNIX socket files where the filename string
> doesn't go through the getname/putname refcount tricks.  In the past (no idea
> how far back this goes off the top of my head) this wasn't an issue since the
> code which recorded the filenames in the audit records was broken, but since
> we just "fixed" that problem, the AF_UNIX socket problem is now making an
> appearance.

Ugh. Ok, I pulled the thing, even if I really hope for a better
solution long-term. That better solution may well be to get rid of all
the audit hackery in getname()/putname(), I wouldn't mind that at all.

                         Linus

Re: [GIT PULL] Audit fixes for 3.19 #2

[Al Viro]

On Wed, Dec 31, 2014 at 05:08:12PM -0500, Paul Moore wrote:

> The getname/putname hacks work in the normal file case, but it falls apart 
> when you start talking about AF_UNIX socket files where the filename string 
> doesn't go through the getname/putname refcount tricks.  In the past (no idea 
> how far back this goes off the top of my head) this wasn't an issue since the 
> code which recorded the filenames in the audit records was broken, but since 
> we just "fixed" that problem, the AF_UNIX socket problem is now making an 
> appearance.
> 
> At least that is how it looks to me right now, if I'm wrong about this and I'm 
> missing an obvious fix I'm all ears/eyes/etc.

Umm...  How about just adding a function that would be used instead of
all those
        struct filename filename = { .name = name };
and created an object that would be destroyed later by putname()?

Re: [GIT PULL] Audit fixes for 3.19 #2

[Paul Moore]

On Wed, Dec 31, 2014 at 5:54 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Wed, Dec 31, 2014 at 2:08 PM, Paul Moore <paul@paul-moore.com> wrote:
>>
>> The getname/putname hacks work in the normal file case, but it falls apart
>> when you start talking about AF_UNIX socket files where the filename string
>> doesn't go through the getname/putname refcount tricks.  In the past (no idea
>> how far back this goes off the top of my head) this wasn't an issue since the
>> code which recorded the filenames in the audit records was broken, but since
>> we just "fixed" that problem, the AF_UNIX socket problem is now making an
>> appearance.
>
> Ugh. Ok, I pulled the thing, even if I really hope for a better
> solution long-term. That better solution may well be to get rid of all
> the audit hackery in getname()/putname(), I wouldn't mind that at all.

Thanks.  We'll make this better, I'm confident of that, I'm just not
certain if it will be v3.19-rcX or v3.20 material at this point.

-- 
paul moore
www.paul-moore.com

Re: [GIT PULL] Audit fixes for 3.19 #2

[Paul Moore]

On Wed, Dec 31, 2014 at 7:01 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Wed, Dec 31, 2014 at 05:08:12PM -0500, Paul Moore wrote:
>
>> The getname/putname hacks work in the normal file case, but it falls apart
>> when you start talking about AF_UNIX socket files where the filename string
>> doesn't go through the getname/putname refcount tricks.  In the past (no idea
>> how far back this goes off the top of my head) this wasn't an issue since the
>> code which recorded the filenames in the audit records was broken, but since
>> we just "fixed" that problem, the AF_UNIX socket problem is now making an
>> appearance.
>>
>> At least that is how it looks to me right now, if I'm wrong about this and I'm
>> missing an obvious fix I'm all ears/eyes/etc.
>
> Umm...  How about just adding a function that would be used instead of
> all those
>         struct filename filename = { .name = name };
> and created an object that would be destroyed later by putname()?

That was one of the thoughts I had, but considering all the love for
the audit subsystem I wasn't certain if that would be well received by
the other kernel devs.  Now that we at least have the panic/oops
resolved, that frees up some time for debate/discussion; I'll work up
a patchset that takes this approach and we'll see what pushback there
is, if any.

-- 
paul moore
www.paul-moore.com

Re: [GIT PULL] Audit fixes for 3.19 #2

[Al Viro]

On Thu, Jan 01, 2015 at 12:01:49AM +0000, Al Viro wrote:

> Umm...  How about just adding a function that would be used instead of
> all those
>         struct filename filename = { .name = name };
> and created an object that would be destroyed later by putname()?

... such as getname_kernel(), actually.  There are 5 places like that:
fs/exec.c:open_exec(), fs/namei.c:do_path_lookup(),
fs/namei.c:kern_path_mountpoint(), fs/namei.c:do_file_open_root(),
fs/open.c:filp_open().  Said that, I'm not sure that no call chains
allow names just under PATH_MAX, so getname_kernel() might need to be
taught to handle those.