2:33 p.m.
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(-)
3:23 p.m.
On Wed, Dec 31, 2014 at 12:33 PM, Paul Moore <paul(a)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
4:08 p.m.
On Wednesday, December 31, 2014 01:23:14 PM Linus Torvalds wrote:
On Wed, Dec 31, 2014 at 12:33 PM, Paul Moore
<paul(a)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
4:54 p.m.
On Wed, Dec 31, 2014 at 2:08 PM, Paul Moore <paul(a)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
12:18 p.m.
On Wed, Dec 31, 2014 at 5:54 PM, Linus Torvalds
<torvalds(a)linux-foundation.org> wrote:
On Wed, Dec 31, 2014 at 2:08 PM, Paul Moore
<paul(a)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
6:01 p.m.
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()?
12:22 p.m.
On Wed, Dec 31, 2014 at 7:01 PM, Al Viro <viro(a)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
12:41 p.m.
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.
4088
days inactive
4089
days old
linux-audit@lists.linux-audit.osci.io
7 comments
3 participants
participants (3)
-
Al Viro -
Linus Torvalds -
Paul Moore