Re: [PATCH V2] audit: log 32-bit socketcalls
On Mon, Jan 16, 2017 at 10:53 PM, Richard Guy Briggs wrote: > On 2017-01-16 15:04, Paul Moore wrote: >> On Fri, Jan 13, 2017 at 9:42 AM, Eric Paris wrote: >> > On Fri, 2017-01-13 at 04:51 -0500, Richard Guy Briggs wrote: >> >> diff --git a/include/linux/audit.h b/include/linux/audit.h >> >> index 9d4443f..43d8003 100644 >> >> --- a/include/linux/audit.h >> >> +++ b/include/linux/audit.h >> >> @@ -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
Re: [PATCH V2] audit: log 32-bit socketcalls
On 2017-01-16 13:27, David Miller wrote: > From: Richard Guy Briggs > Date: Fri, 13 Jan 2017 04:51:48 -0500 > > > diff --git a/include/linux/audit.h b/include/linux/audit.h > > index 9d4443f..43d8003 100644 > > --- a/include/linux/audit.h > > +++ b/include/linux/audit.h > > @@ -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) > > +{ > > Please put an empty line between function definitions. Ok, should I reformat the rest of the file while I'm at it? > > + if (unlikely(!audit_dummy_context())) { > > + int i; > > + unsigned long a[AUDITSC_ARGS]; > > Please order local variable declarations from longest to shortest line. Ok. Is this a recent addition to a style guide or in checkpatch.pl? > > + > > + for (i=0; i > Please put a space around operators such as "=" and "<". Oops, my bad... > > + a[i] = (unsigned long)args[i]; > > + return __audit_socketcall(nargs, a); > > + } > > + return 0; > > +} > > static inline int audit_sockaddr(int len, void *addr) > > Again, empty line between function definitions please. > > > @@ -781,14 +782,24 @@ COMPAT_SYSCALL_DEFINE5(recvmmsg, int, fd, 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; > > Longest to shortest line for local variable declarations please. - RGB -- Richard Guy Briggs Kernel Security Engineering, Base Operating Systems, Red Hat Remote, Ottawa, Canada Voice: +1.647.777.2635, Internal: (81) 32635
Re: [PATCH V2] audit: log 32-bit socketcalls
On 2017-01-16 15:04, Paul Moore wrote: > On Fri, Jan 13, 2017 at 9:42 AM, Eric Paris wrote: > > On Fri, 2017-01-13 at 04:51 -0500, Richard Guy Briggs wrote: > >> diff --git a/include/linux/audit.h b/include/linux/audit.h > >> index 9d4443f..43d8003 100644 > >> --- a/include/linux/audit.h > >> +++ b/include/linux/audit.h > >> @@ -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 Kernel Security Engineering, Base Operating Systems, Red Hat Remote, Ottawa, Canada Voice: +1.647.777.2635, Internal: (81) 32635
Re: [PATCH V2] audit: log 32-bit socketcalls
From: Paul Moore 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 ... No objection.
Re: [PATCH V2] audit: log 32-bit socketcalls
On Mon, Jan 16, 2017 at 1:27 PM, David Miller wrote: > From: Richard Guy Briggs > Date: Fri, 13 Jan 2017 04:51:48 -0500 > >> diff --git a/include/linux/audit.h b/include/linux/audit.h >> index 9d4443f..43d8003 100644 >> --- a/include/linux/audit.h >> +++ b/include/linux/audit.h >> @@ -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) >> +{ > > 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 ... >> + if (unlikely(!audit_dummy_context())) { >> + int i; >> + unsigned long a[AUDITSC_ARGS]; > > Please order local variable declarations from longest to shortest line. > >> + >> + for (i=0; i > Please put a space around operators such as "=" and "<". > >> + a[i] = (unsigned long)args[i]; >> + return __audit_socketcall(nargs, a); >> + } >> + return 0; >> +} >> static inline int audit_sockaddr(int len, void *addr) > > Again, empty line between function definitions please. > >> @@ -781,14 +782,24 @@ COMPAT_SYSCALL_DEFINE5(recvmmsg, int, fd, 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; > > Longest to shortest line for local variable declarations please. > > -- > Linux-audit mailing list > linux-au...@redhat.com > https://www.redhat.com/mailman/listinfo/linux-audit -- paul moore www.paul-moore.com
Re: [PATCH V2] audit: log 32-bit socketcalls
On Fri, Jan 13, 2017 at 9:42 AM, Eric Paris wrote: > On Fri, 2017-01-13 at 04:51 -0500, Richard Guy Briggs wrote: >> diff --git a/include/linux/audit.h b/include/linux/audit.h >> index 9d4443f..43d8003 100644 >> --- a/include/linux/audit.h >> +++ b/include/linux/audit.h >> @@ -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
Re: [PATCH V2] audit: log 32-bit socketcalls
From: Richard Guy Briggs Date: Fri, 13 Jan 2017 04:51:48 -0500 > diff --git a/include/linux/audit.h b/include/linux/audit.h > index 9d4443f..43d8003 100644 > --- a/include/linux/audit.h > +++ b/include/linux/audit.h > @@ -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) > +{ Please put an empty line between function definitions. > + if (unlikely(!audit_dummy_context())) { > + int i; > + unsigned long a[AUDITSC_ARGS]; Please order local variable declarations from longest to shortest line. > + > + for (i=0; i + a[i] = (unsigned long)args[i]; > + return __audit_socketcall(nargs, a); > + } > + return 0; > +} > static inline int audit_sockaddr(int len, void *addr) Again, empty line between function definitions please. > @@ -781,14 +782,24 @@ COMPAT_SYSCALL_DEFINE5(recvmmsg, int, fd, 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; Longest to shortest line for local variable declarations please.
Re: [PATCH V2] audit: log 32-bit socketcalls
On 2017-01-13 10:18, Eric Paris wrote: > On Fri, 2017-01-13 at 10:06 -0500, Richard Guy Briggs wrote: > > On 2017-01-13 09:42, Eric Paris wrote: > > > On Fri, 2017-01-13 at 04:51 -0500, Richard Guy Briggs wrote: > > > > > > diff --git a/include/linux/audit.h b/include/linux/audit.h > > > > index 9d4443f..43d8003 100644 > > > > --- a/include/linux/audit.h > > > > +++ b/include/linux/audit.h > > > > @@ -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 -- Richard Guy Briggs Kernel Security Engineering, Base Operating Systems, Red Hat Remote, Ottawa, Canada Voice: +1.647.777.2635, Internal: (81) 32635
Re: [PATCH V2] audit: log 32-bit socketcalls
On Fri, 2017-01-13 at 10:06 -0500, Richard Guy Briggs wrote: > On 2017-01-13 09:42, Eric Paris wrote: > > On Fri, 2017-01-13 at 04:51 -0500, Richard Guy Briggs wrote: > > > diff --git a/include/linux/audit.h b/include/linux/audit.h > > > index 9d4443f..43d8003 100644 > > > --- a/include/linux/audit.h > > > +++ b/include/linux/audit.h > > > @@ -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.
Re: [PATCH V2] audit: log 32-bit socketcalls
On 2017-01-13 09:42, Eric Paris wrote: > On Fri, 2017-01-13 at 04:51 -0500, Richard Guy Briggs wrote: > > 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 > > > > -- > > 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(-) > > > > diff --git a/include/linux/audit.h b/include/linux/audit.h > > index 9d4443f..43d8003 100644 > > --- a/include/linux/audit.h > > +++ b/include/linux/audit.h > > @@ -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]; > > [...] > } > > > + int i; > > + unsigned long a[AUDITSC_ARGS]; > > + > > + for (i=0; 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) > > diff --git a/net/compat.c b/net/compat.c > > index 1cd2ec0..f0404d4 100644 > > --- a/net/compat.c > > +++ b/net/compat.c > > @@ -22,6 +22,7 @@ > > #include > > #include > > #include > > +#include > > #include > > > > #include > > @@ -781,14 +782,24 @@ COMPAT_SYSCALL_DEFINE5(recvmmsg, int, fd, > > 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 Br
Re: [PATCH V2] audit: log 32-bit socketcalls
On Fri, 2017-01-13 at 04:51 -0500, Richard Guy Briggs wrote: > 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 > > -- > 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(-) > > diff --git a/include/linux/audit.h b/include/linux/audit.h > index 9d4443f..43d8003 100644 > --- a/include/linux/audit.h > +++ b/include/linux/audit.h > @@ -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]; [...] } > + int i; > + unsigned long a[AUDITSC_ARGS]; > + > + for (i=0; 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) > diff --git a/net/compat.c b/net/compat.c > index 1cd2ec0..f0404d4 100644 > --- a/net/compat.c > +++ b/net/compat.c > @@ -22,6 +22,7 @@ > #include > #include > #include > +#include > #include > > #include > @@ -781,14 +782,24 @@ COMPAT_SYSCALL_DEFINE5(recvmmsg, int, fd, > 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]; >
[PATCH V2] audit: log 32-bit socketcalls
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 -- 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(-) diff --git a/include/linux/audit.h b/include/linux/audit.h index 9d4443f..43d8003 100644 --- a/include/linux/audit.h +++ b/include/linux/audit.h @@ -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())) { + int i; + unsigned long a[AUDITSC_ARGS]; + + for (i=0; i #include #include +#include #include #include @@ -781,14 +782,24 @@ COMPAT_SYSCALL_DEFINE5(recvmmsg, int, fd, 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]; -- 1.7.1