From: Richard Guy Briggs <hidden> Date: 2017-01-13 09:52:32
32-bit socketcalls were not being logged by audit on x86_64 systems.
Log them. This is basically a duplicate of the call from
net/socket.c:sys_socketcall(), but it addresses the impedance mismatch
between 32-bit userspace process and 64-bit kernel audit.
See: https://github.com/linux-audit/audit-kernel/issues/14
Signed-off-by: Richard Guy Briggs <redacted>
--
v2:
Move work to audit_socketcall_compat() and use audit_dummy_context().
---
include/linux/audit.h | 16 ++++++++++++++++
net/compat.c | 15 +++++++++++++--
2 files changed, 29 insertions(+), 2 deletions(-)
From: Eric Paris <eparis@redhat.com> Date: 2017-01-13 14:42:26
On Fri, 2017-01-13 at 04:51 -0500, Richard Guy Briggs wrote:
quoted hunk
32-bit socketcalls were not being logged by audit on x86_64 systems.
Log them. This is basically a duplicate of the call from
net/socket.c:sys_socketcall(), but it addresses the impedance
mismatch
between 32-bit userspace process and 64-bit kernel audit.
See: https://github.com/linux-audit/audit-kernel/issues/14
Signed-off-by: Richard Guy Briggs <redacted>
--
v2:
Move work to audit_socketcall_compat() and use
audit_dummy_context().
---
include/linux/audit.h | 16 ++++++++++++++++
net/compat.c | 15 +++++++++++++--
2 files changed, 29 insertions(+), 2 deletions(-)
@@ -387,6 +387,18 @@ static inline int audit_socketcall(int nargs,
unsigned long *args)
return __audit_socketcall(nargs, args);
return 0;
}
+static inline int audit_socketcall_compat(int nargs, u32 *args)
+{
+ if (unlikely(!audit_dummy_context())) {
I've always hated these likely/unlikely. Mostly because I think they
are so often wrong. I believe this says that you compiled audit in but
you expect it to be explicitly disabled. While that is (recently) true
in Fedora I highly doubt that's true on the vast majority of systems
that have audit compiled in.
I think all of the likely/unlikely need to just be abandoned, but at
least don't add more? It certainly wouldn't be the first time I was
wrong, and I haven't profiled it. But the function would definitely
look better if coded
static inline int audit_socketcall_compat(int nargs, u32 *args)
{
if (audit_cummy_context()) {
return 0
}
int i;
unsigned long a[AUDITSC_ARGS];
[...]
}
quoted hunk
+ int i;
+ unsigned long a[AUDITSC_ARGS];
+
+ for (i=0; i<nargs; i++)
+ a[i] = (unsigned long)args[i];
+ return __audit_socketcall(nargs, a);
+ }
+ return 0;
+}
static inline int audit_sockaddr(int len, void *addr)
{
if (unlikely(!audit_dummy_context()))
@@ -513,6 +525,10 @@ static inline int audit_socketcall(int nargs,
unsigned long *args)
{
return 0;
}
+static inline int audit_socketcall_compat(int nargs, u32 *args)
+{
+ return 0;
+}
static inline void audit_fd_pair(int fd1, int fd2)
{ }
static inline int audit_sockaddr(int len, void *addr)
From: Richard Guy Briggs <hidden> Date: 2017-01-13 15:06:43
On 2017-01-13 09:42, Eric Paris wrote:
On Fri, 2017-01-13 at 04:51 -0500, Richard Guy Briggs wrote:
quoted
32-bit socketcalls were not being logged by audit on x86_64 systems.
Log them. This is basically a duplicate of the call from
net/socket.c:sys_socketcall(), but it addresses the impedance
mismatch
between 32-bit userspace process and 64-bit kernel audit.
See: https://github.com/linux-audit/audit-kernel/issues/14
Signed-off-by: Richard Guy Briggs <redacted>
--
v2:
Move work to audit_socketcall_compat() and use
audit_dummy_context().
---
include/linux/audit.h | 16 ++++++++++++++++
net/compat.c | 15 +++++++++++++--
2 files changed, 29 insertions(+), 2 deletions(-)
@@ -387,6 +387,18 @@ static inline int audit_socketcall(int nargs,
unsigned long *args)
return __audit_socketcall(nargs, args);
return 0;
}
+static inline int audit_socketcall_compat(int nargs, u32 *args)
+{
+ if (unlikely(!audit_dummy_context())) {
I've always hated these likely/unlikely. Mostly because I think they
are so often wrong. I believe this says that you compiled audit in but
you expect it to be explicitly disabled. While that is (recently) true
in Fedora I highly doubt that's true on the vast majority of systems
that have audit compiled in.
It has been argued that audit should have pretty much no performance
impact if it is not in use and that if it is, we're willing to take the
more significant overhead of the rest of the code for the sake of one
test to determine whether or not to follow this code path. The audit
subsystem isn't everyone's favourite baby so not making performance
worse for non-audit-users might help play nice in the community. I did
contemplate Hanlon's Razor when wondering if this call had been left out
to avoid thunking complications or overhead.
In this case, compat users are less likely to raise a stink about
performance issues, so this situation is not particularly critical.
I think all of the likely/unlikely need to just be abandoned, but at
least don't add more? It certainly wouldn't be the first time I was
wrong, and I haven't profiled it. But the function would definitely
look better if coded
This is a seperate issue and I agree about simply bailing early rather
than putting the rest of the function in one conditional. Given the
scoping though, the local variables aren't needed to be allocated in the
unlikely case, but I'm sure the compiler optimizer knows better than we
do how to make this go fast...
In both cases, I thought there was value in making
audit_socketcall_compat() as similar in logic to audit_socketcall() as
possible to make them easier to maintain.
Is either issue a cause for patch respin? If so, I'll want to normalize
the two functions for consistency.
static inline int audit_socketcall_compat(int nargs, u32 *args)
{
if (audit_cummy_context()) {
return 0
}
int i;
unsigned long a[AUDITSC_ARGS];
[...]
}
quoted
+ int i;
+ unsigned long a[AUDITSC_ARGS];
+
+ for (i=0; i<nargs; i++)
+ a[i] = (unsigned long)args[i];
+ return __audit_socketcall(nargs, a);
+ }
+ return 0;
+}
static inline int audit_sockaddr(int len, void *addr)
{
if (unlikely(!audit_dummy_context()))
@@ -513,6 +525,10 @@ static inline int audit_socketcall(int nargs,
unsigned long *args)
{
return 0;
}
+static inline int audit_socketcall_compat(int nargs, u32 *args)
+{
+ return 0;
+}
static inline void audit_fd_pair(int fd1, int fd2)
{ }
static inline int audit_sockaddr(int len, void *addr)
struct compat_mmsghdr __user *, mmsg,
COMPAT_SYSCALL_DEFINE2(socketcall, int, call, u32 __user *, args)
{
+ unsigned int len;
int ret;
- u32 a[6];
+ u32 a[AUDITSC_ARGS];
u32 a0, a1;
if (call < SYS_SOCKET || call > SYS_SENDMMSG)
return -EINVAL;
- if (copy_from_user(a, args, nas[call]))
+ len = nas[call];
+ if (len > sizeof(a))
+ return -EINVAL;
+
+ if (copy_from_user(a, args, len))
return -EFAULT;
+
+ ret = audit_socketcall_compat(len/sizeof(a[0]), a);
+ if (ret)
+ return ret;
+
a0 = a[0];
a1 = a[1];
- RGB
--
Richard Guy Briggs [off-list ref]
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635
@@ -387,6 +387,18 @@ static inline int audit_socketcall(int
nargs,
unsigned long *args)
return __audit_socketcall(nargs, args);
return 0;
}
+static inline int audit_socketcall_compat(int nargs, u32 *args)
+{
+ if (unlikely(!audit_dummy_context())) {
I've always hated these likely/unlikely. Mostly because I think
they
are so often wrong. I believe this says that you compiled audit in
but
you expect it to be explicitly disabled. While that is (recently)
true
in Fedora I highly doubt that's true on the vast majority of
systems
that have audit compiled in.
It has been argued that audit should have pretty much no performance
impact if it is not in use and that if it is, we're willing to take
the
more significant overhead of the rest of the code for the sake of one
test to determine whether or not to follow this code path.
Ok, I can buy that argument. Not sure its where I would have settled,
but it does make sense. I'll obviously defer to Paul on what he wants
out of style. I always assume the compiler is brilliant and write
stupid code but your logic is sound there too.
You can/should pretend I said nothing.
@@ -387,6 +387,18 @@ static inline int audit_socketcall(int
nargs,
unsigned long *args)
return __audit_socketcall(nargs, args);
return 0;
}
+static inline int audit_socketcall_compat(int nargs, u32 *args)
+{
+ if (unlikely(!audit_dummy_context())) {
I've always hated these likely/unlikely. Mostly because I think
they
are so often wrong. I believe this says that you compiled audit in
but
you expect it to be explicitly disabled. While that is (recently)
true
in Fedora I highly doubt that's true on the vast majority of
systems
that have audit compiled in.
It has been argued that audit should have pretty much no performance
impact if it is not in use and that if it is, we're willing to take
the
more significant overhead of the rest of the code for the sake of one
test to determine whether or not to follow this code path.
Ok, I can buy that argument. Not sure its where I would have settled,
but it does make sense. I'll obviously defer to Paul on what he wants
out of style. I always assume the compiler is brilliant and write
stupid code but your logic is sound there too.
You can/should pretend I said nothing.
You're keeping me honest and making me work for my dinner! ;-)
- RGB
@@ -387,6 +387,18 @@ static inline int audit_socketcall(int nargs,
unsigned long *args)
return __audit_socketcall(nargs, args);
return 0;
}
+static inline int audit_socketcall_compat(int nargs, u32 *args)
+{
+ if (unlikely(!audit_dummy_context())) {
I've always hated these likely/unlikely. Mostly because I think they
are so often wrong. I believe this says that you compiled audit in but
you expect it to be explicitly disabled. While that is (recently) true
in Fedora I highly doubt that's true on the vast majority of systems
that have audit compiled in.
Richard and I have talked about the likely/unlikely optimization
before and I know Richard likes to use them, but I don't for the
reasons Eric has already mentioned. Richard, since you're respinning
the patch, go ahead and yank out the unlikely() call.
--
paul moore
www.paul-moore.com
@@ -387,6 +387,18 @@ static inline int audit_socketcall(int nargs, unsigned long *args)return__audit_socketcall(nargs,args);return0;}+staticinlineintaudit_socketcall_compat(intnargs,u32*args)+{
Please put an empty line between function definitions.
David, assuming Richard makes your requested changes, any objection if
I merge this via the audit tree instead of the netdev tree? It's a
bit easier for us from a testing perspective this way ...
quoted
+ if (unlikely(!audit_dummy_context())) {
+ int i;
+ unsigned long a[AUDITSC_ARGS];
Please order local variable declarations from longest to shortest line.
quoted
+
+ for (i=0; i<nargs; i++)
Please put a space around operators such as "=" and "<".
From: David Miller <davem@davemloft.net> Date: 2017-01-16 21:55:17
From: Paul Moore <paul@paul-moore.com>
Date: Mon, 16 Jan 2017 15:38:33 -0500
David, assuming Richard makes your requested changes, any objection if
I merge this via the audit tree instead of the netdev tree? It's a
bit easier for us from a testing perspective this way ...
@@ -387,6 +387,18 @@ static inline int audit_socketcall(int nargs,
unsigned long *args)
return __audit_socketcall(nargs, args);
return 0;
}
+static inline int audit_socketcall_compat(int nargs, u32 *args)
+{
+ if (unlikely(!audit_dummy_context())) {
I've always hated these likely/unlikely. Mostly because I think they
are so often wrong. I believe this says that you compiled audit in but
you expect it to be explicitly disabled. While that is (recently) true
in Fedora I highly doubt that's true on the vast majority of systems
that have audit compiled in.
Richard and I have talked about the likely/unlikely optimization
before and I know Richard likes to use them, but I don't for the
reasons Eric has already mentioned. Richard, since you're respinning
the patch, go ahead and yank out the unlikely() call.
I don't "like to use them". I'm simply following the use and style of
existing code and the arguments of others in places of critical
performance. If I "fix" that one, then I would feel compelled to yank
out the one in the function immediately above, audit_socketcall() for
consistency to ease my conscience. Eric conceded that argument.
paul moore
- RGB
--
Richard Guy Briggs [off-list ref]
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635
Longest to shortest line for local variable declarations please.
- RGB
--
Richard Guy Briggs [off-list ref]
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635
@@ -387,6 +387,18 @@ static inline int audit_socketcall(int nargs,
unsigned long *args)
return __audit_socketcall(nargs, args);
return 0;
}
+static inline int audit_socketcall_compat(int nargs, u32 *args)
+{
+ if (unlikely(!audit_dummy_context())) {
I've always hated these likely/unlikely. Mostly because I think they
are so often wrong. I believe this says that you compiled audit in but
you expect it to be explicitly disabled. While that is (recently) true
in Fedora I highly doubt that's true on the vast majority of systems
that have audit compiled in.
Richard and I have talked about the likely/unlikely optimization
before and I know Richard likes to use them, but I don't for the
reasons Eric has already mentioned. Richard, since you're respinning
the patch, go ahead and yank out the unlikely() call.
I don't "like to use them". I'm simply following the use and style of
existing code and the arguments of others in places of critical
performance.
Okay that's a reasonable reason for why you continued the tradition,
however I'm asking you (for the second time now) not to use it in
audit_socketcall_compat().
If I "fix" that one, then I would feel compelled to yank
out the one in the function immediately above, audit_socketcall() for
consistency to ease my conscience. Eric conceded that argument.
I'll be very clear on what I want to see in this patch: don't use
likely/unlikely in audit_socketcall_compat() and don't touch it's use
in the other functions at this point in time.
--
paul moore
www.paul-moore.com