* [PATCH] crypto: af_alg - Fix regression on empty requests
From: Herbert Xu @ 2020-06-26 6:29 UTC (permalink / raw)
To: Eric Biggers
Cc: Naresh Kamboju, Luis Chamberlain, LTP List, open list,
linux-security-module, keyrings, lkft-triage, linux-crypto,
Jan Stancek, chrubis, Serge E. Hallyn, James Morris,
Jarkko Sakkinen, David Howells, David S. Miller, Sachin Sant,
Linux Next Mailing List, linuxppc-dev
In-Reply-To: <20200623170217.GB150582@gmail.com>
On Tue, Jun 23, 2020 at 10:02:17AM -0700, Eric Biggers wrote:
>
> The source code for the two failing AF_ALG tests is here:
>
> https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/crypto/af_alg02.c
> https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/crypto/af_alg05.c
>
> They use read() and write(), not send() and recv().
>
> af_alg02 uses read() to read from a "salsa20" request socket without writing
> anything to it. It is expected that this returns 0, i.e. that behaves like
> encrypting an empty message.
>
> af_alg05 uses write() to write 15 bytes to a "cbc(aes-generic)" request socket,
> then read() to read 15 bytes. It is expected that this fails with EINVAL, since
> the length is not aligned to the AES block size (16 bytes).
This patch should fix the regression:
---8<---
Some user-space programs rely on crypto requests that have no
control metadata. This broke when a check was added to require
the presence of control metadata with the ctx->init flag.
This patch fixes the regression by removing the ctx->init flag.
This means that we do not distinguish the case of no metadata
as opposed to an empty request. IOW it is always assumed that
if you call recv(2) before sending metadata that you are working
with an empty request.
Reported-by: Sachin Sant <sachinp@linux.vnet.ibm.com>
Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
Fixes: f3c802a1f300 ("crypto: algif_aead - Only wake up when...")
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index 9fcb91ea10c4..2d391117c020 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -635,7 +635,6 @@ void af_alg_pull_tsgl(struct sock *sk, size_t used, struct scatterlist *dst,
if (!ctx->used)
ctx->merge = 0;
- ctx->init = ctx->more;
}
EXPORT_SYMBOL_GPL(af_alg_pull_tsgl);
@@ -757,8 +756,7 @@ int af_alg_wait_for_data(struct sock *sk, unsigned flags, unsigned min)
break;
timeout = MAX_SCHEDULE_TIMEOUT;
if (sk_wait_event(sk, &timeout,
- ctx->init && (!ctx->more ||
- (min && ctx->used >= min)),
+ !ctx->more || (min && ctx->used >= min),
&wait)) {
err = 0;
break;
@@ -847,7 +845,7 @@ int af_alg_sendmsg(struct socket *sock, struct msghdr *msg, size_t size,
}
lock_sock(sk);
- if (ctx->init && (init || !ctx->more)) {
+ if (!ctx->more && ctx->used) {
err = -EINVAL;
goto unlock;
}
@@ -858,7 +856,6 @@ int af_alg_sendmsg(struct socket *sock, struct msghdr *msg, size_t size,
memcpy(ctx->iv, con.iv->iv, ivsize);
ctx->aead_assoclen = con.aead_assoclen;
- ctx->init = true;
}
while (size) {
diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c
index d48d2156e621..749fe42315be 100644
--- a/crypto/algif_aead.c
+++ b/crypto/algif_aead.c
@@ -106,7 +106,7 @@ static int _aead_recvmsg(struct socket *sock, struct msghdr *msg,
size_t usedpages = 0; /* [in] RX bufs to be used from user */
size_t processed = 0; /* [in] TX bufs to be consumed */
- if (!ctx->init || ctx->more) {
+ if (ctx->more) {
err = af_alg_wait_for_data(sk, flags, 0);
if (err)
return err;
diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index a51ba22fef58..5b6fa5e8c00d 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -61,7 +61,7 @@ static int _skcipher_recvmsg(struct socket *sock, struct msghdr *msg,
int err = 0;
size_t len = 0;
- if (!ctx->init || (ctx->more && ctx->used < bs)) {
+ if (ctx->more && ctx->used < bs) {
err = af_alg_wait_for_data(sk, flags, bs);
if (err)
return err;
diff --git a/include/crypto/if_alg.h b/include/crypto/if_alg.h
index ee6412314f8f..08c087cc89d6 100644
--- a/include/crypto/if_alg.h
+++ b/include/crypto/if_alg.h
@@ -135,7 +135,6 @@ struct af_alg_async_req {
* SG?
* @enc: Cryptographic operation to be performed when
* recvmsg is invoked.
- * @init: True if metadata has been sent.
* @len: Length of memory allocated for this data structure.
*/
struct af_alg_ctx {
@@ -152,7 +151,6 @@ struct af_alg_ctx {
bool more;
bool merge;
bool enc;
- bool init;
unsigned int len;
};
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply related
* Re: [RFC][PATCH] net/bpfilter: Remove this broken and apparently unmantained
From: Tetsuo Handa @ 2020-06-26 6:20 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Linus Torvalds, David Miller, Greg Kroah-Hartman,
Eric W. Biederman, Kees Cook, Andrew Morton, Alexei Starovoitov,
Al Viro, bpf, linux-fsdevel, Daniel Borkmann, Jakub Kicinski,
Masahiro Yamada, Gary Lin, Bruno Meneguele, LSM List,
Casey Schaufler
In-Reply-To: <20200626054137.m44jpsvlapuyslzw@ast-mbp.dhcp.thefacebook.com>
On 2020/06/26 14:41, Alexei Starovoitov wrote:
>> I was hoping that fork_usermode_blob() accepts only simple program
>> like the content of "hello64" generated by
>
> pretty much. statically compiled elf that is self contained.
But fork_usermode_blob() itself does not check that.
>> due to interference from the rest of the system, how can we say "we trust kernel
>> code executing in userspace" ?
>
> I answered this already in the previous email.
Previous post is mostly summary for David Miller who responded
It's kernel code executing in userspace. If you don't trust the
signed code you don't trust the signed code.
Nothing is magic about a piece of code executing in userspace.
without understanding my concerns.
> Use security_ptrace_access_check() LSM hook to make sure that no other process
> can tamper with blob's memory when it's running as user process.
Yes, security_ptrace_access_check() hook is there. But see the reality explained later.
> In the future it would be trivial to add a new ptrace flag to
> make sure that blob's memory is not ptraceable from the start.
I guess it is some PF_* flag (like PF_KTHREAD is used for avoiding some interference).
>> There is security_ptrace_access_check() LSM hook, but no zero-configuration
>> method is available.
>
> huh?
> tomoyo is not using that hook, but selinux and many other LSMs do.
> please learn from others.
What I am hoping is that we can restrict interference between usermode blob processes
and other processes without using LSMs, for the reality is
(1) Linux kernel community does not allow legally accessing LSM infrastructure from
loadable kernel modules since Linux 2.6.24.
(2) Red Hat folks enable only SELinux in their kernels.
(3) Customers I'm working for cannot afford enabling SELinux in their environments.
and therefore
(4) I have to maintain loadable kernel module version of LSM modules which illegally
access LSM infrastructure in order to implement single function LSM modules.
Implementing security_ptrace_access_check() hook in TOMOYO is not a solution.
>>> security label can carry that execution context.
>>
>> If files get a chance to be associated with appropriate pathname and
>> security label.
>
> I can easily add a fake pathname to the blob, but it won't help tomoyo.
> That's what I was saying all along.
> pathname based security provides false sense of security.
>
> I'm pretty sure this old blog has been read by many folks who
> are following this thread, but it's worth reminding again:
> https://securityblog.org/2006/04/19/security-anti-pattern-path-based-access-control/
> I cannot agree more with Joshua.
> Here is a quote:
> "The most obvious problem with this is that not all objects are files and thus do not have paths."
Don't you know that TOMOYO can coexist with SELinux/Smack/AppArmor since Linux 5.1 ? ;-)
^ permalink raw reply
* Re: [RFC][PATCH] net/bpfilter: Remove this broken and apparently unmantained
From: Alexei Starovoitov @ 2020-06-26 5:41 UTC (permalink / raw)
To: Tetsuo Handa
Cc: Linus Torvalds, David Miller, Greg Kroah-Hartman,
Eric W. Biederman, Kees Cook, Andrew Morton, Alexei Starovoitov,
Al Viro, bpf, linux-fsdevel, Daniel Borkmann, Jakub Kicinski,
Masahiro Yamada, Gary Lin, Bruno Meneguele, LSM List,
Casey Schaufler
In-Reply-To: <eb3bec08-9de4-c708-fb8e-b6a47145eb5e@i-love.sakura.ne.jp>
On Fri, Jun 26, 2020 at 01:58:35PM +0900, Tetsuo Handa wrote:
> On 2020/06/26 10:51, Alexei Starovoitov wrote:
> > On Thu, Jun 25, 2020 at 06:36:34PM -0700, Linus Torvalds wrote:
> >> On Thu, Jun 25, 2020 at 12:34 PM David Miller <davem@davemloft.net> wrote:
> >>>
> >>> It's kernel code executing in userspace. If you don't trust the
> >>> signed code you don't trust the signed code.
> >>>
> >>> Nothing is magic about a piece of code executing in userspace.
> >>
> >> Well, there's one real issue: the most likely thing that code is going
> >> to do is execute llvm to generate more code.
>
> Wow! Are we going to allow execution of such complicated programs?
No. llvm was _never_ intended to be run from the blob.
bpfilter was envisioned as self contained binary. If it needed to do
optimizations on generated bpf code it would have to do them internally.
> I was hoping that fork_usermode_blob() accepts only simple program
> like the content of "hello64" generated by
pretty much. statically compiled elf that is self contained.
> For example, a usermode process started by fork_usermode_blob() which was initially
> containing
>
> ----------
> while (read(0, &uid, sizeof(uid)) == sizeof(uid)) {
> if (uid == 0)
> write(1, "OK\n", 3);
> else
> write(1, "NG\n", 3);
> }
> ----------
>
> can be somehow tampered like
>
> ----------
> while (read(0, &uid, sizeof(uid)) == sizeof(uid)) {
> if (uid != 0)
> write(1, "OK\n", 3);
> else
> write(1, "NG\n", 3);
> }
> ----------
>
> due to interference from the rest of the system, how can we say "we trust kernel
> code executing in userspace" ?
I answered this already in the previous email.
Use security_ptrace_access_check() LSM hook to make sure that no other process
can tamper with blob's memory when it's running as user process.
In the future it would be trivial to add a new ptrace flag to
make sure that blob's memory is not ptraceable from the start.
> My question is: how is the byte array (which was copied from kernel space) kept secure/intact
> under "root can poke into kernel or any process memory." environment? It is obvious that
> we can't say "we trust kernel code executing in userspace" without some mechanism.
Already answered.
> Currently fork_usermode_blob() is not providing security context for the byte array to be
> executed. We could modify fork_usermode_blob() to provide security context for LSMs, but
> I'll be more happy if we can implement that mechanism without counting on in-tree LSMs, for
> SELinux is too complicated to support.
I'm pretty sure it was answered in the upthread by selinux folks.
Quick recap: we can add security labels, sha, strings, you_name_it to the blob that
lsm hooks can track.
We can also add another LSM hook to fork_usermode_blob(), so if tomoyo is so worried
about blobs it would be able to reject all of them without too much work.
>
> > I think that's Tetsuo's point about lack of LSM hooks is kernel_sock_shutdown().
> > Obviously, kernel_sock_shutdown() can be called by kernel only.
>
> I can't catch what you mean. The kernel code executing in userspace uses syscall
> interface (e.g. SYSCALL_DEFINE2(shutdown, int, fd, int, how) path), doesn't it?
yes.
> > I suspect he's imaging a hypothetical situation where kernel bits of kernel module
> > interact with userblob bits of kernel module.
> > Then another root process tampers with memory of userblob.
>
> Yes, how to protect the memory of userblob is a concern. The memory of userblob can
> interfere (or can be interfered by) the rest of the system is a problem.
answered.
> > I think this is trivially enforceable without creating new features.
> > Existing security_ptrace_access_check() LSM hook can prevent tampering with
> > memory of userblob.
>
> There is security_ptrace_access_check() LSM hook, but no zero-configuration
> method is available.
huh?
tomoyo is not using that hook, but selinux and many other LSMs do.
please learn from others.
> > security label can carry that execution context.
>
> If files get a chance to be associated with appropriate pathname and
> security label.
I can easily add a fake pathname to the blob, but it won't help tomoyo.
That's what I was saying all along.
pathname based security provides false sense of security.
I'm pretty sure this old blog has been read by many folks who
are following this thread, but it's worth reminding again:
https://securityblog.org/2006/04/19/security-anti-pattern-path-based-access-control/
I cannot agree more with Joshua.
Here is a quote:
"The most obvious problem with this is that not all objects are files and thus do not have paths."
> >> My personally strongest argument for remoiving this kernel code is
> >> that it's been there for a couple of years now, and it has never
> >> actually done anything useful, and there's no actual sign that it ever
> >> will, or that there is a solid plan in place for it.
> >
> > you probably missed the detailed plan:
> > https://lore.kernel.org/bpf/20200609235631.ukpm3xngbehfqthz@ast-mbp.dhcp.thefacebook.com/
> >
> > The project #3 is the above is the one we're working on right now.
> > It should be ready to post in a week.
>
> I got a question on project #3. Given that "cat /sys/fs/bpf/my_ipv6_route"
> produces the same human output as "cat /proc/net/ipv6_route", how security
> checks which are done for "cat /proc/net/ipv6_route" can be enforced for
> "cat /sys/fs/bpf/my_ipv6_route" ? Unless same security checks (e.g. permission
> to read /proc/net/ipv6_route ) is enforced, such bpf usage sounds like a method
> for bypassing existing security mechanisms.
Standard file permissions. Nothing to do with bpf.
^ permalink raw reply
* Re: linux-next: umh: fix processed error when UMH_WAIT_PROC is used seems to break linux bridge on s390x (bisected)
From: Christian Borntraeger @ 2020-06-26 5:22 UTC (permalink / raw)
To: Luis Chamberlain
Cc: Christoph Hellwig, ast, axboe, bfields, bridge, chainsaw,
christian.brauner, chuck.lever, davem, dhowells, gregkh,
jarkko.sakkinen, jmorris, josh, keescook, keyrings, kuba,
lars.ellenberg, linux-fsdevel, linux-kernel, linux-nfs,
linux-security-module, nikolay, philipp.reisner, ravenexp, roopa,
serge, slyfox, viro, yangtiezhu, netdev, markward, linux-s390
In-Reply-To: <20200626025410.GJ4332@42.do-not-panic.com>
On 26.06.20 04:54, Luis Chamberlain wrote:
> On Wed, Jun 24, 2020 at 08:37:55PM +0200, Christian Borntraeger wrote:
>>
>>
>> On 24.06.20 20:32, Christian Borntraeger wrote:
>> [...]>
>>> So the translations look correct. But your change is actually a sematic change
>>> if(ret) will only trigger if there is an error
>>> if (KWIFEXITED(ret)) will always trigger when the process ends. So we will always overwrite -ECHILD
>>> and we did not do it before.
>>>
>>
>> So the right fix is
>>
>> diff --git a/kernel/umh.c b/kernel/umh.c
>> index f81e8698e36e..a3a3196e84d1 100644
>> --- a/kernel/umh.c
>> +++ b/kernel/umh.c
>> @@ -154,7 +154,7 @@ static void call_usermodehelper_exec_sync(struct subprocess_info *sub_info)
>> * the real error code is already in sub_info->retval or
>> * sub_info->retval is 0 anyway, so don't mess with it then.
>> */
>> - if (KWIFEXITED(ret))
>> + if (KWEXITSTATUS(ret))
>> sub_info->retval = KWEXITSTATUS(ret);
>> }
>>
>> I think.
>
> Nope, the right form is to check for WIFEXITED() before using WEXITSTATUS().
But this IS a change over the previous code, no?
I will test next week as I am travelling right now.
^ permalink raw reply
* Re: [RFC][PATCH] net/bpfilter: Remove this broken and apparently unmantained
From: Tetsuo Handa @ 2020-06-26 4:58 UTC (permalink / raw)
To: Alexei Starovoitov, Linus Torvalds
Cc: David Miller, Greg Kroah-Hartman, Eric W. Biederman, Kees Cook,
Andrew Morton, Alexei Starovoitov, Al Viro, bpf, linux-fsdevel,
Daniel Borkmann, Jakub Kicinski, Masahiro Yamada, Gary Lin,
Bruno Meneguele, LSM List, Casey Schaufler
In-Reply-To: <20200626015121.qpxkdaqtsywe3zqx@ast-mbp.dhcp.thefacebook.com>
On 2020/06/26 10:51, Alexei Starovoitov wrote:
> On Thu, Jun 25, 2020 at 06:36:34PM -0700, Linus Torvalds wrote:
>> On Thu, Jun 25, 2020 at 12:34 PM David Miller <davem@davemloft.net> wrote:
>>>
>>> It's kernel code executing in userspace. If you don't trust the
>>> signed code you don't trust the signed code.
>>>
>>> Nothing is magic about a piece of code executing in userspace.
>>
>> Well, there's one real issue: the most likely thing that code is going
>> to do is execute llvm to generate more code.
Wow! Are we going to allow execution of such complicated programs?
I was hoping that fork_usermode_blob() accepts only simple program
like the content of "hello64" generated by
----------
; nasm -f elf64 hello64.asm && ld -s -m elf_x86_64 -o hello64 hello64.o
section .text
global _start
_start:
mov rax, 1 ; write(
mov rdi, 1 ; 1,
mov rsi, msg ; "Hello world\n",
mov rdx, 12 ; 12
syscall ; );
mov rax, 231 ; _exit(
mov rdi, 0 ; 0
syscall ; );
section .rodata
msg: db "Hello world", 0x0a
----------
which can be contained by mechanisms like seccomp; there is no pathname
resolution, no networking access etc.
>>
>> And that's I think the real security issue here: the context in which
>> the code executes. It may be triggered in one namespace, but what
>> namespaces and what rules should the thing actually then execute in.
>>
>> So no, trying to dismiss this as "there are no security issues" is
>> bogus. There very much are security issues.
>
> I think you're referring to:
>
>>> We might need to invent built-in "protected userspace" because existing
>>> "unprotected userspace" is not trustworthy enough to run kernel modules.
>>> That's not just inventing fork_usermode_blob().
>
> Another root process can modify the memory of usermode_blob process.
I'm not familiar with ptrace(); I'm just using /usr/bin/strace and /usr/bin/ltrace .
What I'm worrying is that some root process tampers with memory which initially
contained "hello64" above in order to let that memory do something different behavior.
For example, a usermode process started by fork_usermode_blob() which was initially
containing
----------
while (read(0, &uid, sizeof(uid)) == sizeof(uid)) {
if (uid == 0)
write(1, "OK\n", 3);
else
write(1, "NG\n", 3);
}
----------
can be somehow tampered like
----------
while (read(0, &uid, sizeof(uid)) == sizeof(uid)) {
if (uid != 0)
write(1, "OK\n", 3);
else
write(1, "NG\n", 3);
}
----------
due to interference from the rest of the system, how can we say "we trust kernel
code executing in userspace" ?
My question is: how is the byte array (which was copied from kernel space) kept secure/intact
under "root can poke into kernel or any process memory." environment? It is obvious that
we can't say "we trust kernel code executing in userspace" without some mechanism.
Currently fork_usermode_blob() is not providing security context for the byte array to be
executed. We could modify fork_usermode_blob() to provide security context for LSMs, but
I'll be more happy if we can implement that mechanism without counting on in-tree LSMs, for
SELinux is too complicated to support.
> I think that's Tetsuo's point about lack of LSM hooks is kernel_sock_shutdown().
> Obviously, kernel_sock_shutdown() can be called by kernel only.
I can't catch what you mean. The kernel code executing in userspace uses syscall
interface (e.g. SYSCALL_DEFINE2(shutdown, int, fd, int, how) path), doesn't it?
> I suspect he's imaging a hypothetical situation where kernel bits of kernel module
> interact with userblob bits of kernel module.
> Then another root process tampers with memory of userblob.
Yes, how to protect the memory of userblob is a concern. The memory of userblob can
interfere (or can be interfered by) the rest of the system is a problem.
> Then userblob interaction with kernel module can do kernel_sock_shutdown()
> on something that initial design of kernel+userblob module didn't intend.
I can't catch what you mean.
> I think this is trivially enforceable without creating new features.
> Existing security_ptrace_access_check() LSM hook can prevent tampering with
> memory of userblob.
There is security_ptrace_access_check() LSM hook, but no zero-configuration
method is available.
>
> As far as userblob calling llvm and other things in sequence.
> That is no different from systemd calling things.
Right.
> security label can carry that execution context.
If files get a chance to be associated with appropriate pathname and
security label.
>
>> My personally strongest argument for remoiving this kernel code is
>> that it's been there for a couple of years now, and it has never
>> actually done anything useful, and there's no actual sign that it ever
>> will, or that there is a solid plan in place for it.
>
> you probably missed the detailed plan:
> https://lore.kernel.org/bpf/20200609235631.ukpm3xngbehfqthz@ast-mbp.dhcp.thefacebook.com/
>
> The project #3 is the above is the one we're working on right now.
> It should be ready to post in a week.
I got a question on project #3. Given that "cat /sys/fs/bpf/my_ipv6_route"
produces the same human output as "cat /proc/net/ipv6_route", how security
checks which are done for "cat /proc/net/ipv6_route" can be enforced for
"cat /sys/fs/bpf/my_ipv6_route" ? Unless same security checks (e.g. permission
to read /proc/net/ipv6_route ) is enforced, such bpf usage sounds like a method
for bypassing existing security mechanisms.
^ permalink raw reply
* Re: linux-next: umh: fix processed error when UMH_WAIT_PROC is used seems to break linux bridge on s390x (bisected)
From: Luis Chamberlain @ 2020-06-26 2:54 UTC (permalink / raw)
To: Christian Borntraeger
Cc: Christoph Hellwig, ast, axboe, bfields, bridge, chainsaw,
christian.brauner, chuck.lever, davem, dhowells, gregkh,
jarkko.sakkinen, jmorris, josh, keescook, keyrings, kuba,
lars.ellenberg, linux-fsdevel, linux-kernel, linux-nfs,
linux-security-module, nikolay, philipp.reisner, ravenexp, roopa,
serge, slyfox, viro, yangtiezhu, netdev, markward, linux-s390
In-Reply-To: <1263e370-7cee-24d8-b98c-117bf7c90a83@de.ibm.com>
On Wed, Jun 24, 2020 at 08:37:55PM +0200, Christian Borntraeger wrote:
>
>
> On 24.06.20 20:32, Christian Borntraeger wrote:
> [...]>
> > So the translations look correct. But your change is actually a sematic change
> > if(ret) will only trigger if there is an error
> > if (KWIFEXITED(ret)) will always trigger when the process ends. So we will always overwrite -ECHILD
> > and we did not do it before.
> >
>
> So the right fix is
>
> diff --git a/kernel/umh.c b/kernel/umh.c
> index f81e8698e36e..a3a3196e84d1 100644
> --- a/kernel/umh.c
> +++ b/kernel/umh.c
> @@ -154,7 +154,7 @@ static void call_usermodehelper_exec_sync(struct subprocess_info *sub_info)
> * the real error code is already in sub_info->retval or
> * sub_info->retval is 0 anyway, so don't mess with it then.
> */
> - if (KWIFEXITED(ret))
> + if (KWEXITSTATUS(ret))
> sub_info->retval = KWEXITSTATUS(ret);
> }
>
> I think.
Nope, the right form is to check for WIFEXITED() before using WEXITSTATUS().
I'm not able to reproduce this on x86 with a bridge. What type of bridge
are you using on a guest, or did you mean using KVM so that the *host*
can spawn kvm guests?
It would be good if you can try to add a bridge manually and see where
things fail. Can you do something like this:
brctl addbr br0
brctl addif br0 ens6
ip link set dev br0 up
Note that most callers are for modprobe. I'd be curious to see which
umh is failing which breaks bridge for you. Can you trut this so we can
see which umh call is failing?
diff --git a/kernel/umh.c b/kernel/umh.c
index f81e8698e36e..5ad74bc301d8 100644
--- a/kernel/umh.c
+++ b/kernel/umh.c
@@ -2,6 +2,9 @@
/*
* umh - the kernel usermode helper
*/
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
#include <linux/module.h>
#include <linux/sched.h>
#include <linux/sched/task.h>
@@ -154,8 +157,12 @@ static void call_usermodehelper_exec_sync(struct subprocess_info *sub_info)
* the real error code is already in sub_info->retval or
* sub_info->retval is 0 anyway, so don't mess with it then.
*/
- if (KWIFEXITED(ret))
+ printk("== ret: %02x\n", ret);
+ printk("== KWIFEXITED(ret): %02x\n", KWIFEXITED(ret));
+ if (KWIFEXITED(ret)) {
+ printk("KWEXITSTATUS(ret): %d\n", KWEXITSTATUS(ret));
sub_info->retval = KWEXITSTATUS(ret);
+ }
}
/* Restore default kernel sig handler */
@@ -383,6 +390,7 @@ struct subprocess_info *call_usermodehelper_setup(const char *path, char **argv,
void *data)
{
struct subprocess_info *sub_info;
+ unsigned int i = 0;
sub_info = kzalloc(sizeof(struct subprocess_info), gfp_mask);
if (!sub_info)
goto out;
@@ -394,6 +402,11 @@ struct subprocess_info *call_usermodehelper_setup(const char *path, char **argv,
#else
sub_info->path = path;
#endif
+ pr_info("sub_info->path: %s\n", sub_info->path);
+ while (argv[i])
+ printk(KERN_INFO "%s ", argv[i++]);
+ printk(KERN_INFO "\n");
+
sub_info->argv = argv;
sub_info->envp = envp;
^ permalink raw reply related
* Re: [PATCH] ima: AppArmor satisfies the audit rule requirements
From: Mimi Zohar @ 2020-06-26 2:34 UTC (permalink / raw)
To: Tyler Hicks, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Matthew Garrett, linux-kernel,
linux-integrity, linux-security-module
In-Reply-To: <20200623233823.904882-1-tyhicks@linux.microsoft.com>
On Tue, 2020-06-23 at 18:38 -0500, Tyler Hicks wrote:
> AppArmor meets all the requirements for IMA in terms of audit rules
> since commit e79c26d04043 ("apparmor: Add support for audit rule
> filtering"). Update IMA's Kconfig section for CONFIG_IMA_LSM_RULES to
> reflect this.
>
> Fixes: e79c26d04043 ("apparmor: Add support for audit rule filtering")
> Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
Thanks
Mimi
^ permalink raw reply
* Re: [RFC][PATCH] net/bpfilter: Remove this broken and apparently unmantained
From: Alexei Starovoitov @ 2020-06-26 1:51 UTC (permalink / raw)
To: Linus Torvalds
Cc: David Miller, Greg Kroah-Hartman, Tetsuo Handa, Eric W. Biederman,
Kees Cook, Andrew Morton, Alexei Starovoitov, Al Viro, bpf,
linux-fsdevel, Daniel Borkmann, Jakub Kicinski, Masahiro Yamada,
Gary Lin, Bruno Meneguele, LSM List, Casey Schaufler
In-Reply-To: <CAHk-=whuTwGHEPjvtbBvneHHXeqJC=q5S09mbPnqb=Q+MSPMag@mail.gmail.com>
On Thu, Jun 25, 2020 at 06:36:34PM -0700, Linus Torvalds wrote:
> On Thu, Jun 25, 2020 at 12:34 PM David Miller <davem@davemloft.net> wrote:
> >
> > It's kernel code executing in userspace. If you don't trust the
> > signed code you don't trust the signed code.
> >
> > Nothing is magic about a piece of code executing in userspace.
>
> Well, there's one real issue: the most likely thing that code is going
> to do is execute llvm to generate more code.
>
> And that's I think the real security issue here: the context in which
> the code executes. It may be triggered in one namespace, but what
> namespaces and what rules should the thing actually then execute in.
>
> So no, trying to dismiss this as "there are no security issues" is
> bogus. There very much are security issues.
I think you're referring to:
>> We might need to invent built-in "protected userspace" because existing
>> "unprotected userspace" is not trustworthy enough to run kernel modules.
>> That's not just inventing fork_usermode_blob().
Another root process can modify the memory of usermode_blob process.
I think that's Tetsuo's point about lack of LSM hooks is kernel_sock_shutdown().
Obviously, kernel_sock_shutdown() can be called by kernel only.
I suspect he's imaging a hypothetical situation where kernel bits of kernel module
interact with userblob bits of kernel module.
Then another root process tampers with memory of userblob.
Then userblob interaction with kernel module can do kernel_sock_shutdown()
on something that initial design of kernel+userblob module didn't intend.
I think this is trivially enforceable without creating new features.
Existing security_ptrace_access_check() LSM hook can prevent tampering with
memory of userblob.
As far as userblob calling llvm and other things in sequence.
That is no different from systemd calling things.
security label can carry that execution context.
> My personally strongest argument for remoiving this kernel code is
> that it's been there for a couple of years now, and it has never
> actually done anything useful, and there's no actual sign that it ever
> will, or that there is a solid plan in place for it.
you probably missed the detailed plan:
https://lore.kernel.org/bpf/20200609235631.ukpm3xngbehfqthz@ast-mbp.dhcp.thefacebook.com/
The project #3 is the above is the one we're working on right now.
It should be ready to post in a week.
> We can dance around the "what about security modules", but that
> fundamental problem of "this code hasn't done anything useful for two
> years and we don't even know if it's the right thing to do or what the
> real security issues _will_ be" is I think the real issue here.
Please see above link. bpfilter didn't go anywhere, but fork_usermode_blob()
has plenty of use cases that will materialize soon.
^ permalink raw reply
* Re: [RFC][PATCH] net/bpfilter: Remove this broken and apparently unmantained
From: Linus Torvalds @ 2020-06-26 1:36 UTC (permalink / raw)
To: David Miller
Cc: Greg Kroah-Hartman, Tetsuo Handa, Alexei Starovoitov,
Eric W. Biederman, Kees Cook, Andrew Morton, Alexei Starovoitov,
Al Viro, bpf, linux-fsdevel, Daniel Borkmann, Jakub Kicinski,
Masahiro Yamada, Gary Lin, Bruno Meneguele, LSM List,
Casey Schaufler
In-Reply-To: <20200625.123437.2219826613137938086.davem@davemloft.net>
On Thu, Jun 25, 2020 at 12:34 PM David Miller <davem@davemloft.net> wrote:
>
> It's kernel code executing in userspace. If you don't trust the
> signed code you don't trust the signed code.
>
> Nothing is magic about a piece of code executing in userspace.
Well, there's one real issue: the most likely thing that code is going
to do is execute llvm to generate more code.
And that's I think the real security issue here: the context in which
the code executes. It may be triggered in one namespace, but what
namespaces and what rules should the thing actually then execute in.
So no, trying to dismiss this as "there are no security issues" is
bogus. There very much are security issues.
It's just that the current code that is just a dummy wrapper around
something that doesn't actually do anything doesn't happen to _show_
those issues, because it does nothing.
I've stayed away from this discussion because I wanted to see if it
went anywhere, but it doesn't seem to.
My personally strongest argument for remoiving this kernel code is
that it's been there for a couple of years now, and it has never
actually done anything useful, and there's no actual sign that it ever
will, or that there is a solid plan in place for it.
So to me, it really looks like it was an interesting idea, but one
that hasn't proven itself, and most certainly not one that has shown
itself to be the _right_ idea.
We can dance around the "what about security modules", but that
fundamental problem of "this code hasn't done anything useful for two
years and we don't even know if it's the right thing to do or what the
real security issues _will_ be" is I think the real issue here.
Hmm?
Linus
^ permalink raw reply
* Re: Enabling interrupts in QEMU TPM TIS
From: Jason Gunthorpe @ 2020-06-25 23:19 UTC (permalink / raw)
To: Stefan Berger
Cc: Jerry Snitselaar, linux-integrity, Jarkko Sakkinen, LSM List,
LKML
In-Reply-To: <5b3267b7-57d5-badf-6664-9d47bc9909e7@linux.ibm.com>
On Thu, Jun 25, 2020 at 06:48:09PM -0400, Stefan Berger wrote:
> On 6/25/20 5:26 PM, Stefan Berger wrote:
> > On 6/25/20 1:28 PM, Jason Gunthorpe wrote:
> > > On Thu, Jun 25, 2020 at 10:56:43AM -0400, Stefan Berger wrote:
> > > > Hello!
> > > >
> > > > I want to enable IRQs now in QEMU's TPM TIS device model and I
> > > > need to work
> > > > with the following patch to Linux TIS. I am wondering whether
> > > > the changes
> > > > there look reasonable to you? Windows works with the QEMU modifications
> > > > as-is, so maybe it's a bug in the TIS code (which I had not run into
> > > > before).
> > > >
> > > >
> > > > The point of the loop I need to introduce in the interrupt
> > > > handler is that
> > > > while the interrupt handler is running another interrupt may
> > > > occur/be posted
> > > > that then does NOT cause the interrupt handler to be invoked again but
> > > > causes a stall, unless the loop is there.
> > > That seems like a qemu bug, TPM interrupts are supposed to be level
> > > interrupts, not edge.
> >
> >
> > Following this document here the hardware may choose to support
> > different types of interrutps:
> >
> > https://trustedcomputinggroup.org/wp-content/uploads/PC-Client-Specific-Platform-TPM-Profile-for-TPM-2p0-v1p04_r0p37_pub-1.pdf
> >
> >
> > Table 23. Edge falling or rising, level low or level high.
> >
> > So with different steps in the driver causing different types of
> > interrupts, we may get into such situations where we process some
> > interrupt 'reasons' but then another one gets posted, I guess due to
> > parallel processing.
>
>
> Another data point: I had the TIS driver working on IRQ 5 (festeoi) without
> the introduction of this loop. There are additional bits being set while the
> interrupt handler is running, but the handler deals with them in the next
> invocation. On IRQ 13 (edge), it does need the loop since the next interrupt
> handler invocation is not happening.
A loop like that is never the correct way to handle edge interrupts.
I don't think the tpm driver was ever designed for edge, so most
likely the structure and order of the hard irq is not correct.
Jason
^ permalink raw reply
* Re: [PATCH 12/12] ima: Support additional conditionals in the KEXEC_CMDLINE hook function
From: Tyler Hicks @ 2020-06-25 22:59 UTC (permalink / raw)
To: Mimi Zohar
Cc: Dmitry Kasatkin, James Morris, Serge E . Hallyn,
Lakshmi Ramasubramanian, Prakhar Srivastava, linux-kernel,
linux-integrity, linux-security-module, Eric Biederman, kexec
In-Reply-To: <1593125804.27152.426.camel@linux.ibm.com>
On 2020-06-25 18:56:44, Mimi Zohar wrote:
> On Mon, 2020-06-22 at 19:32 -0500, Tyler Hicks wrote:
> > Take the properties of the kexec kernel's inode and the current task
> > ownership into consideration when matching a KEXEC_CMDLINE operation to
> > the rules in the IMA policy. This allows for some uniformity when
> > writing IMA policy rules for KEXEC_KERNEL_CHECK, KEXEC_INITRAMFS_CHECK,
> > and KEXEC_CMDLINE operations.
> >
> > Prior to this patch, it was not possible to write a set of rules like
> > this:
> >
> > dont_measure func=KEXEC_KERNEL_CHECK obj_type=foo_t
> > dont_measure func=KEXEC_INITRAMFS_CHECK obj_type=foo_t
> > dont_measure func=KEXEC_CMDLINE obj_type=foo_t
> > measure func=KEXEC_KERNEL_CHECK
> > measure func=KEXEC_INITRAMFS_CHECK
> > measure func=KEXEC_CMDLINE
> >
> > The inode information associated with the kernel being loaded by a
> > kexec_kernel_load(2) syscall can now be included in the decision to
> > measure or not
> >
> > Additonally, the uid, euid, and subj_* conditionals can also now be
> > used in KEXEC_CMDLINE rules. There was no technical reason as to why
> > those conditionals weren't being considered previously other than
> > ima_match_rules() didn't have a valid inode to use so it immediately
> > bailed out for KEXEC_CMDLINE operations rather than going through the
> > full list of conditional comparisons.
>
> This makes a lot of sense.
>
> <snip>
>
> > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > index c1583d98c5e5..82acd66bf653 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -731,13 +731,15 @@ int ima_load_data(enum kernel_load_data_id id)
> > * @eventname: event name to be used for the buffer entry.
> > * @func: IMA hook
> > * @pcr: pcr to extend the measurement
> > + * @inode: inode associated with the object being measured (NULL for KEY_CHECK)
> > * @keyring: keyring name to determine the action to be performed
> > *
> > * Based on policy, the buffer is measured into the ima log.
> > */
> > void process_buffer_measurement(const void *buf, int size,
> > const char *eventname, enum ima_hooks func,
> > - int pcr, const char *keyring)
> > + int pcr, struct inode *inode,
> > + const char *keyring)
> > {
>
> The file descriptor is passed as the first arg to
> process_measurement(). Sorry for the patch churn, but could we do the
> same for process_buffer_measurements. As much as possible lets keep
> them in same.
Yep! That makes sense to me.
Tyler
>
> thanks,
>
> Mimi
^ permalink raw reply
* Re: [PATCH 12/12] ima: Support additional conditionals in the KEXEC_CMDLINE hook function
From: Mimi Zohar @ 2020-06-25 22:56 UTC (permalink / raw)
To: Tyler Hicks, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
Prakhar Srivastava, linux-kernel, linux-integrity,
linux-security-module, Eric Biederman, kexec
In-Reply-To: <20200623003236.830149-13-tyhicks@linux.microsoft.com>
On Mon, 2020-06-22 at 19:32 -0500, Tyler Hicks wrote:
> Take the properties of the kexec kernel's inode and the current task
> ownership into consideration when matching a KEXEC_CMDLINE operation to
> the rules in the IMA policy. This allows for some uniformity when
> writing IMA policy rules for KEXEC_KERNEL_CHECK, KEXEC_INITRAMFS_CHECK,
> and KEXEC_CMDLINE operations.
>
> Prior to this patch, it was not possible to write a set of rules like
> this:
>
> dont_measure func=KEXEC_KERNEL_CHECK obj_type=foo_t
> dont_measure func=KEXEC_INITRAMFS_CHECK obj_type=foo_t
> dont_measure func=KEXEC_CMDLINE obj_type=foo_t
> measure func=KEXEC_KERNEL_CHECK
> measure func=KEXEC_INITRAMFS_CHECK
> measure func=KEXEC_CMDLINE
>
> The inode information associated with the kernel being loaded by a
> kexec_kernel_load(2) syscall can now be included in the decision to
> measure or not
>
> Additonally, the uid, euid, and subj_* conditionals can also now be
> used in KEXEC_CMDLINE rules. There was no technical reason as to why
> those conditionals weren't being considered previously other than
> ima_match_rules() didn't have a valid inode to use so it immediately
> bailed out for KEXEC_CMDLINE operations rather than going through the
> full list of conditional comparisons.
This makes a lot of sense.
<snip>
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index c1583d98c5e5..82acd66bf653 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -731,13 +731,15 @@ int ima_load_data(enum kernel_load_data_id id)
> * @eventname: event name to be used for the buffer entry.
> * @func: IMA hook
> * @pcr: pcr to extend the measurement
> + * @inode: inode associated with the object being measured (NULL for KEY_CHECK)
> * @keyring: keyring name to determine the action to be performed
> *
> * Based on policy, the buffer is measured into the ima log.
> */
> void process_buffer_measurement(const void *buf, int size,
> const char *eventname, enum ima_hooks func,
> - int pcr, const char *keyring)
> + int pcr, struct inode *inode,
> + const char *keyring)
> {
The file descriptor is passed as the first arg to
process_measurement(). Sorry for the patch churn, but could we do the
same for process_buffer_measurements. As much as possible lets keep
them in same.
thanks,
Mimi
^ permalink raw reply
* Re: Enabling interrupts in QEMU TPM TIS
From: Stefan Berger @ 2020-06-25 22:48 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Jerry Snitselaar, linux-integrity, Jarkko Sakkinen, LSM List,
LKML
In-Reply-To: <0a524093-e744-e266-6087-ddc17b5c598c@linux.ibm.com>
On 6/25/20 5:26 PM, Stefan Berger wrote:
> On 6/25/20 1:28 PM, Jason Gunthorpe wrote:
>> On Thu, Jun 25, 2020 at 10:56:43AM -0400, Stefan Berger wrote:
>>> Hello!
>>>
>>> I want to enable IRQs now in QEMU's TPM TIS device model and I
>>> need to work
>>> with the following patch to Linux TIS. I am wondering whether the
>>> changes
>>> there look reasonable to you? Windows works with the QEMU modifications
>>> as-is, so maybe it's a bug in the TIS code (which I had not run into
>>> before).
>>>
>>>
>>> The point of the loop I need to introduce in the interrupt handler
>>> is that
>>> while the interrupt handler is running another interrupt may
>>> occur/be posted
>>> that then does NOT cause the interrupt handler to be invoked again but
>>> causes a stall, unless the loop is there.
>> That seems like a qemu bug, TPM interrupts are supposed to be level
>> interrupts, not edge.
>
>
> Following this document here the hardware may choose to support
> different types of interrutps:
>
> https://trustedcomputinggroup.org/wp-content/uploads/PC-Client-Specific-Platform-TPM-Profile-for-TPM-2p0-v1p04_r0p37_pub-1.pdf
>
>
> Table 23. Edge falling or rising, level low or level high.
>
> So with different steps in the driver causing different types of
> interrupts, we may get into such situations where we process some
> interrupt 'reasons' but then another one gets posted, I guess due to
> parallel processing.
Another data point: I had the TIS driver working on IRQ 5 (festeoi)
without the introduction of this loop. There are additional bits being
set while the interrupt handler is running, but the handler deals with
them in the next invocation. On IRQ 13 (edge), it does need the loop
since the next interrupt handler invocation is not happening. That IRQ
13 is an edge interrupt, is this a hard-configured PC 'thing'? Windows
drove this to IRQ 13, which seemed to be the only one accepted by it and
iirc it wouldn't even touch the TIS (found via tracing) if another IRQ
than 13 was declared in the ACPI table.
>
> Stefan
>
>
^ permalink raw reply
* Re: [PATCH 11/12] ima: Use the common function to detect LSM conditionals in a rule
From: Mimi Zohar @ 2020-06-25 22:45 UTC (permalink / raw)
To: Tyler Hicks, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
Prakhar Srivastava, linux-kernel, linux-integrity,
linux-security-module
In-Reply-To: <20200623003236.830149-12-tyhicks@linux.microsoft.com>
On Mon, 2020-06-22 at 19:32 -0500, Tyler Hicks wrote:
> Make broader use of ima_rule_contains_lsm_cond() to check if a given
> rule contains an LSM conditional. This is a code cleanup and has no
> user-facing change.
>
> Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
Mimi
^ permalink raw reply
* Re: [PATCH 06/12] ima: Fail rule parsing when the KEXEC_CMDLINE hook is combined with an invalid cond
From: Mimi Zohar @ 2020-06-25 21:53 UTC (permalink / raw)
To: Tyler Hicks, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
Prakhar Srivastava, linux-kernel, linux-integrity,
linux-security-module
In-Reply-To: <20200623003236.830149-7-tyhicks@linux.microsoft.com>
On Mon, 2020-06-22 at 19:32 -0500, Tyler Hicks wrote:
> The KEXEC_CMDLINE hook function only supports the pcr conditional. Make
> this clear at policy load so that IMA policy authors don't assume that
> other conditionals are supported.
>
> Since KEXEC_CMDLINE's inception, ima_match_rules() has always returned
> true on any loaded KEXEC_CMDLINE rule without any consideration for
> other conditionals present in the rule. Make it clear that pcr is the
> only supported KEXEC_CMDLINE conditional by returning an error during
> policy load.
>
> An example of why this is a problem can be explained with the following
> rule:
>
> dont_measure func=KEXEC_CMDLINE obj_type=foo_t
>
> An IMA policy author would have assumed that rule is valid because the
> parser accepted it but the result was that measurements for all
> KEXEC_CMDLINE operations would be disabled.
>
> Fixes: b0935123a183 ("IMA: Define a new hook to measure the kexec boot command line arguments")
> Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
^ permalink raw reply
* Re: [PATCH 05/12] ima: Fail rule parsing when buffer hook functions have an invalid action
From: Mimi Zohar @ 2020-06-25 21:51 UTC (permalink / raw)
To: Tyler Hicks, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
Prakhar Srivastava, linux-kernel, linux-integrity,
linux-security-module
In-Reply-To: <20200623003236.830149-6-tyhicks@linux.microsoft.com>
On Mon, 2020-06-22 at 19:32 -0500, Tyler Hicks wrote:
> Buffer based hook functions, such as KEXEC_CMDLINE and KEY_CHECK, can
> only measure. The process_buffer_measurement() function quietly ignores
> all actions except measure so make this behavior clear at the time of
> policy load.
>
> The parsing of the keyrings conditional had a check to ensure that it
> was only specified with measure actions but the check should be on the
> hook function and not the keyrings conditional since
> "appraise func=KEY_CHECK" is not a valid rule.
>
> Fixes: b0935123a183 ("IMA: Define a new hook to measure the kexec boot command line arguments")
> Fixes: 5808611cccb2 ("IMA: Add KEY_CHECK func to measure keys")
> Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> ---
> security/integrity/ima/ima_policy.c | 36 +++++++++++++++++++++++++++--
> 1 file changed, 34 insertions(+), 2 deletions(-)
>
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index ee5152ecd3d9..ecc234b956a2 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -979,6 +979,39 @@ static void check_template_modsig(const struct ima_template_desc *template)
> #undef MSG
> }
>
> +static bool ima_validate_rule(struct ima_rule_entry *entry)
> +{
> + if (entry->action == UNKNOWN)
> + return false;
> +
> + if (entry->flags & IMA_FUNC) {
> + switch (entry->func) {
> + case NONE:
> + case FILE_CHECK:
> + case MMAP_CHECK:
> + case BPRM_CHECK:
> + case CREDS_CHECK:
> + case POST_SETATTR:
> + case MODULE_CHECK:
> + case FIRMWARE_CHECK:
> + case KEXEC_KERNEL_CHECK:
> + case KEXEC_INITRAMFS_CHECK:
> + case POLICY_CHECK:
> + break;
> + case KEXEC_CMDLINE:
> + case KEY_CHECK:
> + if (entry->action & ~(MEASURE | DONT_MEASURE))
> + return false;
> +
> + break;
> + default:
> + return false;
> + }
> + }
> +
> + return true;
> +}
> +
Good idea. There are a couple of other examples that could be cleaned
up as well. For example, for performance reasons
"appraise_flag=check_blacklist" is limited to files with appended
signatures, like kernel modules and the kexec kernel image
(OpenPower).
Mimi
^ permalink raw reply
* [PATCH v7 2/2] tpm: Add support for event log pointer found in TPM2 ACPI table
From: Stefan Berger @ 2020-06-25 21:50 UTC (permalink / raw)
To: linux-integrity, linux-kernel, jarkko.sakkinen, linux-acpi,
linux-security-module
Cc: Stefan Berger
In-Reply-To: <20200625215000.2052086-1-stefanb@linux.vnet.ibm.com>
From: Stefan Berger <stefanb@linux.ibm.com>
In case a TPM2 is attached, search for a TPM2 ACPI table when trying
to get the event log from ACPI. If one is found, use it to get the
start and length of the log area. This allows non-UEFI systems, such
as SeaBIOS, to pass an event log when using a TPM2.
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
drivers/char/tpm/eventlog/acpi.c | 62 +++++++++++++++++++++-----------
1 file changed, 41 insertions(+), 21 deletions(-)
diff --git a/drivers/char/tpm/eventlog/acpi.c b/drivers/char/tpm/eventlog/acpi.c
index 63ada5e53f13..e2258cfa6cb1 100644
--- a/drivers/char/tpm/eventlog/acpi.c
+++ b/drivers/char/tpm/eventlog/acpi.c
@@ -49,9 +49,9 @@ int tpm_read_log_acpi(struct tpm_chip *chip)
void __iomem *virt;
u64 len, start;
struct tpm_bios_log *log;
-
- if (chip->flags & TPM_CHIP_FLAG_TPM2)
- return -ENODEV;
+ struct acpi_table_tpm2 *tbl;
+ struct acpi_tpm2_phy *t2phy;
+ int format;
log = &chip->log;
@@ -61,23 +61,43 @@ int tpm_read_log_acpi(struct tpm_chip *chip)
if (!chip->acpi_dev_handle)
return -ENODEV;
- /* Find TCPA entry in RSDT (ACPI_LOGICAL_ADDRESSING) */
- status = acpi_get_table(ACPI_SIG_TCPA, 1,
- (struct acpi_table_header **)&buff);
-
- if (ACPI_FAILURE(status))
- return -ENODEV;
-
- switch(buff->platform_class) {
- case BIOS_SERVER:
- len = buff->server.log_max_len;
- start = buff->server.log_start_addr;
- break;
- case BIOS_CLIENT:
- default:
- len = buff->client.log_max_len;
- start = buff->client.log_start_addr;
- break;
+ if (chip->flags & TPM_CHIP_FLAG_TPM2) {
+ status = acpi_get_table("TPM2", 1,
+ (struct acpi_table_header **)&tbl);
+ if (ACPI_FAILURE(status))
+ return -ENODEV;
+
+ if (tbl->header.length <
+ sizeof(*tbl) + sizeof(struct acpi_tpm2_phy))
+ return -ENODEV;
+
+ t2phy = (void *)tbl + sizeof(*tbl);
+ len = t2phy->log_area_minimum_length;
+
+ start = t2phy->log_area_start_address;
+ if (!start || !len)
+ return -ENODEV;
+
+ format = EFI_TCG2_EVENT_LOG_FORMAT_TCG_2;
+ } else {
+ /* Find TCPA entry in RSDT (ACPI_LOGICAL_ADDRESSING) */
+ status = acpi_get_table(ACPI_SIG_TCPA, 1,
+ (struct acpi_table_header **)&buff);
+ if (ACPI_FAILURE(status))
+ return -ENODEV;
+
+ switch (buff->platform_class) {
+ case BIOS_SERVER:
+ len = buff->server.log_max_len;
+ start = buff->server.log_start_addr;
+ break;
+ case BIOS_CLIENT:
+ default:
+ len = buff->client.log_max_len;
+ start = buff->client.log_start_addr;
+ break;
+ }
+ format = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
}
if (!len) {
dev_warn(&chip->dev, "%s: TCPA log area empty\n", __func__);
@@ -98,7 +118,7 @@ int tpm_read_log_acpi(struct tpm_chip *chip)
memcpy_fromio(log->bios_event_log, virt, len);
acpi_os_unmap_iomem(virt, len);
- return EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
+ return format;
err:
kfree(log->bios_event_log);
--
2.26.2
^ permalink raw reply related
* [PATCH v7 1/2] acpi: Extend TPM2 ACPI table with missing log fields
From: Stefan Berger @ 2020-06-25 21:49 UTC (permalink / raw)
To: linux-integrity, linux-kernel, jarkko.sakkinen, linux-acpi,
linux-security-module
Cc: Stefan Berger, Rafael J . Wysocki
In-Reply-To: <20200625215000.2052086-1-stefanb@linux.vnet.ibm.com>
From: Stefan Berger <stefanb@linux.ibm.com>
Recent extensions of the TPM2 ACPI table added 3 more fields
including 12 bytes of start method specific parameters and Log Area
Minimum Length (u32) and Log Area Start Address (u64). So, we define
a new structure acpi_tpm2_phy that holds these optional new fields.
The new fields allow non-UEFI systems to access the TPM2's log.
The specification that has the new fields is the following:
TCG ACPI Specification
Family "1.2" and "2.0"
Version 1.2, Revision 8
https://trustedcomputinggroup.org/wp-content/uploads/TCG_ACPIGeneralSpecification_v1.20_r8.pdf
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
Cc: linux-acpi@vger.kernel.org
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
include/acpi/actbl3.h | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/include/acpi/actbl3.h b/include/acpi/actbl3.h
index b0b163b9efc6..bdcac69fa6bd 100644
--- a/include/acpi/actbl3.h
+++ b/include/acpi/actbl3.h
@@ -415,6 +415,13 @@ struct acpi_table_tpm2 {
/* Platform-specific data follows */
};
+/* Optional trailer for revision 4 holding platform-specific data */
+struct acpi_tpm2_phy {
+ u8 start_method_specific[12];
+ u32 log_area_minimum_length;
+ u64 log_area_start_address;
+};
+
/* Values for start_method above */
#define ACPI_TPM2_NOT_ALLOWED 0
--
2.26.2
^ permalink raw reply related
* [PATCH v7 0/2] tpm2: Make TPM2 logs accessible for non-UEFI firmware
From: Stefan Berger @ 2020-06-25 21:49 UTC (permalink / raw)
To: linux-integrity, linux-kernel, jarkko.sakkinen, linux-acpi,
linux-security-module
Cc: Stefan Berger
From: Stefan Berger <stefanb@linux.ibm.com>
This series of patches adds an optional extensions for the TPM2 ACPI table
with additional fields found in the TPM2 TCG ACPI specification (reference
is in the patch) that allow access to the log's address and its size. We
then modify the code that so far only enables access to a TPM 1.2's log for
a TPM2 as well. This then enables access to the TPM2's log on non-UEFI
system that for example run SeaBIOS.
Stefan
v6->v7:
- Added empty lines and R-b.
v5->v6:
- Moved extensions of TPM2 table into acpi_tpm2_phy.
v4->v5:
- Added R-bs and A-bs.
v3->v4:
- Repost as one series
v2->v3:
- Split the series into two separate patches
- Added comments to ACPI table fields
- Added check for null pointer to log area and zero log size
v1->v2:
- Repost of the series
Stefan Berger (2):
acpi: Extend TPM2 ACPI table with missing log fields
tpm: Add support for event log pointer found in TPM2 ACPI table
drivers/char/tpm/eventlog/acpi.c | 62 +++++++++++++++++++++-----------
include/acpi/actbl3.h | 7 ++++
2 files changed, 48 insertions(+), 21 deletions(-)
--
2.26.2
^ permalink raw reply
* Re: [PATCH v6 2/2] tpm: Add support for event log pointer found in TPM2 ACPI table
From: Jarkko Sakkinen @ 2020-06-25 21:35 UTC (permalink / raw)
To: Stefan Berger
Cc: linux-integrity, linux-kernel, linux-acpi, linux-security-module,
Stefan Berger
In-Reply-To: <20200625124222.1954580-3-stefanb@linux.vnet.ibm.com>
On Thu, Jun 25, 2020 at 08:42:22AM -0400, Stefan Berger wrote:
> From: Stefan Berger <stefanb@linux.ibm.com>
>
> In case a TPM2 is attached, search for a TPM2 ACPI table when trying
> to get the event log from ACPI. If one is found, use it to get the
> start and length of the log area. This allows non-UEFI systems, such
> as SeaBIOS, to pass an event log when using a TPM2.
>
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
> drivers/char/tpm/eventlog/acpi.c | 59 ++++++++++++++++++++------------
> 1 file changed, 38 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/char/tpm/eventlog/acpi.c b/drivers/char/tpm/eventlog/acpi.c
> index 63ada5e53f13..8b9e33d57f70 100644
> --- a/drivers/char/tpm/eventlog/acpi.c
> +++ b/drivers/char/tpm/eventlog/acpi.c
> @@ -49,9 +49,9 @@ int tpm_read_log_acpi(struct tpm_chip *chip)
> void __iomem *virt;
> u64 len, start;
> struct tpm_bios_log *log;
> -
> - if (chip->flags & TPM_CHIP_FLAG_TPM2)
> - return -ENODEV;
> + struct acpi_table_tpm2 *tbl;
> + struct acpi_tpm2_phy *t2phy;
> + int format;
>
> log = &chip->log;
>
> @@ -61,23 +61,40 @@ int tpm_read_log_acpi(struct tpm_chip *chip)
> if (!chip->acpi_dev_handle)
> return -ENODEV;
>
> - /* Find TCPA entry in RSDT (ACPI_LOGICAL_ADDRESSING) */
> - status = acpi_get_table(ACPI_SIG_TCPA, 1,
> - (struct acpi_table_header **)&buff);
> -
> - if (ACPI_FAILURE(status))
> - return -ENODEV;
> -
> - switch(buff->platform_class) {
> - case BIOS_SERVER:
> - len = buff->server.log_max_len;
> - start = buff->server.log_start_addr;
> - break;
> - case BIOS_CLIENT:
> - default:
> - len = buff->client.log_max_len;
> - start = buff->client.log_start_addr;
> - break;
> + if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> + status = acpi_get_table("TPM2", 1,
> + (struct acpi_table_header **)&tbl);
> + if (ACPI_FAILURE(status))
> + return -ENODEV;
Add empty line.
> + if (tbl->header.length <
> + sizeof(*tbl) + sizeof(struct acpi_tpm2_phy))
> + return -ENODEV;
Ditto.
> + t2phy = (void *)tbl + sizeof(*tbl);
> + len = t2phy->log_area_minimum_length;
Ditto.
> + start = t2phy->log_area_start_address;
> + if (!start || !len)
> + return -ENODEV;
Ditto.
> + format = EFI_TCG2_EVENT_LOG_FORMAT_TCG_2;
> + } else {
> + /* Find TCPA entry in RSDT (ACPI_LOGICAL_ADDRESSING) */
> + status = acpi_get_table(ACPI_SIG_TCPA, 1,
> + (struct acpi_table_header **)&buff);
> +
Remove this empty line.
> + if (ACPI_FAILURE(status))
> + return -ENODEV;
> +
> + switch (buff->platform_class) {
> + case BIOS_SERVER:
> + len = buff->server.log_max_len;
> + start = buff->server.log_start_addr;
> + break;
> + case BIOS_CLIENT:
> + default:
> + len = buff->client.log_max_len;
> + start = buff->client.log_start_addr;
> + break;
> + }
> + format = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
> }
> if (!len) {
> dev_warn(&chip->dev, "%s: TCPA log area empty\n", __func__);
> @@ -98,7 +115,7 @@ int tpm_read_log_acpi(struct tpm_chip *chip)
> memcpy_fromio(log->bios_event_log, virt, len);
>
> acpi_os_unmap_iomem(virt, len);
> - return EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
> + return format;
>
> err:
> kfree(log->bios_event_log);
> --
> 2.26.2
/Jarkko
^ permalink raw reply
* Re: [PATCH v6 1/2] acpi: Extend TPM2 ACPI table with missing log fields
From: Jarkko Sakkinen @ 2020-06-25 21:26 UTC (permalink / raw)
To: Stefan Berger
Cc: linux-integrity, linux-kernel, linux-acpi, linux-security-module,
Stefan Berger, Rafael J . Wysocki, Jiandi An
In-Reply-To: <20200625124222.1954580-2-stefanb@linux.vnet.ibm.com>
On Thu, Jun 25, 2020 at 08:42:21AM -0400, Stefan Berger wrote:
> From: Stefan Berger <stefanb@linux.ibm.com>
>
> Recent extensions of the TPM2 ACPI table added 3 more fields
> including 12 bytes of start method specific parameters and Log Area
> Minimum Length (u32) and Log Area Start Address (u64). So, we define
> a new structure acpi_tpm2_phy that holds these optional new fields.
> The new fields allow non-UEFI systems to access the TPM2's log.
>
> The specification that has the new fields is the following:
> TCG ACPI Specification
> Family "1.2" and "2.0"
> Version 1.2, Revision 8
>
> https://trustedcomputinggroup.org/wp-content/uploads/TCG_ACPIGeneralSpecification_v1.20_r8.pdf
>
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> Cc: linux-acpi@vger.kernel.org
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Jiandi An <anjiandi@codeaurora.org>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
/Jarkko
^ permalink raw reply
* Re: Enabling interrupts in QEMU TPM TIS
From: Stefan Berger @ 2020-06-25 21:26 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Jerry Snitselaar, linux-integrity, Jarkko Sakkinen, LSM List,
LKML
In-Reply-To: <20200625172802.GS6578@ziepe.ca>
On 6/25/20 1:28 PM, Jason Gunthorpe wrote:
> On Thu, Jun 25, 2020 at 10:56:43AM -0400, Stefan Berger wrote:
>> Hello!
>>
>> I want to enable IRQs now in QEMU's TPM TIS device model and I need to work
>> with the following patch to Linux TIS. I am wondering whether the changes
>> there look reasonable to you? Windows works with the QEMU modifications
>> as-is, so maybe it's a bug in the TIS code (which I had not run into
>> before).
>>
>>
>> The point of the loop I need to introduce in the interrupt handler is that
>> while the interrupt handler is running another interrupt may occur/be posted
>> that then does NOT cause the interrupt handler to be invoked again but
>> causes a stall, unless the loop is there.
> That seems like a qemu bug, TPM interrupts are supposed to be level
> interrupts, not edge.
Following this document here the hardware may choose to support
different types of interrutps:
https://trustedcomputinggroup.org/wp-content/uploads/PC-Client-Specific-Platform-TPM-Profile-for-TPM-2p0-v1p04_r0p37_pub-1.pdf
Table 23. Edge falling or rising, level low or level high.
So with different steps in the driver causing different types of
interrupts, we may get into such situations where we process some
interrupt 'reasons' but then another one gets posted, I guess due to
parallel processing.
Stefan
^ permalink raw reply
* Re: [PATCH 09/12] ima: Use correct type for the args_p member of ima_rule_entry.lsm elements
From: Mimi Zohar @ 2020-06-25 21:20 UTC (permalink / raw)
To: Tyler Hicks, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
Prakhar Srivastava, linux-kernel, linux-integrity,
linux-security-module
In-Reply-To: <20200623003236.830149-10-tyhicks@linux.microsoft.com>
On Mon, 2020-06-22 at 19:32 -0500, Tyler Hicks wrote:
> Make args_p be of the char pointer type rather than have it be a void
> pointer that gets casted to char pointer when it is used. It is a simple
> NUL-terminated string as returned by match_strdup().
>
> Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
Thanks!
Mimi
^ permalink raw reply
* Re: [PATCH 08/12] ima: Shallow copy the args_p member of ima_rule_entry.lsm elements
From: Mimi Zohar @ 2020-06-25 21:18 UTC (permalink / raw)
To: Tyler Hicks, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
Prakhar Srivastava, linux-kernel, linux-integrity,
linux-security-module
In-Reply-To: <20200623003236.830149-9-tyhicks@linux.microsoft.com>
On Mon, 2020-06-22 at 19:32 -0500, Tyler Hicks wrote:
> The args_p member is a simple string that is allocated by
> ima_rule_init(). Shallow copy it like other non-LSM references in
> ima_rule_entry structs.
>
> There are no longer any necessary error path cleanups to do in
> ima_lsm_copy_rule() so reference ownership from entry to nentry becomes
> easier.
>
> Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> ---
> security/integrity/ima/ima_policy.c | 13 ++-----------
> 1 file changed, 2 insertions(+), 11 deletions(-)
>
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index e33347148aa9..e9c7d318fdd4 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -306,10 +306,8 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
> continue;
>
> nentry->lsm[i].type = entry->lsm[i].type;
> - nentry->lsm[i].args_p = kstrdup(entry->lsm[i].args_p,
> - GFP_KERNEL);
> - if (!nentry->lsm[i].args_p)
> - goto out_err;
> + nentry->lsm[i].args_p = entry->lsm[i].args_p;
> + entry->lsm[i].args_p = NULL;
Nice.
>
> security_filter_rule_init(nentry->lsm[i].type,
> Audit_equal,
> @@ -325,13 +323,6 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
> entry->keyrings = NULL;
> entry->template = NULL;
> return nentry;
> -
> -out_err:
> - nentry->fsname = NULL;
> - nentry->keyrings = NULL;
> - nentry->template = NULL;
> - ima_free_rule(nentry);
> - return NULL;
> }
Definitely moving ima_free_rule() to the subsequent patch makes sense.
Mimi
>
> static int ima_lsm_update_rule(struct ima_rule_entry *entry)
^ permalink raw reply
* Re: [PATCH 03/12] ima: Free the entire rule when deleting a list of rules
From: Mimi Zohar @ 2020-06-25 21:08 UTC (permalink / raw)
To: Tyler Hicks, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
Prakhar Srivastava, linux-kernel, linux-integrity,
linux-security-module
In-Reply-To: <1593119238.27152.395.camel@linux.ibm.com>
On Thu, 2020-06-25 at 17:07 -0400, Mimi Zohar wrote:
> On Mon, 2020-06-22 at 19:32 -0500, Tyler Hicks wrote:
> > Use ima_free_rule() to fix memory leaks of allocated ima_rule_entry
> > members, such as .fsname and .keyrings, when deleting a list of rules.
> >
> > This fixes a memory leak seen when loading by a valid rule that contains
> > an additional piece of allocated memory, such as an fsname, followed by
> > an invalid rule that triggers a policy load failure:
> >
> > # echo -e "dont_measure fsname=securityfs\nbad syntax" > \
> > /sys/kernel/security/ima/policy
> > -bash: echo: write error: Invalid argument
> > # echo scan > /sys/kernel/debug/kmemleak
> > # cat /sys/kernel/debug/kmemleak
> > unreferenced object 0xffff9bab67ca12c0 (size 16):
> > comm "tee", pid 684, jiffies 4295212803 (age 252.344s)
> > hex dump (first 16 bytes):
> > 73 65 63 75 72 69 74 79 66 73 00 6b 6b 6b 6b a5 securityfs.kkkk.
> > backtrace:
> > [<00000000adc80b1b>] kstrdup+0x2e/0x60
> > [<00000000d504cb0d>] ima_parse_add_rule+0x7d4/0x1020
> > [<00000000444825ac>] ima_write_policy+0xab/0x1d0
> > [<000000002b7f0d6c>] vfs_write+0xde/0x1d0
> > [<0000000096feedcf>] ksys_write+0x68/0xe0
> > [<0000000052b544a2>] do_syscall_64+0x56/0xa0
> > [<000000007ead1ba7>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> >
> > Fixes: f1b08bbcbdaf ("ima: define a new policy condition based on the filesystem name")
> > Fixes: 2b60c0ecedf8 ("IMA: Read keyrings= option from the IMA policy")
> > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
>
> Your decision, but you might consider squashing this patch with 3/12.
> Everything all together in one patch.
Oops, that was the comment for 4/12.
Mimi
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox