* Re: [PATCH v8 02/28] LSM: Infrastructure management of the sock security
From: John Johansen @ 2019-09-18 7:19 UTC (permalink / raw)
To: Stephen Smalley, Casey Schaufler, casey.schaufler, jmorris,
linux-security-module, selinux
Cc: keescook, penguin-kernel, paul
In-Reply-To: <5fde58fe-3925-c9d6-39bf-9adb318f7186@tycho.nsa.gov>
On 9/16/19 11:42 AM, Stephen Smalley wrote:
> On 8/29/19 7:29 PM, Casey Schaufler wrote:
>> Move management of the sock->sk_security blob out
>> of the individual security modules and into the security
>> infrastructure. Instead of allocating the blobs from within
>> the modules the modules tell the infrastructure how much
>> space is required, and the space is allocated there.
>>
>> Reviewed-by: Kees Cook <keescook@chromium.org>
>> Reviewed-by: John Johansen <john.johansen@canonical.com>
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>
> One oddity noted below, but it isn't introduced by this patch so you can add my:
>
> Reviewed-by: Stephen Smalley <sds@tycho.nsa.gov>
>
>> ---
>> include/linux/lsm_hooks.h | 1 +
>> security/apparmor/include/net.h | 6 ++-
>> security/apparmor/lsm.c | 38 ++++-----------
>> security/security.c | 36 +++++++++++++-
>> security/selinux/hooks.c | 78 +++++++++++++++----------------
>> security/selinux/include/objsec.h | 5 ++
>> security/selinux/netlabel.c | 23 ++++-----
>> security/smack/smack.h | 5 ++
>> security/smack/smack_lsm.c | 64 ++++++++++++-------------
>> security/smack/smack_netfilter.c | 8 ++--
>> 10 files changed, 144 insertions(+), 120 deletions(-)
>>
>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>> index f9222a04968d..b353482ea348 100644
>> --- a/include/linux/lsm_hooks.h
>> +++ b/include/linux/lsm_hooks.h
>> @@ -2047,6 +2047,7 @@ struct lsm_blob_sizes {
>> int lbs_cred;
>> int lbs_file;
>> int lbs_inode;
>> + int lbs_sock;
>> int lbs_superblock;
>> int lbs_ipc;
>> int lbs_msg_msg;
>> diff --git a/security/apparmor/include/net.h b/security/apparmor/include/net.h
>> index 7334ac966d01..adac04e3b3cc 100644
>> --- a/security/apparmor/include/net.h
>> +++ b/security/apparmor/include/net.h
>> @@ -55,7 +55,11 @@ struct aa_sk_ctx {
>> struct aa_label *peer;
>> };
>> -#define SK_CTX(X) ((X)->sk_security)
>> +static inline struct aa_sk_ctx *aa_sock(const struct sock *sk)
>> +{
>> + return sk->sk_security + apparmor_blob_sizes.lbs_sock;
>> +}
>> +
>> #define SOCK_ctx(X) SOCK_INODE(X)->i_security
>
> This use of i_security looks suspicious, but SOCK_ctx doesn't appear to be used presently. Probably should be removed in a separate patch.
>
yes this leaked is from some in dev patches that leaked into the base socket mediation patch. I can put together a patch to remove it
^ permalink raw reply
* Re: [Patch v6 4/4] KEYS: trusted: Move TPM2 trusted keys code
From: Sumit Garg @ 2019-09-18 6:23 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: dhowells, peterhuewe, keyrings, linux-integrity,
open list:HARDWARE RANDOM NUMBER GENERATOR CORE,
linux-security-module, Herbert Xu, davem, jgg, Arnd Bergmann,
Greg Kroah-Hartman, jejb, Mimi Zohar, James Morris,
Serge E. Hallyn, Jerry Snitselaar, Linux Kernel Mailing List,
Daniel Thompson
In-Reply-To: <20190917181507.GB8472@linux.intel.com>
On Tue, 17 Sep 2019 at 23:45, Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> On Tue, Sep 17, 2019 at 09:14:15PM +0300, Jarkko Sakkinen wrote:
> > On Mon, Sep 16, 2019 at 04:04:24PM +0530, Sumit Garg wrote:
> > > Move TPM2 trusted keys code to trusted keys subsystem. The reason
> > > being it's better to consolidate all the trusted keys code to a single
> > > location so that it can be maintained sanely.
> > >
> > > Suggested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> >
> > This commit has couple of issues that I only noticed when looking into
> > bug reported by Mimi.
> >
> > Right now tpm_send() is the exported function that is used by other
> > subsystems. tpm_transmit_cmd() is an internal function. This commit adds
> > two unrelated code paths to send TPM commands, which is unacceptable.
Makes sense, will update.
> >
> > You should make tpm2 functionality to use tpm_send() instead and remove
> > tpm_seal_trusted() and tpm_unseal_trusted() completely in this commit.
Okay.
>
> The consequence is that the result needs unfortunately re-review. Sorry
> about that, just took this time to notice this glitch.
No worries :). I will send next version of patch-set.
FYI, I will be travelling for Linaro Connect next week so you could
expect some delays in my responses.
-Sumit
>
> /Jarkko
^ permalink raw reply
* [GIT PULL] SELinux patches for v5.4
From: Paul Moore @ 2019-09-17 19:38 UTC (permalink / raw)
To: Linus Torvalds; +Cc: selinux, linux-kernel, linux-security-module
Hi Linus,
Eight SELinux patches for v5.4, the highlights are listed below and
all pass the selinux-testsuite, please merge for v5.4.
- Add LSM hooks, and SELinux access control hooks, for dnotify,
fanotify, and inotify watches. This has been discussed with both the
LSM and fs/notify folks and everybody is good with these new hooks.
- The LSM stacking changes missed a few calls to current_security() in
the SELinux code; we fix those and remove current_security() for good.
- Improve our network object labeling cache so that we always return
the object's label, even when under memory pressure. Previously we
would return an error if we couldn't allocate a new cache entry, now
we always return the label even if we can't create a new cache entry
for it.
- Convert the sidtab atomic_t counter to a normal u32 with
READ/WRITE_ONCE() and memory barrier protection.
- A few patches to policydb.c to clean things up (remove forward
declarations, long lines, bad variable names, etc.).
Thanks,
-Paul
--
The following changes since commit 45385237f65aeee73641f1ef737d7273905a233f:
selinux: fix memory leak in policydb_init() (2019-07-31 16:51:23 -0400)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git
tags/selinux-pr-20190917
for you to fetch changes up to 15322a0d90b6fd62ae8f22e5b87f735c3fdfeff7:
lsm: remove current_security() (2019-09-04 18:53:39 -0400)
----------------------------------------------------------------
selinux/stable-5.4 PR 20190917
----------------------------------------------------------------
Aaron Goidel (1):
fanotify, inotify, dnotify, security: add security hook for fs
notifications
Ondrej Mosnacek (3):
selinux: policydb - fix some checkpatch.pl warnings
selinux: policydb - rename type_val_to_struct_array
selinux: avoid atomic_t usage in sidtab
Paul Moore (3):
selinux: shuffle around policydb.c to get rid of forward declarations
selinux: always return a secid from the network caches if we find one
lsm: remove current_security()
Stephen Smalley (1):
selinux: fix residual uses of current_security() for the SELinux blob
fs/notify/dnotify/dnotify.c | 15 +-
fs/notify/fanotify/fanotify_user.c | 19 +-
fs/notify/inotify/inotify_user.c | 14 +-
include/linux/cred.h | 1 -
include/linux/lsm_hooks.h | 9 +-
include/linux/security.h | 10 +-
security/security.c | 6 +
security/selinux/hooks.c | 49 ++++-
security/selinux/include/classmap.h | 5 +-
security/selinux/include/objsec.h | 20 +-
security/selinux/netif.c | 31 ++-
security/selinux/netnode.c | 30 ++-
security/selinux/netport.c | 24 +--
security/selinux/ss/policydb.c | 402 +++++++++++++++---------------
security/selinux/ss/policydb.h | 2 +-
security/selinux/ss/services.c | 6 +-
security/selinux/ss/sidtab.c | 48 ++---
security/selinux/ss/sidtab.h | 19 +-
18 files changed, 403 insertions(+), 307 deletions(-)
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [RFC v1 12/14] krsi: Add an eBPF helper function to get the value of an env variable
From: KP Singh @ 2019-09-17 19:36 UTC (permalink / raw)
To: Yonghong Song
Cc: linux-kernel@vger.kernel.org, bpf@vger.kernel.org,
linux-security-module@vger.kernel.org, Alexei Starovoitov,
Daniel Borkmann, James Morris, Kees Cook, Thomas Garnier,
Michael Halcrow, Paul Turner, Brendan Gregg, Jann Horn,
Matthew Garrett, Christian Brauner, Mickaël Salaün,
Florent Revest, Martin Lau, Song Liu, Serge E. Hallyn,
Mauro Carvalho Chehab, David S. Miller, Greg Kroah-Hartman,
Nicolas Ferre, Stanislav Fomichev, Quentin Monnet, Andrey Ignatov,
Joe Stringer
In-Reply-To: <66d5a0b2-8ea1-cdb1-87b6-71021d875296@fb.com>
On 17-Sep 16:58, Yonghong Song wrote:
>
>
> On 9/16/19 6:00 AM, KP Singh wrote:
> > Thanks for reviewing!
> >
> > On 15-Sep 00:16, Yonghong Song wrote:
> >>
> >>
> >> On 9/10/19 12:55 PM, KP Singh wrote:
> >>> From: KP Singh <kpsingh@google.com>
> >>
> >> This patch cannot apply cleanly.
> >>
> >> -bash-4.4$ git apply ~/p12.txt
> >> error: patch failed: include/uapi/linux/bpf.h:2715
> >> error: include/uapi/linux/bpf.h: patch does not apply
> >> error: patch failed: tools/include/uapi/linux/bpf.h:2715
> >> error: tools/include/uapi/linux/bpf.h: patch does not apply
> >> -bash-4.4$
> >
> > I am not sure why this is happening, I tried:
> >
> > git clone \
> > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git && \
> > cd linux-next && \
> > git checkout -b review v5.3-rc6 && \
> > wget -P /tmp https://lore.kernel.org/patchwork/series/410101/mbox && \
> > git am /tmp/mbox
> >
> > and it worked.
> >
> > This seems to work too:
> >
> > patch -p1 < <file>.patch
> >
> > Can you try with "git am" please?
>
> Will try next time when reviewing the tree.
Thanks!
>
> >
> >>
> >>>
> >>> The helper returns the value of the environment variable in the buffer
> >>> that is passed to it. If the var is set multiple times, the helper
> >>> returns all the values as null separated strings.
> >>>
> >>> If the buffer is too short for these values, the helper tries to fill it
> >>> the best it can and guarantees that the value returned in the buffer
> >>> is always null terminated. After the buffer is filled, the helper keeps
> >>> counting the number of times the environment variable is set in the
> >>> envp.
> >>>
> >>> The return value of the helper is an u64 value which carries two pieces
> >>> of information.
> >>>
> >>> * The upper 32 bits are a u32 value signifying the number of times
> >>> the environment variable is set in the envp.
> >>
> >> Not sure how useful this 'upper 32' bit value is. What user expected to do?
> >>
> >> Another option is to have upper 32 bits encode the required buffer size
> >> to hold all values. This may cause some kind of user space action, e.g.,
> >> to replace the program with new program with larger per cpu map value size?
> >>
> >
> > The upper 32-bit value is actually an important part of the LSM's MAC
> > policy. It allows the user to:
> >
> > - Return an -EPERM when if the environment variable is set more than
> > once.
> > - Log a warning (this is what we are doing in the example) so
> > this is flagged as a potential malicious actor.
>
> So the intention is to catch cases where the env variable
> to set by more than once, not exactly to capture all the values
> of the env variable.
>
> Then may be there is no need to record the values once the number of
> values is more than one? Do you have use case for what user to react if
> there are more than one setting?
Since KRSI intends to be an API for creating MAC and Audit Policies,
it would be best to leave the decision to the user, as to what they
intend to do with the extra information. In general, having both the
values helps in building more context about the potential malicious actor.
What should ideally be done is returning an -EPERM error, because there
should be no reason to set an env variable twice.
In fact, two+ env vars can only be passed directly to the
execve system call arguments. Shells (e.g. bash) remove the duplicate
value before passing the envv to the system call.
>
> >
> >>> * The lower 32 bits are a s32 value signifying the number of bytes
> >>> written to the buffer or an error code. >
> >>> Since the value of the environment variable can be very long and exceed
> >>> what can be allocated on the BPF stack, a per-cpu array can be used
> >>> instead:
> >>>
> >>> struct bpf_map_def SEC("maps") env_map = {
> >>> .type = BPF_MAP_TYPE_PERCPU_ARRAY,
> >>> .key_size = sizeof(u32),
> >>> .value_size = 4096,
> >>> .max_entries = 1,
> >>> };
> >>
> >> Could you use use map definition with SEC(".maps")?
> >
> > Sure, I added this example program in the commit message. Will update
> > it to be more canonical. Thanks!
> >
> >>
> >>>
> >>> SEC("prgrm")
> >>> int bpf_prog1(void *ctx)
> >>> {
> >>> u32 map_id = 0;
> >>> u64 times_ret;
> >>> s32 ret;
> >>> char name[48] = "LD_PRELOAD";
> >>
> >> Reverse Christmas tree coding style, here and other places?
> >
> > Will happily fix it.
> >
> > However, I did not find it mentioned in the style guide:
> >
> > https://www.kernel.org/doc/html/v4.10/process/coding-style.html
> > https://urldefense.proofpoint.com/v2/url?u=https-3A__elixir.bootlin.com_linux_v4.6_source_Documentation_CodingStyle&d=DwIBAg&c=5VD0RTtNlTh3ycd41b3MUw&r=DA8e1B5r073vIqRrFz7MRA&m=G_3dpLivr0lLqQlQqAVEw9EZB3GonzJjILyCBqbmMIo&s=krLmTogyg9eSdco0j4tv2etr4PmNEPZGXMMoOkWdVG4&e=
> >
> > Is there one specific to BPF?
>
> I forgot where is the documentation. We follow this for bpf/net.
Thanks, I searched and found some threads where this is recommended
for bpf/net. I will update it in the next iteration.
>
> >
> >
> >>
> >>>
> >>> char *map_value = bpf_map_lookup_elem(&env_map, &map_id);
> >>> if (!map_value)
> >>> return 0;
> >>>
> >>> // Read the lower 32 bits for the return value
> >>> times_ret = krsi_get_env_var(ctx, name, 48, map_value, 4096);
> >>> ret = times_ret & 0xffffffff;
> >>> if (ret < 0)
> >>> return ret;
> >>> return 0;
> >>> }
> >>>
> >>> Signed-off-by: KP Singh <kpsingh@google.com>
> >>> ---
> >>> include/uapi/linux/bpf.h | 42 ++++++-
> >>> security/krsi/ops.c | 129 ++++++++++++++++++++++
> >>> tools/include/uapi/linux/bpf.h | 42 ++++++-
> >>> tools/testing/selftests/bpf/bpf_helpers.h | 3 +
> >>> 4 files changed, 214 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> >>> index 32ab38f1a2fe..a4ef07956e07 100644
> >>> --- a/include/uapi/linux/bpf.h
> >>> +++ b/include/uapi/linux/bpf.h
> >>> @@ -2715,6 +2715,45 @@ union bpf_attr {
> >>> * **-EPERM** if no permission to send the *sig*.
> >>> *
> >>> * **-EAGAIN** if bpf program can try again.
> >>> + *
> >>> + * u64 krsi_get_env_var(void *ctx, char *name, char *buf,
> >>> + * size_t name_len, size_t buf_len)
> >>
> >> This signature is not the same as the later
> >> krsi_get_env_var(...) helper definition.
> >> BPF_CALL_5(krsi_get_env_var, struct krsi_ctx *, ctx, char *, name, u32,
> >> n_size,
> >> char *, dest, u32, size)
> >>
> >
> > I did this because the krsi_ctx is not exposed to the userspace and
> > allows KRSI to modify the context without worrying about breaking
> > userspace.
> >
> > That said, I could mark it as a (void *) here and cast it internally.
> > I guess that would be better/cleaner?
>
> "void *" is okay, I am complaining the argument ordering is different
> from real definition.
Ah, nice catch! Apologies for overlooking.
>
> >
> >>> + * Description
> >>> + * This helper can be used as a part of the
> >>> + * process_execution hook of the KRSI LSM in
> >>> + * programs of type BPF_PROG_TYPE_KRSI.
> >>> + *
> >>> + * The helper returns the value of the environment
> >>> + * variable with the provided "name" for process that's
> >>> + * going to be executed in the passed buffer, "buf". If the var
> >>> + * is set multiple times, the helper returns all
> >>> + * the values as null separated strings.
> >>> + *
> >>> + * If the buffer is too short for these values, the helper
> >>> + * tries to fill it the best it can and guarantees that the value
> >>> + * returned in the buffer is always null terminated.
> >>> + * After the buffer is filled, the helper keeps counting the number
> >>> + * of times the environment variable is set in the envp.
> >>> + *
> >>> + * Return:
> >>> + *
> >>> + * The return value of the helper is an u64 value
> >>> + * which carries two pieces of information:
> >>> + *
> >>> + * The upper 32 bits are a u32 value signifying
> >>> + * the number of times the environment variable
> >>> + * is set in the envp.
> >>> + * The lower 32 bits are an s32 value signifying
> >>> + * the number of bytes written to the buffer or an error code:
> >>> + *
> >>> + * **-ENOMEM** if the kernel is unable to allocate memory
> >>> + * for pinning the argv and envv.
> >>> + *
> >>> + * **-E2BIG** if the value is larger than the size of the
> >>> + * destination buffer. The higher bits will still
> >>> + * the number of times the variable was set in the envp.
> >>
> >> The -E2BIG is returned because buffer sizee is not big enough.
> >> Another possible error code is -ENOSPC, which typically indicates
> >> buffer size not big enough.
> >
> > Sure, I am fine with using either.
> >
> >>
> >>> + *
> >>> + * **-EINVAL** if name is not a NULL terminated string.
> >>> */
> >>> #define __BPF_FUNC_MAPPER(FN) \
> >>> FN(unspec), \
> >>> @@ -2826,7 +2865,8 @@ union bpf_attr {
> >>> FN(strtoul), \
> >>> FN(sk_storage_get), \
> >>> FN(sk_storage_delete), \
> >>> - FN(send_signal),
> >>> + FN(send_signal), \
> >>> + FN(krsi_get_env_var),
> >>>
> >>> /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> >>> * function eBPF program intends to call
> >>> diff --git a/security/krsi/ops.c b/security/krsi/ops.c
> >>> index 1f4df920139c..1db94dfaac15 100644
> >>> --- a/security/krsi/ops.c
> >>> +++ b/security/krsi/ops.c
> >>> @@ -6,6 +6,8 @@
> >>> #include <linux/bpf.h>
> >>> #include <linux/security.h>
> >>> #include <linux/krsi.h>
> >>> +#include <linux/binfmts.h>
> >>> +#include <linux/highmem.h>
> >>>
> >>> #include "krsi_init.h"
> >>> #include "krsi_fs.h"
> >>> @@ -162,6 +164,131 @@ static bool krsi_prog_is_valid_access(int off, int size,
> >>> return false;
> >>> }
> >>>
> >>> +static char *array_next_entry(char *array, unsigned long *offset,
> >>> + unsigned long end)
> >>> +{
> >>> + char *entry;
> >>> + unsigned long current_offset = *offset;
> >>> +
> >>> + if (current_offset >= end)
> >>> + return NULL;
> >>> +
> >>> + /*
> >>> + * iterate on the array till the null byte is encountered
> >>> + * and check for any overflows.
> >>> + */
> >>> + entry = array + current_offset;
> >>> + while (array[current_offset]) {
> >>> + if (unlikely(++current_offset >= end))
> >>> + return NULL;
> >>> + }
> >>> +
> >>> + /*
> >>> + * Point the offset to the next element in the array.
> >>> + */
> >>> + *offset = current_offset + 1;
> >>> +
> >>> + return entry;
> >>> +}
> >>> +
> >>> +static u64 get_env_var(struct krsi_ctx *ctx, char *name, char *dest,
> >>> + u32 n_size, u32 size)
> >>> +{
> >>> + s32 ret = 0;
> >>> + u32 num_vars = 0;
> >>> + int i, name_len;
> >>> + struct linux_binprm *bprm = ctx->bprm_ctx.bprm;
> >>> + int argc = bprm->argc;
> >>> + int envc = bprm->envc;
> >>> + unsigned long end = ctx->bprm_ctx.max_arg_offset;
> >>> + unsigned long offset = bprm->p % PAGE_SIZE;
> >>
> >> why we need bprm->p % PAGE_SIZE instead of bprm->p?
> >
> > bprm->p points to the top of the memory and it's not an offset.
> >
> > The pinned buffer contains the pages for the (argv+env) and the
> > brpm->p % PAGE_SIZE is the offset into the first page where the
> > (argv+envv) starts.
>
> Thanks for the explanation.
>
> >
> >>
> >>> + char *buf = ctx->bprm_ctx.arg_pages;
> >>> + char *curr_dest = dest;
> >>> + char *entry;
> >>> +
> >>> + if (unlikely(!buf))
> >>> + return -ENOMEM;
> >>> +
> >>> + for (i = 0; i < argc; i++) {
> >>> + entry = array_next_entry(buf, &offset, end);
> >>> + if (!entry)
> >>> + return 0;
> >>> + }
> >>> +
> >>> + name_len = strlen(name);
> >>> + for (i = 0; i < envc; i++) {
> >>> + entry = array_next_entry(buf, &offset, end);
> >>> + if (!entry)
> >>> + return 0;
> >>
> >> If the buf is "LD_PRELOAD=a.so\0LD_PRELOAD=b.so" and argc=0,
> >> we may skip the first entry?
> >
> > I think I need to rename the "array_next_entry" function / document it
> > better.
> >
> > The function updates the offset to the next location and the
> > returns the entry at the current offset.
> >
> > So, in the first instance:
> >
> > // offset is the offset into the first page.
> > entry = buf + offset;
> > offset = <updated value to the next entry>.
>
> Yes, some documentation will help.
>
> >
> >>
> >>
> >>> +
> >>> + if (!strncmp(entry, name, name_len)) {
> >>> + num_vars++;
> >>
> >> There helper permits n_size = 0 (ARG_CONST_SIZE_OR_ZERO).
> >> in this case, name_len = 0, strncmp(entry, name, name_len) will be always 0.
> >
> > Thanks, you are right, It does not make sense to have name_len = 0. I
> > will change it to ARG_CONST_SIZE.
> >
> >>
> >>> +
> >>> + /*
> >>> + * There is no need to do further copying
> >>> + * if the buffer is already full. Just count how many
> >>> + * times the environment variable is set.
> >>> + */
> >>> + if (ret == -E2BIG)
> >>> + continue;
> >>> +
> >>> + if (entry[name_len] != '=')
> >>> + continue;
> >>> +
> >>> + /*
> >>> + * Move the buf pointer by name_len + 1
> >>> + * (for the "=" sign)
> >>> + */
> >>> + entry += name_len + 1;
> >>> + ret = strlcpy(curr_dest, entry, size);
> >>> +
> >>> + if (ret >= size) {
> >>> + ret = -E2BIG;
> >>
> >> Here, we have a partial copy. Should you instead nullify (memset) it as
> >> it is not really invalid one?
> >
> > The function does specify that the it will return a null terminated
> > value even if an -E2BIG is returned so that user does get a truncated
> > value. It's better to give the user some data. (I mentioned this in
> > the documentation for the helper).
>
> Do you know what user could do with this partial data?
> Again, I am not a security expert, maybe partial data can still
> be used in meaningful.
It's best to give the userspace as much data as possible and indicate an
overflow so that user-space can use this information to create more
context around the possible malicious activity.
- KP
>
> >
> >>
> >>> + continue;
> >>> + }
> >>> +
> >>> + /*
> >>> + * strlcpy just returns the length of the string copied.
> >>> + * The remaining space needs to account for the added
> >>> + * null character.
> >>> + */
> >>> + curr_dest += ret + 1;
> >>> + size -= ret + 1;
> >>> + /*
> >>> + * Update ret to be the current number of bytes written
> >>> + * to the destination
> >>> + */
> >>> + ret = curr_dest - dest;
> >>> + }
> >>> + }
> >>> +
> >>> + return (u64) num_vars << 32 | (u32) ret;
> >>> +}
> >>> +
> >>> +BPF_CALL_5(krsi_get_env_var, struct krsi_ctx *, ctx, char *, name, u32, n_size,
> >>> + char *, dest, u32, size)
> >>> +{
> >>> + char *name_end;
> >>> +
> >>> + name_end = memchr(name, '\0', n_size);
> >>> + if (!name_end)
> >>> + return -EINVAL;
> >>> +
> >>> + memset(dest, 0, size);
> >
> > This memset ensures the buffer is zeroed out (incase the buffer is
> > fully / partially empty).
> >
> >>> + return get_env_var(ctx, name, dest, n_size, size);
> >>> +}
> >>> +
> >>> +static const struct bpf_func_proto krsi_get_env_var_proto = {
> >>> + .func = krsi_get_env_var,
> >>> + .gpl_only = true,
> >>> + .ret_type = RET_INTEGER,
> >>> + .arg1_type = ARG_PTR_TO_CTX,
> >>> + .arg2_type = ARG_PTR_TO_MEM,
> >>> + .arg3_type = ARG_CONST_SIZE_OR_ZERO,
> >>> + .arg4_type = ARG_PTR_TO_UNINIT_MEM,
> >>> + .arg5_type = ARG_CONST_SIZE_OR_ZERO,
> >>> +};
> >>> +
> >>> BPF_CALL_5(krsi_event_output, void *, log,
> >>> struct bpf_map *, map, u64, flags, void *, data, u64, size)
> >>> {
> >>> @@ -192,6 +319,8 @@ static const struct bpf_func_proto *krsi_prog_func_proto(enum bpf_func_id
> >>> return &bpf_map_lookup_elem_proto;
> >>> case BPF_FUNC_get_current_pid_tgid:
> >>> return &bpf_get_current_pid_tgid_proto;
> >>> + case BPF_FUNC_krsi_get_env_var:
> >>> + return &krsi_get_env_var_proto;
> >>> case BPF_FUNC_perf_event_output:
> >>> return &krsi_event_output_proto;
> >>> default:
> >>> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> >>> index 32ab38f1a2fe..a4ef07956e07 100644
> >>> --- a/tools/include/uapi/linux/bpf.h
> >>> +++ b/tools/include/uapi/linux/bpf.h
> >>> @@ -2715,6 +2715,45 @@ union bpf_attr {
> >>> * **-EPERM** if no permission to send the *sig*.
> >>> *
> >>> * **-EAGAIN** if bpf program can try again.
> >>> + *
> >>> + * u64 krsi_get_env_var(void *ctx, char *name, char *buf,
> >>> + * size_t name_len, size_t buf_len)
> >>
> >> Inconsistent helper definitions.
> >
> > As discussed above, I can change the BPF_CALL_5 declaration to have
> > a (void *) and cast to the krsi_ctx in the helper itself.
> >
> > - KP
> >
> >>
> >>> + * Description
> >>> + * This helper can be used as a part of the
> >>> + * process_execution hook of the KRSI LSM in
> >>> + * programs of type BPF_PROG_TYPE_KRSI.
> >>> + *
> >>> + * The helper returns the value of the environment
> >>> + * variable with the provided "name" for process that's
> >>> + * going to be executed in the passed buffer, "buf". If the var
> >>> + * is set multiple times, the helper returns all
> >>> + * the values as null separated strings.
> >>> + *
> >>> + * If the buffer is too short for these values, the helper
> >>> + * tries to fill it the best it can and guarantees that the value
> >>> + * returned in the buffer is always null terminated.
> >>> + * After the buffer is filled, the helper keeps counting the number
> >>> + * of times the environment variable is set in the envp.
> >>> + *
> >>> + * Return:
> >>> + *
> >>> + * The return value of the helper is an u64 value
> >>> + * which carries two pieces of information:
> >>> + *
> >>> + * The upper 32 bits are a u32 value signifying
> >>> + * the number of times the environment variable
> >>> + * is set in the envp.
> >>> + * The lower 32 bits are an s32 value signifying
> >>> + * the number of bytes written to the buffer or an error code:
> >>> + *
> >>> + * **-ENOMEM** if the kernel is unable to allocate memory
> >>> + * for pinning the argv and envv.
> >>> + *
> >>> + * **-E2BIG** if the value is larger than the size of the
> >>> + * destination buffer. The higher bits will still
> >>> + * the number of times the variable was set in the envp.
> >>> + *
> >>> + * **-EINVAL** if name is not a NULL terminated string.
> >>> */
> >>> #define __BPF_FUNC_MAPPER(FN) \
> >>> FN(unspec), \
> >>> @@ -2826,7 +2865,8 @@ union bpf_attr {
> >>> FN(strtoul), \
> >>> FN(sk_storage_get), \
> >>> FN(sk_storage_delete), \
> >>> - FN(send_signal),
> >>> + FN(send_signal), \
> >>> + FN(krsi_get_env_var),
> >>>
> >>> /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> >>> * function eBPF program intends to call
> >>> diff --git a/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h
> >>> index f804f210244e..ecebdb772a9d 100644
> >>> --- a/tools/testing/selftests/bpf/bpf_helpers.h
> >>> +++ b/tools/testing/selftests/bpf/bpf_helpers.h
> >>> @@ -303,6 +303,9 @@ static int (*bpf_get_numa_node_id)(void) =
> >>> static int (*bpf_probe_read_str)(void *ctx, __u32 size,
> >>> const void *unsafe_ptr) =
> >>> (void *) BPF_FUNC_probe_read_str;
> >>> +static unsigned long long (*krsi_get_env_var)(void *ctx,
> >>> + void *name, __u32 n_size, void *buf, __u32 size) =
> >>> + (void *) BPF_FUNC_krsi_get_env_var;
> >>> static unsigned int (*bpf_get_socket_uid)(void *ctx) =
> >>> (void *) BPF_FUNC_get_socket_uid;
> >>> static unsigned int (*bpf_set_hash)(void *ctx, __u32 hash) =
> >>>
^ permalink raw reply
* Re: [Patch v6 4/4] KEYS: trusted: Move TPM2 trusted keys code
From: Jarkko Sakkinen @ 2019-09-17 18:15 UTC (permalink / raw)
To: Sumit Garg
Cc: dhowells, peterhuewe, keyrings, linux-integrity, linux-crypto,
linux-security-module, herbert, davem, jgg, arnd, gregkh, jejb,
zohar, jmorris, serge, jsnitsel, linux-kernel, daniel.thompson
In-Reply-To: <20190917181415.GA8472@linux.intel.com>
On Tue, Sep 17, 2019 at 09:14:15PM +0300, Jarkko Sakkinen wrote:
> On Mon, Sep 16, 2019 at 04:04:24PM +0530, Sumit Garg wrote:
> > Move TPM2 trusted keys code to trusted keys subsystem. The reason
> > being it's better to consolidate all the trusted keys code to a single
> > location so that it can be maintained sanely.
> >
> > Suggested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>
> This commit has couple of issues that I only noticed when looking into
> bug reported by Mimi.
>
> Right now tpm_send() is the exported function that is used by other
> subsystems. tpm_transmit_cmd() is an internal function. This commit adds
> two unrelated code paths to send TPM commands, which is unacceptable.
>
> You should make tpm2 functionality to use tpm_send() instead and remove
> tpm_seal_trusted() and tpm_unseal_trusted() completely in this commit.
The consequence is that the result needs unfortunately re-review. Sorry
about that, just took this time to notice this glitch.
/Jarkko
^ permalink raw reply
* Re: [Patch v6 4/4] KEYS: trusted: Move TPM2 trusted keys code
From: Jarkko Sakkinen @ 2019-09-17 18:14 UTC (permalink / raw)
To: Sumit Garg
Cc: dhowells, peterhuewe, keyrings, linux-integrity, linux-crypto,
linux-security-module, herbert, davem, jgg, arnd, gregkh, jejb,
zohar, jmorris, serge, jsnitsel, linux-kernel, daniel.thompson
In-Reply-To: <1568630064-14887-5-git-send-email-sumit.garg@linaro.org>
On Mon, Sep 16, 2019 at 04:04:24PM +0530, Sumit Garg wrote:
> Move TPM2 trusted keys code to trusted keys subsystem. The reason
> being it's better to consolidate all the trusted keys code to a single
> location so that it can be maintained sanely.
>
> Suggested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
This commit has couple of issues that I only noticed when looking into
bug reported by Mimi.
Right now tpm_send() is the exported function that is used by other
subsystems. tpm_transmit_cmd() is an internal function. This commit adds
two unrelated code paths to send TPM commands, which is unacceptable.
You should make tpm2 functionality to use tpm_send() instead and remove
tpm_seal_trusted() and tpm_unseal_trusted() completely in this commit.
/Jarkko
^ permalink raw reply
* Re: [RFC v1 12/14] krsi: Add an eBPF helper function to get the value of an env variable
From: Yonghong Song @ 2019-09-17 16:58 UTC (permalink / raw)
To: KP Singh
Cc: linux-kernel@vger.kernel.org, bpf@vger.kernel.org,
linux-security-module@vger.kernel.org, Alexei Starovoitov,
Daniel Borkmann, James Morris, Kees Cook, Thomas Garnier,
Michael Halcrow, Paul Turner, Brendan Gregg, Jann Horn,
Matthew Garrett, Christian Brauner, Mickaël Salaün,
Florent Revest, Martin Lau, Song Liu, Serge E. Hallyn,
Mauro Carvalho Chehab, David S. Miller, Greg Kroah-Hartman,
Nicolas Ferre, Stanislav Fomichev, Quentin Monnet, Andrey Ignatov,
Joe Stringer
In-Reply-To: <20190916130043.GA64010@google.com>
On 9/16/19 6:00 AM, KP Singh wrote:
> Thanks for reviewing!
>
> On 15-Sep 00:16, Yonghong Song wrote:
>>
>>
>> On 9/10/19 12:55 PM, KP Singh wrote:
>>> From: KP Singh <kpsingh@google.com>
>>
>> This patch cannot apply cleanly.
>>
>> -bash-4.4$ git apply ~/p12.txt
>> error: patch failed: include/uapi/linux/bpf.h:2715
>> error: include/uapi/linux/bpf.h: patch does not apply
>> error: patch failed: tools/include/uapi/linux/bpf.h:2715
>> error: tools/include/uapi/linux/bpf.h: patch does not apply
>> -bash-4.4$
>
> I am not sure why this is happening, I tried:
>
> git clone \
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git && \
> cd linux-next && \
> git checkout -b review v5.3-rc6 && \
> wget -P /tmp https://lore.kernel.org/patchwork/series/410101/mbox && \
> git am /tmp/mbox
>
> and it worked.
>
> This seems to work too:
>
> patch -p1 < <file>.patch
>
> Can you try with "git am" please?
Will try next time when reviewing the tree.
>
>>
>>>
>>> The helper returns the value of the environment variable in the buffer
>>> that is passed to it. If the var is set multiple times, the helper
>>> returns all the values as null separated strings.
>>>
>>> If the buffer is too short for these values, the helper tries to fill it
>>> the best it can and guarantees that the value returned in the buffer
>>> is always null terminated. After the buffer is filled, the helper keeps
>>> counting the number of times the environment variable is set in the
>>> envp.
>>>
>>> The return value of the helper is an u64 value which carries two pieces
>>> of information.
>>>
>>> * The upper 32 bits are a u32 value signifying the number of times
>>> the environment variable is set in the envp.
>>
>> Not sure how useful this 'upper 32' bit value is. What user expected to do?
>>
>> Another option is to have upper 32 bits encode the required buffer size
>> to hold all values. This may cause some kind of user space action, e.g.,
>> to replace the program with new program with larger per cpu map value size?
>>
>
> The upper 32-bit value is actually an important part of the LSM's MAC
> policy. It allows the user to:
>
> - Return an -EPERM when if the environment variable is set more than
> once.
> - Log a warning (this is what we are doing in the example) so
> this is flagged as a potential malicious actor.
So the intention is to catch cases where the env variable
to set by more than once, not exactly to capture all the values
of the env variable.
Then may be there is no need to record the values once the number of
values is more than one? Do you have use case for what user to react if
there are more than one setting?
>
>>> * The lower 32 bits are a s32 value signifying the number of bytes
>>> written to the buffer or an error code. >
>>> Since the value of the environment variable can be very long and exceed
>>> what can be allocated on the BPF stack, a per-cpu array can be used
>>> instead:
>>>
>>> struct bpf_map_def SEC("maps") env_map = {
>>> .type = BPF_MAP_TYPE_PERCPU_ARRAY,
>>> .key_size = sizeof(u32),
>>> .value_size = 4096,
>>> .max_entries = 1,
>>> };
>>
>> Could you use use map definition with SEC(".maps")?
>
> Sure, I added this example program in the commit message. Will update
> it to be more canonical. Thanks!
>
>>
>>>
>>> SEC("prgrm")
>>> int bpf_prog1(void *ctx)
>>> {
>>> u32 map_id = 0;
>>> u64 times_ret;
>>> s32 ret;
>>> char name[48] = "LD_PRELOAD";
>>
>> Reverse Christmas tree coding style, here and other places?
>
> Will happily fix it.
>
> However, I did not find it mentioned in the style guide:
>
> https://www.kernel.org/doc/html/v4.10/process/coding-style.html
> https://urldefense.proofpoint.com/v2/url?u=https-3A__elixir.bootlin.com_linux_v4.6_source_Documentation_CodingStyle&d=DwIBAg&c=5VD0RTtNlTh3ycd41b3MUw&r=DA8e1B5r073vIqRrFz7MRA&m=G_3dpLivr0lLqQlQqAVEw9EZB3GonzJjILyCBqbmMIo&s=krLmTogyg9eSdco0j4tv2etr4PmNEPZGXMMoOkWdVG4&e=
>
> Is there one specific to BPF?
I forgot where is the documentation. We follow this for bpf/net.
>
>
>>
>>>
>>> char *map_value = bpf_map_lookup_elem(&env_map, &map_id);
>>> if (!map_value)
>>> return 0;
>>>
>>> // Read the lower 32 bits for the return value
>>> times_ret = krsi_get_env_var(ctx, name, 48, map_value, 4096);
>>> ret = times_ret & 0xffffffff;
>>> if (ret < 0)
>>> return ret;
>>> return 0;
>>> }
>>>
>>> Signed-off-by: KP Singh <kpsingh@google.com>
>>> ---
>>> include/uapi/linux/bpf.h | 42 ++++++-
>>> security/krsi/ops.c | 129 ++++++++++++++++++++++
>>> tools/include/uapi/linux/bpf.h | 42 ++++++-
>>> tools/testing/selftests/bpf/bpf_helpers.h | 3 +
>>> 4 files changed, 214 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>>> index 32ab38f1a2fe..a4ef07956e07 100644
>>> --- a/include/uapi/linux/bpf.h
>>> +++ b/include/uapi/linux/bpf.h
>>> @@ -2715,6 +2715,45 @@ union bpf_attr {
>>> * **-EPERM** if no permission to send the *sig*.
>>> *
>>> * **-EAGAIN** if bpf program can try again.
>>> + *
>>> + * u64 krsi_get_env_var(void *ctx, char *name, char *buf,
>>> + * size_t name_len, size_t buf_len)
>>
>> This signature is not the same as the later
>> krsi_get_env_var(...) helper definition.
>> BPF_CALL_5(krsi_get_env_var, struct krsi_ctx *, ctx, char *, name, u32,
>> n_size,
>> char *, dest, u32, size)
>>
>
> I did this because the krsi_ctx is not exposed to the userspace and
> allows KRSI to modify the context without worrying about breaking
> userspace.
>
> That said, I could mark it as a (void *) here and cast it internally.
> I guess that would be better/cleaner?
"void *" is okay, I am complaining the argument ordering is different
from real definition.
>
>>> + * Description
>>> + * This helper can be used as a part of the
>>> + * process_execution hook of the KRSI LSM in
>>> + * programs of type BPF_PROG_TYPE_KRSI.
>>> + *
>>> + * The helper returns the value of the environment
>>> + * variable with the provided "name" for process that's
>>> + * going to be executed in the passed buffer, "buf". If the var
>>> + * is set multiple times, the helper returns all
>>> + * the values as null separated strings.
>>> + *
>>> + * If the buffer is too short for these values, the helper
>>> + * tries to fill it the best it can and guarantees that the value
>>> + * returned in the buffer is always null terminated.
>>> + * After the buffer is filled, the helper keeps counting the number
>>> + * of times the environment variable is set in the envp.
>>> + *
>>> + * Return:
>>> + *
>>> + * The return value of the helper is an u64 value
>>> + * which carries two pieces of information:
>>> + *
>>> + * The upper 32 bits are a u32 value signifying
>>> + * the number of times the environment variable
>>> + * is set in the envp.
>>> + * The lower 32 bits are an s32 value signifying
>>> + * the number of bytes written to the buffer or an error code:
>>> + *
>>> + * **-ENOMEM** if the kernel is unable to allocate memory
>>> + * for pinning the argv and envv.
>>> + *
>>> + * **-E2BIG** if the value is larger than the size of the
>>> + * destination buffer. The higher bits will still
>>> + * the number of times the variable was set in the envp.
>>
>> The -E2BIG is returned because buffer sizee is not big enough.
>> Another possible error code is -ENOSPC, which typically indicates
>> buffer size not big enough.
>
> Sure, I am fine with using either.
>
>>
>>> + *
>>> + * **-EINVAL** if name is not a NULL terminated string.
>>> */
>>> #define __BPF_FUNC_MAPPER(FN) \
>>> FN(unspec), \
>>> @@ -2826,7 +2865,8 @@ union bpf_attr {
>>> FN(strtoul), \
>>> FN(sk_storage_get), \
>>> FN(sk_storage_delete), \
>>> - FN(send_signal),
>>> + FN(send_signal), \
>>> + FN(krsi_get_env_var),
>>>
>>> /* integer value in 'imm' field of BPF_CALL instruction selects which helper
>>> * function eBPF program intends to call
>>> diff --git a/security/krsi/ops.c b/security/krsi/ops.c
>>> index 1f4df920139c..1db94dfaac15 100644
>>> --- a/security/krsi/ops.c
>>> +++ b/security/krsi/ops.c
>>> @@ -6,6 +6,8 @@
>>> #include <linux/bpf.h>
>>> #include <linux/security.h>
>>> #include <linux/krsi.h>
>>> +#include <linux/binfmts.h>
>>> +#include <linux/highmem.h>
>>>
>>> #include "krsi_init.h"
>>> #include "krsi_fs.h"
>>> @@ -162,6 +164,131 @@ static bool krsi_prog_is_valid_access(int off, int size,
>>> return false;
>>> }
>>>
>>> +static char *array_next_entry(char *array, unsigned long *offset,
>>> + unsigned long end)
>>> +{
>>> + char *entry;
>>> + unsigned long current_offset = *offset;
>>> +
>>> + if (current_offset >= end)
>>> + return NULL;
>>> +
>>> + /*
>>> + * iterate on the array till the null byte is encountered
>>> + * and check for any overflows.
>>> + */
>>> + entry = array + current_offset;
>>> + while (array[current_offset]) {
>>> + if (unlikely(++current_offset >= end))
>>> + return NULL;
>>> + }
>>> +
>>> + /*
>>> + * Point the offset to the next element in the array.
>>> + */
>>> + *offset = current_offset + 1;
>>> +
>>> + return entry;
>>> +}
>>> +
>>> +static u64 get_env_var(struct krsi_ctx *ctx, char *name, char *dest,
>>> + u32 n_size, u32 size)
>>> +{
>>> + s32 ret = 0;
>>> + u32 num_vars = 0;
>>> + int i, name_len;
>>> + struct linux_binprm *bprm = ctx->bprm_ctx.bprm;
>>> + int argc = bprm->argc;
>>> + int envc = bprm->envc;
>>> + unsigned long end = ctx->bprm_ctx.max_arg_offset;
>>> + unsigned long offset = bprm->p % PAGE_SIZE;
>>
>> why we need bprm->p % PAGE_SIZE instead of bprm->p?
>
> bprm->p points to the top of the memory and it's not an offset.
>
> The pinned buffer contains the pages for the (argv+env) and the
> brpm->p % PAGE_SIZE is the offset into the first page where the
> (argv+envv) starts.
Thanks for the explanation.
>
>>
>>> + char *buf = ctx->bprm_ctx.arg_pages;
>>> + char *curr_dest = dest;
>>> + char *entry;
>>> +
>>> + if (unlikely(!buf))
>>> + return -ENOMEM;
>>> +
>>> + for (i = 0; i < argc; i++) {
>>> + entry = array_next_entry(buf, &offset, end);
>>> + if (!entry)
>>> + return 0;
>>> + }
>>> +
>>> + name_len = strlen(name);
>>> + for (i = 0; i < envc; i++) {
>>> + entry = array_next_entry(buf, &offset, end);
>>> + if (!entry)
>>> + return 0;
>>
>> If the buf is "LD_PRELOAD=a.so\0LD_PRELOAD=b.so" and argc=0,
>> we may skip the first entry?
>
> I think I need to rename the "array_next_entry" function / document it
> better.
>
> The function updates the offset to the next location and the
> returns the entry at the current offset.
>
> So, in the first instance:
>
> // offset is the offset into the first page.
> entry = buf + offset;
> offset = <updated value to the next entry>.
Yes, some documentation will help.
>
>>
>>
>>> +
>>> + if (!strncmp(entry, name, name_len)) {
>>> + num_vars++;
>>
>> There helper permits n_size = 0 (ARG_CONST_SIZE_OR_ZERO).
>> in this case, name_len = 0, strncmp(entry, name, name_len) will be always 0.
>
> Thanks, you are right, It does not make sense to have name_len = 0. I
> will change it to ARG_CONST_SIZE.
>
>>
>>> +
>>> + /*
>>> + * There is no need to do further copying
>>> + * if the buffer is already full. Just count how many
>>> + * times the environment variable is set.
>>> + */
>>> + if (ret == -E2BIG)
>>> + continue;
>>> +
>>> + if (entry[name_len] != '=')
>>> + continue;
>>> +
>>> + /*
>>> + * Move the buf pointer by name_len + 1
>>> + * (for the "=" sign)
>>> + */
>>> + entry += name_len + 1;
>>> + ret = strlcpy(curr_dest, entry, size);
>>> +
>>> + if (ret >= size) {
>>> + ret = -E2BIG;
>>
>> Here, we have a partial copy. Should you instead nullify (memset) it as
>> it is not really invalid one?
>
> The function does specify that the it will return a null terminated
> value even if an -E2BIG is returned so that user does get a truncated
> value. It's better to give the user some data. (I mentioned this in
> the documentation for the helper).
Do you know what user could do with this partial data?
Again, I am not a security expert, maybe partial data can still
be used in meaningful.
>
>>
>>> + continue;
>>> + }
>>> +
>>> + /*
>>> + * strlcpy just returns the length of the string copied.
>>> + * The remaining space needs to account for the added
>>> + * null character.
>>> + */
>>> + curr_dest += ret + 1;
>>> + size -= ret + 1;
>>> + /*
>>> + * Update ret to be the current number of bytes written
>>> + * to the destination
>>> + */
>>> + ret = curr_dest - dest;
>>> + }
>>> + }
>>> +
>>> + return (u64) num_vars << 32 | (u32) ret;
>>> +}
>>> +
>>> +BPF_CALL_5(krsi_get_env_var, struct krsi_ctx *, ctx, char *, name, u32, n_size,
>>> + char *, dest, u32, size)
>>> +{
>>> + char *name_end;
>>> +
>>> + name_end = memchr(name, '\0', n_size);
>>> + if (!name_end)
>>> + return -EINVAL;
>>> +
>>> + memset(dest, 0, size);
>
> This memset ensures the buffer is zeroed out (incase the buffer is
> fully / partially empty).
>
>>> + return get_env_var(ctx, name, dest, n_size, size);
>>> +}
>>> +
>>> +static const struct bpf_func_proto krsi_get_env_var_proto = {
>>> + .func = krsi_get_env_var,
>>> + .gpl_only = true,
>>> + .ret_type = RET_INTEGER,
>>> + .arg1_type = ARG_PTR_TO_CTX,
>>> + .arg2_type = ARG_PTR_TO_MEM,
>>> + .arg3_type = ARG_CONST_SIZE_OR_ZERO,
>>> + .arg4_type = ARG_PTR_TO_UNINIT_MEM,
>>> + .arg5_type = ARG_CONST_SIZE_OR_ZERO,
>>> +};
>>> +
>>> BPF_CALL_5(krsi_event_output, void *, log,
>>> struct bpf_map *, map, u64, flags, void *, data, u64, size)
>>> {
>>> @@ -192,6 +319,8 @@ static const struct bpf_func_proto *krsi_prog_func_proto(enum bpf_func_id
>>> return &bpf_map_lookup_elem_proto;
>>> case BPF_FUNC_get_current_pid_tgid:
>>> return &bpf_get_current_pid_tgid_proto;
>>> + case BPF_FUNC_krsi_get_env_var:
>>> + return &krsi_get_env_var_proto;
>>> case BPF_FUNC_perf_event_output:
>>> return &krsi_event_output_proto;
>>> default:
>>> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
>>> index 32ab38f1a2fe..a4ef07956e07 100644
>>> --- a/tools/include/uapi/linux/bpf.h
>>> +++ b/tools/include/uapi/linux/bpf.h
>>> @@ -2715,6 +2715,45 @@ union bpf_attr {
>>> * **-EPERM** if no permission to send the *sig*.
>>> *
>>> * **-EAGAIN** if bpf program can try again.
>>> + *
>>> + * u64 krsi_get_env_var(void *ctx, char *name, char *buf,
>>> + * size_t name_len, size_t buf_len)
>>
>> Inconsistent helper definitions.
>
> As discussed above, I can change the BPF_CALL_5 declaration to have
> a (void *) and cast to the krsi_ctx in the helper itself.
>
> - KP
>
>>
>>> + * Description
>>> + * This helper can be used as a part of the
>>> + * process_execution hook of the KRSI LSM in
>>> + * programs of type BPF_PROG_TYPE_KRSI.
>>> + *
>>> + * The helper returns the value of the environment
>>> + * variable with the provided "name" for process that's
>>> + * going to be executed in the passed buffer, "buf". If the var
>>> + * is set multiple times, the helper returns all
>>> + * the values as null separated strings.
>>> + *
>>> + * If the buffer is too short for these values, the helper
>>> + * tries to fill it the best it can and guarantees that the value
>>> + * returned in the buffer is always null terminated.
>>> + * After the buffer is filled, the helper keeps counting the number
>>> + * of times the environment variable is set in the envp.
>>> + *
>>> + * Return:
>>> + *
>>> + * The return value of the helper is an u64 value
>>> + * which carries two pieces of information:
>>> + *
>>> + * The upper 32 bits are a u32 value signifying
>>> + * the number of times the environment variable
>>> + * is set in the envp.
>>> + * The lower 32 bits are an s32 value signifying
>>> + * the number of bytes written to the buffer or an error code:
>>> + *
>>> + * **-ENOMEM** if the kernel is unable to allocate memory
>>> + * for pinning the argv and envv.
>>> + *
>>> + * **-E2BIG** if the value is larger than the size of the
>>> + * destination buffer. The higher bits will still
>>> + * the number of times the variable was set in the envp.
>>> + *
>>> + * **-EINVAL** if name is not a NULL terminated string.
>>> */
>>> #define __BPF_FUNC_MAPPER(FN) \
>>> FN(unspec), \
>>> @@ -2826,7 +2865,8 @@ union bpf_attr {
>>> FN(strtoul), \
>>> FN(sk_storage_get), \
>>> FN(sk_storage_delete), \
>>> - FN(send_signal),
>>> + FN(send_signal), \
>>> + FN(krsi_get_env_var),
>>>
>>> /* integer value in 'imm' field of BPF_CALL instruction selects which helper
>>> * function eBPF program intends to call
>>> diff --git a/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h
>>> index f804f210244e..ecebdb772a9d 100644
>>> --- a/tools/testing/selftests/bpf/bpf_helpers.h
>>> +++ b/tools/testing/selftests/bpf/bpf_helpers.h
>>> @@ -303,6 +303,9 @@ static int (*bpf_get_numa_node_id)(void) =
>>> static int (*bpf_probe_read_str)(void *ctx, __u32 size,
>>> const void *unsafe_ptr) =
>>> (void *) BPF_FUNC_probe_read_str;
>>> +static unsigned long long (*krsi_get_env_var)(void *ctx,
>>> + void *name, __u32 n_size, void *buf, __u32 size) =
>>> + (void *) BPF_FUNC_krsi_get_env_var;
>>> static unsigned int (*bpf_get_socket_uid)(void *ctx) =
>>> (void *) BPF_FUNC_get_socket_uid;
>>> static unsigned int (*bpf_set_hash)(void *ctx, __u32 hash) =
>>>
^ permalink raw reply
* Re: [PATCH 1/3] ima: keep the integrity state of open files up to date
From: Janne Karhunen @ 2019-09-17 7:24 UTC (permalink / raw)
To: Eric Biggers
Cc: linux-integrity, linux-security-module, Mimi Zohar, linux-mm,
viro, Konsta Karsisto
In-Reply-To: <20190917042334.GA1436@sol.localdomain>
On Tue, Sep 17, 2019 at 7:23 AM Eric Biggers <ebiggers@kernel.org> wrote:
> > > Who will use it if it isn't 100% safe?
> >
> > I suppose anyone using mutable data with IMA appraise should, unless
> > they have a redundant power supply and a kernel that never crashes. In
> > a way this is like asking if the ima-appraise should be there for
> > mutable data at all. All this is doing is that it improves the crash
> > recovery reliability without taking anything away.
>
> Okay, so why would anyone use mutable data with IMA appraise if it corrupts your
> files by design, both with and without this patchset?
Now you are exaggerating heavily: it does not corrupt your files by
design. A crash in any security related system is supposed to be
pretty rare occurrence.
> > Anyway, I think I'm getting along with my understanding of the page
> > writeback slowly and the journal support will eventually be there at
> > least as an add-on patch for those that want to use it and really need
> > the last 0.n% reliability. Note that even without that patch you can
> > build ima-appraise based systems that are 99.999% reliable just by
>
> On what storage devices, workloads, and filesystems is this number for?
I reached 99.2% recovery rate with the AOSP without touching the
android on top by crashing the kernel with a test case while the
device was in use. 80% if I crash it while the device is in the
busiest write cycle (the first boot, I guess we would suck quite
royally if we never made past this point without dying).
99.95+% of course requires a high-availability system that probably
crashes once per year at best and recovers in seconds. In that case
this will recover it with pretty high odds, so reliability is not all
that much reduced from it's normal reliability statistics. So, the
ima-appraise for the mutable data could be in use even in a
high-availability system. 99% recovery probability for the crash that
occurs once per year would be OK; 0% would not be. I suppose it all
depends on your requirements.
> > having the patch we're discussing here. Without it you would be orders
> > of magnitude worse off. All we are doing is that we give it a fairly
> > good chance to recover instead of giving up without even trying.
> >
> > That said, I'm not sure the 100% crash recovery is ever guaranteed in
> > any Linux system. We just have to do what we can, no?
>
> Filesystems implement consistency mechanisms, e.g. journalling or copy-on-write,
> to recover from crashes by design. This patchset doesn't implement or use any
> such mechanism, so it's not crash-safe. It's not clear that it's even a step in
> the right direction, as no patches have been proposed for a correct solution so
> we can see what it actually involves.
Great, what would be the better alternative? I guess the suggestion
cannot be that 'don't use it' since the code is there?
As for the 'step to the right direction': before we could talk about
any of this journaling stuff we had to make sure that we have the
plumbing where the measurements are accurate. These patches do that
and the journaling is the next step. All the journaling add-on does
now is that it binds the page write and the xattr update into one
transaction, so both of those run as sub-transactions of one master.
Now, only when the master ends the data is moved out of the journal in
one bundle. All this is so ridiculously simple I doubt my own eyes,
but it seems to work fine apart from some slowdown on shutdown when
processes call sync() like there is no tomorrow. Nevertheless,
understanding the related code (the page writeback and the ext4) is
pretty nasty and there are lots of things I need to understand about
that still. The thing I'm currently trying to get my head around is
that whether or not it is possible that we have a measurement over a
page that was not eligible for the writeback. I'm also no ext4 expert
so all help in that regard is highly appreciated if this type of thing
is interesting to others.
Anyway, all this is good info. If this code is not needed upstream,
I'm happy to stop working with it and will maintain this for my use
only. Let me know,
--
Janne
^ permalink raw reply
* Re: [PATCH 1/3] ima: keep the integrity state of open files up to date
From: Eric Biggers @ 2019-09-17 4:23 UTC (permalink / raw)
To: Janne Karhunen
Cc: linux-integrity, linux-security-module, Mimi Zohar, linux-mm,
viro, Konsta Karsisto
In-Reply-To: <CAE=NcrbaJD4CaUvg1tmNSSKjkG-EizNM7GUaztA0=fiUCo03Cg@mail.gmail.com>
On Mon, Sep 16, 2019 at 02:45:56PM +0300, Janne Karhunen wrote:
> On Sun, Sep 15, 2019 at 11:24 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> > > > This still doesn't make it crash-safe. So why is it okay?
> > >
> > > If Android is the load, this makes it crash safe 99% of the time and
> > > that is considerably better than 0% of the time.
> > >
> >
> > Who will use it if it isn't 100% safe?
>
> I suppose anyone using mutable data with IMA appraise should, unless
> they have a redundant power supply and a kernel that never crashes. In
> a way this is like asking if the ima-appraise should be there for
> mutable data at all. All this is doing is that it improves the crash
> recovery reliability without taking anything away.
Okay, so why would anyone use mutable data with IMA appraise if it corrupts your
files by design, both with and without this patchset?
>
> Anyway, I think I'm getting along with my understanding of the page
> writeback slowly and the journal support will eventually be there at
> least as an add-on patch for those that want to use it and really need
> the last 0.n% reliability. Note that even without that patch you can
> build ima-appraise based systems that are 99.999% reliable just by
On what storage devices, workloads, and filesystems is this number for?
> having the patch we're discussing here. Without it you would be orders
> of magnitude worse off. All we are doing is that we give it a fairly
> good chance to recover instead of giving up without even trying.
>
> That said, I'm not sure the 100% crash recovery is ever guaranteed in
> any Linux system. We just have to do what we can, no?
>
Filesystems implement consistency mechanisms, e.g. journalling or copy-on-write,
to recover from crashes by design. This patchset doesn't implement or use any
such mechanism, so it's not crash-safe. It's not clear that it's even a step in
the right direction, as no patches have been proposed for a correct solution so
we can see what it actually involves.
- Eric
^ permalink raw reply
* Re: [GIT PULL] integrity subsystem updates for v5.4
From: Mimi Zohar @ 2019-09-16 22:13 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-security-module, linux-integrity, linux-kernel
In-Reply-To: <CAHk-=whuzoK+sP+feizU520p7ChHqdX8pmwyCnnKTyUNJKngZA@mail.gmail.com>
On Mon, 2019-09-16 at 13:38 -0700, Linus Torvalds wrote:
> On Wed, Sep 11, 2019 at 2:29 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> >
> > The major feature in this pull request is IMA support for measuring
> > and appraising appended file signatures. In addition are a couple of
> > bug fixes and code cleanup to use struct_size().
>
> How is the file signature any different from (and/or better than) the
> fs-verity support?
>
> The fs-verity support got fairly extensively discussed, and is
> apparently going to actually be widely used by Android, and it an
> independent feature of any security model.
>
> What does the IMA version bring to the table?
IMA currently defines a system wide policy for measuring, verifying a
file's integrity (both mutable/immutable files) against known good
values, and adding audit records containing the file hashes. The
policy isn't hard coded in the kernel, allowing people/companies to
configure it as desired for their specific use case.
Support for appended signatures already exists in the kernel for
kernel modules. This pull request adds IMA support for appended
signatures in order to verify the kexec kernel image on OpenPOWER, as
part of Secure and Trusted boot enablement. This would allow distros
to sign kernel images similar to how they currently sign kernel
modules.
IMA verifies file signatures up front, before allowing access to the
file. fs-verity verifies the signature of the Merkle tree (and other
info), but does not verify the file data at the time of first use.
There are pros and cons to each of these approaches.
Mimi
^ permalink raw reply
* Re: [GIT PULL] integrity subsystem updates for v5.4
From: Linus Torvalds @ 2019-09-16 20:38 UTC (permalink / raw)
To: Mimi Zohar; +Cc: linux-security-module, linux-integrity, linux-kernel
In-Reply-To: <1568237365.5783.39.camel@linux.ibm.com>
On Wed, Sep 11, 2019 at 2:29 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> The major feature in this pull request is IMA support for measuring
> and appraising appended file signatures. In addition are a couple of
> bug fixes and code cleanup to use struct_size().
How is the file signature any different from (and/or better than) the
fs-verity support?
The fs-verity support got fairly extensively discussed, and is
apparently going to actually be widely used by Android, and it an
independent feature of any security model.
What does the IMA version bring to the table?
Linus
^ permalink raw reply
* Re: [PATCH v8 04/28] LSM: Create and manage the lsmblob data structure.
From: Stephen Smalley @ 2019-09-16 19:15 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul
In-Reply-To: <20190829232935.7099-5-casey@schaufler-ca.com>
On 8/29/19 7:29 PM, Casey Schaufler wrote:
> When more than one security module is exporting data to
> audit and networking sub-systems a single 32 bit integer
> is no longer sufficient to represent the data. Add a
> structure to be used instead.
>
> The lsmblob structure is currently an array of
> u32 "secids". There is an entry for each of the
> security modules built into the system that would
> use secids if active. The system assigns the module
> a "slot" when it registers hooks. If modules are
> compiled in but not registered there will be unused
> slots.
I won't obstruct the patch on naming but I do have to wonder about the
use of lsmblob here given that we use the term "security blobs"
elsewhere to refer to what the ->security fields reference and this is
not the same thing. Not sure why it wasn't just lsmsecids or similar,
but whatever.
>
> A new lsm_id structure, which contains the name
> of the LSM and its slot number, is created. There
> is an instance for each LSM, which assigns the name
> and passes it to the infrastructure to set the slot.
Is it really desirable/necessary to duplicate the module name, once in
the lsm_id and once in the lsm_info? Again, not a blocker for me but
seems unfortunate.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: John Johansen <john.johansen@canonical.com>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
> include/linux/lsm_hooks.h | 12 +++++--
> include/linux/security.h | 66 ++++++++++++++++++++++++++++++++++++++
> security/apparmor/lsm.c | 7 +++-
> security/commoncap.c | 7 +++-
> security/loadpin/loadpin.c | 8 ++++-
> security/safesetid/lsm.c | 8 ++++-
> security/security.c | 31 ++++++++++++++----
> security/selinux/hooks.c | 8 ++++-
> security/smack/smack_lsm.c | 7 +++-
> security/tomoyo/tomoyo.c | 8 ++++-
> security/yama/yama_lsm.c | 7 +++-
> 11 files changed, 152 insertions(+), 17 deletions(-)
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 3fe39abccc8f..fe1fb7a69ee5 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -2029,6 +2029,14 @@ struct security_hook_heads {
> #endif /* CONFIG_BPF_SYSCALL */
> } __randomize_layout;
>
> +/*
> + * Information that identifies a security module.
> + */
> +struct lsm_id {
> + const char *lsm; /* Name of the LSM */
> + int slot; /* Slot in lsmblob if one is allocated */
> +};
> +
> /*
> * Security module hook list structure.
> * For use with generic list macros for common operations.
> @@ -2037,7 +2045,7 @@ struct security_hook_list {
> struct hlist_node list;
> struct hlist_head *head;
> union security_list_options hook;
> - char *lsm;
> + struct lsm_id *lsmid;
> } __randomize_layout;
>
> /*
> @@ -2068,7 +2076,7 @@ extern struct security_hook_heads security_hook_heads;
> extern char *lsm_names;
>
> extern void security_add_hooks(struct security_hook_list *hooks, int count,
> - char *lsm);
> + struct lsm_id *lsmid);
>
> #define LSM_FLAG_LEGACY_MAJOR BIT(0)
> #define LSM_FLAG_EXCLUSIVE BIT(1)
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 49f2685324b0..5bb8b9a6fa84 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -76,6 +76,72 @@ enum lsm_event {
> LSM_POLICY_CHANGE,
> };
>
> +/*
> + * Data exported by the security modules
> + *
> + * Any LSM that provides secid or secctx based hooks must be included.
> + */
> +#define LSMBLOB_ENTRIES ( \
> + (IS_ENABLED(CONFIG_SECURITY_SELINUX) ? 1 : 0) + \
> + (IS_ENABLED(CONFIG_SECURITY_SMACK) ? 1 : 0) + \
> + (IS_ENABLED(CONFIG_SECURITY_APPARMOR) ? 1 : 0))
> +
> +struct lsmblob {
> + u32 secid[LSMBLOB_ENTRIES];
> +};
> +
> +#define LSMBLOB_INVALID -1 /* Not a valid LSM slot number */
> +#define LSMBLOB_NEEDED -2 /* Slot requested on initialization */
> +#define LSMBLOB_NOT_NEEDED -3 /* Slot not requested */
> +
> +/**
> + * lsmblob_init - initialize an lsmblob structure.
> + * @blob: Pointer to the data to initialize
> + * @secid: The initial secid value
> + *
> + * Set all secid for all modules to the specified value.
> + */
> +static inline void lsmblob_init(struct lsmblob *blob, u32 secid)
> +{
> + int i;
> +
> + for (i = 0; i < LSMBLOB_ENTRIES; i++)
> + blob->secid[i] = secid;
> +}
This seems nonsensical for any value other than 0. Otherwise, we should
only set it for the module that produced the secid originally? And if
setting them all to zero, you could just do a memset.
> +
> +/**
> + * lsmblob_is_set - report if there is an value in the lsmblob
> + * @blob: Pointer to the exported LSM data
> + *
> + * Returns true if there is a secid set, false otherwise
> + */
> +static inline bool lsmblob_is_set(struct lsmblob *blob)
> +{
> + int i;
> +
> + for (i = 0; i < LSMBLOB_ENTRIES; i++)
> + if (blob->secid[i] != 0)
> + return true;
> + return false;
memcmp against the all-zeroes lsmblob?
> +}
> +
> +/**
> + * lsmblob_equal - report if the two lsmblob's are equal
> + * @bloba: Pointer to one LSM data
> + * @blobb: Pointer to the other LSM data
> + *
> + * Returns true if all entries in the two are equal, false otherwise
> + */
> +static inline bool lsmblob_equal(struct lsmblob *bloba, struct lsmblob *blobb)
> +{
> + int i;
> +
> + for (i = 0; i < LSMBLOB_ENTRIES; i++)
> + if (bloba->secid[i] != blobb->secid[i])
> + return false;
> + return true;
memcmp of the two blobs?
> +}
> +
> /* These functions are in security/commoncap.c */
> extern int cap_capable(const struct cred *cred, struct user_namespace *ns,
> int cap, unsigned int opts);
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index 2716e7731279..ec2e39aa9a84 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -1138,6 +1138,11 @@ struct lsm_blob_sizes apparmor_blob_sizes __lsm_ro_after_init = {
> .lbs_sock = sizeof(struct aa_sk_ctx),
> };
>
> +static struct lsm_id apparmor_lsmid __lsm_ro_after_init = {
> + .lsm = "apparmor",
> + .slot = LSMBLOB_NEEDED
> +};
> +
> static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = {
> LSM_HOOK_INIT(ptrace_access_check, apparmor_ptrace_access_check),
> LSM_HOOK_INIT(ptrace_traceme, apparmor_ptrace_traceme),
> @@ -1679,7 +1684,7 @@ static int __init apparmor_init(void)
> goto buffers_out;
> }
> security_add_hooks(apparmor_hooks, ARRAY_SIZE(apparmor_hooks),
> - "apparmor");
> + &apparmor_lsmid);
>
> /* Report that AppArmor successfully initialized */
> apparmor_initialized = 1;
> diff --git a/security/commoncap.c b/security/commoncap.c
> index afd9679ca866..973e6c7009d0 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -1344,6 +1344,11 @@ int cap_mmap_file(struct file *file, unsigned long reqprot,
>
> #ifdef CONFIG_SECURITY
>
> +static struct lsm_id capability_lsmid __lsm_ro_after_init = {
> + .lsm = "capability",
> + .slot = LSMBLOB_NOT_NEEDED
> +};
> +
> static struct security_hook_list capability_hooks[] __lsm_ro_after_init = {
> LSM_HOOK_INIT(capable, cap_capable),
> LSM_HOOK_INIT(settime, cap_settime),
> @@ -1368,7 +1373,7 @@ static struct security_hook_list capability_hooks[] __lsm_ro_after_init = {
> static int __init capability_init(void)
> {
> security_add_hooks(capability_hooks, ARRAY_SIZE(capability_hooks),
> - "capability");
> + &capability_lsmid);
> return 0;
> }
>
> diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c
> index 055fb0a64169..7b23fdf24e27 100644
> --- a/security/loadpin/loadpin.c
> +++ b/security/loadpin/loadpin.c
> @@ -181,6 +181,11 @@ static int loadpin_load_data(enum kernel_load_data_id id)
> return loadpin_read_file(NULL, (enum kernel_read_file_id) id);
> }
>
> +static struct lsm_id loadpin_lsmid __lsm_ro_after_init = {
> + .lsm = "loadpin",
> + .slot = LSMBLOB_NOT_NEEDED
> +};
> +
> static struct security_hook_list loadpin_hooks[] __lsm_ro_after_init = {
> LSM_HOOK_INIT(sb_free_security, loadpin_sb_free_security),
> LSM_HOOK_INIT(kernel_read_file, loadpin_read_file),
> @@ -191,7 +196,8 @@ static int __init loadpin_init(void)
> {
> pr_info("ready to pin (currently %senforcing)\n",
> enforce ? "" : "not ");
> - security_add_hooks(loadpin_hooks, ARRAY_SIZE(loadpin_hooks), "loadpin");
> + security_add_hooks(loadpin_hooks, ARRAY_SIZE(loadpin_hooks),
> + &loadpin_lsmid);
> return 0;
> }
>
> diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c
> index cecd38e2ac80..4a96cd8c0d15 100644
> --- a/security/safesetid/lsm.c
> +++ b/security/safesetid/lsm.c
> @@ -255,6 +255,11 @@ void flush_safesetid_whitelist_entries(void)
> }
> }
>
> +static struct lsm_id safesetid_lsmid __lsm_ro_after_init = {
> + .lsm = "safesetid",
> + .slot = LSMBLOB_NOT_NEEDED
> +};
> +
> static struct security_hook_list safesetid_security_hooks[] = {
> LSM_HOOK_INIT(task_fix_setuid, safesetid_task_fix_setuid),
> LSM_HOOK_INIT(capable, safesetid_security_capable)
> @@ -263,7 +268,8 @@ static struct security_hook_list safesetid_security_hooks[] = {
> static int __init safesetid_security_init(void)
> {
> security_add_hooks(safesetid_security_hooks,
> - ARRAY_SIZE(safesetid_security_hooks), "safesetid");
> + ARRAY_SIZE(safesetid_security_hooks),
> + &safesetid_lsmid);
>
> /* Report that SafeSetID successfully initialized */
> safesetid_initialized = 1;
> diff --git a/security/security.c b/security/security.c
> index 7cfedb90210a..27e2db3d6b04 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -317,6 +317,7 @@ static void __init ordered_lsm_init(void)
> init_debug("sock blob size = %d\n", blob_sizes.lbs_sock);
> init_debug("superblock blob size = %d\n", blob_sizes.lbs_superblock);
> init_debug("task blob size = %d\n", blob_sizes.lbs_task);
> + init_debug("lsmblob size = %lu\n", sizeof(struct lsmblob));
>
> /*
> * Create any kmem_caches needed for blobs
> @@ -399,7 +400,7 @@ static bool match_last_lsm(const char *list, const char *lsm)
> return !strcmp(last, lsm);
> }
>
> -static int lsm_append(char *new, char **result)
> +static int lsm_append(const char *new, char **result)
> {
> char *cp;
>
> @@ -420,24 +421,40 @@ static int lsm_append(char *new, char **result)
> return 0;
> }
>
> +/*
> + * Current index to use while initializing the lsmblob secid list.
> + */
> +static int lsm_slot __initdata;
> +
> /**
> * security_add_hooks - Add a modules hooks to the hook lists.
> * @hooks: the hooks to add
> * @count: the number of hooks to add
> - * @lsm: the name of the security module
> + * @lsmid: the identification information for the security module
> *
> * Each LSM has to register its hooks with the infrastructure.
> + * If the LSM is using hooks that export secids allocate a slot
> + * for it in the lsmblob.
> */
> void __init security_add_hooks(struct security_hook_list *hooks, int count,
> - char *lsm)
> + struct lsm_id *lsmid)
> {
> int i;
>
> + if (lsmid->slot == LSMBLOB_NEEDED) {
> + if (lsm_slot >= LSMBLOB_ENTRIES)
> + panic("%s Too many LSMs registered.\n", __func__);
> + lsmid->slot = lsm_slot++;
> + init_debug("%s assigned lsmblob slot %d\n", lsmid->lsm,
> + lsmid->slot);
> + }
> +
> for (i = 0; i < count; i++) {
> - hooks[i].lsm = lsm;
> + hooks[i].lsmid = lsmid;
> hlist_add_tail_rcu(&hooks[i].list, hooks[i].head);
> }
> - if (lsm_append(lsm, &lsm_names) < 0)
> +
> + if (lsm_append(lsmid->lsm, &lsm_names) < 0)
> panic("%s - Cannot get early memory.\n", __func__);
> }
>
> @@ -1917,7 +1934,7 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
> struct security_hook_list *hp;
>
> hlist_for_each_entry(hp, &security_hook_heads.getprocattr, list) {
> - if (lsm != NULL && strcmp(lsm, hp->lsm))
> + if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm))
> continue;
> return hp->hook.getprocattr(p, name, value);
> }
> @@ -1930,7 +1947,7 @@ int security_setprocattr(const char *lsm, const char *name, void *value,
> struct security_hook_list *hp;
>
> hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) {
> - if (lsm != NULL && strcmp(lsm, hp->lsm))
> + if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm))
> continue;
> return hp->hook.setprocattr(name, value, size);
> }
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index c83ec2652eda..74c491980ed2 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -6622,6 +6622,11 @@ struct lsm_blob_sizes selinux_blob_sizes __lsm_ro_after_init = {
> .lbs_superblock = sizeof(struct superblock_security_struct),
> };
>
> +static struct lsm_id selinux_lsmid __lsm_ro_after_init = {
> + .lsm = "selinux",
> + .slot = LSMBLOB_NEEDED
> +};
> +
> static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
> LSM_HOOK_INIT(binder_set_context_mgr, selinux_binder_set_context_mgr),
> LSM_HOOK_INIT(binder_transaction, selinux_binder_transaction),
> @@ -6877,7 +6882,8 @@ static __init int selinux_init(void)
>
> hashtab_cache_init();
>
> - security_add_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks), "selinux");
> + security_add_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks),
> + &selinux_lsmid);
>
> if (avc_add_callback(selinux_netcache_avc_callback, AVC_CALLBACK_RESET))
> panic("SELinux: Unable to register AVC netcache callback\n");
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index e9560b078efe..7a0ead4da479 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -4553,6 +4553,11 @@ struct lsm_blob_sizes smack_blob_sizes __lsm_ro_after_init = {
> .lbs_superblock = sizeof(struct superblock_smack),
> };
>
> +static struct lsm_id smack_lsmid __lsm_ro_after_init = {
> + .lsm = "smack",
> + .slot = LSMBLOB_NEEDED
> +};
> +
> static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
> LSM_HOOK_INIT(ptrace_access_check, smack_ptrace_access_check),
> LSM_HOOK_INIT(ptrace_traceme, smack_ptrace_traceme),
> @@ -4743,7 +4748,7 @@ static __init int smack_init(void)
> /*
> * Register with LSM
> */
> - security_add_hooks(smack_hooks, ARRAY_SIZE(smack_hooks), "smack");
> + security_add_hooks(smack_hooks, ARRAY_SIZE(smack_hooks), &smack_lsmid);
> smack_enabled = 1;
>
> pr_info("Smack: Initializing.\n");
> diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
> index 716c92ec941a..f1968e80f06d 100644
> --- a/security/tomoyo/tomoyo.c
> +++ b/security/tomoyo/tomoyo.c
> @@ -529,6 +529,11 @@ static void tomoyo_task_free(struct task_struct *task)
> }
> }
>
> +static struct lsm_id tomoyo_lsmid __lsm_ro_after_init = {
> + .lsm = "tomoyo",
> + .slot = LSMBLOB_NOT_NEEDED
> +};
> +
> /*
> * tomoyo_security_ops is a "struct security_operations" which is used for
> * registering TOMOYO.
> @@ -581,7 +586,8 @@ static int __init tomoyo_init(void)
> struct tomoyo_task *s = tomoyo_task(current);
>
> /* register ourselves with the security framework */
> - security_add_hooks(tomoyo_hooks, ARRAY_SIZE(tomoyo_hooks), "tomoyo");
> + security_add_hooks(tomoyo_hooks, ARRAY_SIZE(tomoyo_hooks),
> + &tomoyo_lsmid);
> pr_info("TOMOYO Linux initialized\n");
> s->domain_info = &tomoyo_kernel_domain;
> atomic_inc(&tomoyo_kernel_domain.users);
> diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c
> index efac68556b45..0529ecc86954 100644
> --- a/security/yama/yama_lsm.c
> +++ b/security/yama/yama_lsm.c
> @@ -425,6 +425,11 @@ static int yama_ptrace_traceme(struct task_struct *parent)
> return rc;
> }
>
> +static struct lsm_id yama_lsmid __lsm_ro_after_init = {
> + .lsm = "yama",
> + .slot = LSMBLOB_NOT_NEEDED
> +};
> +
> static struct security_hook_list yama_hooks[] __lsm_ro_after_init = {
> LSM_HOOK_INIT(ptrace_access_check, yama_ptrace_access_check),
> LSM_HOOK_INIT(ptrace_traceme, yama_ptrace_traceme),
> @@ -482,7 +487,7 @@ static inline void yama_init_sysctl(void) { }
> static int __init yama_init(void)
> {
> pr_info("Yama: becoming mindful.\n");
> - security_add_hooks(yama_hooks, ARRAY_SIZE(yama_hooks), "yama");
> + security_add_hooks(yama_hooks, ARRAY_SIZE(yama_hooks), &yama_lsmid);
> yama_init_sysctl();
> return 0;
> }
>
^ permalink raw reply
* Re: [PATCH v8 03/28] LSM: Infrastructure management of the key blob
From: Stephen Smalley @ 2019-09-16 18:47 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul
In-Reply-To: <20190829232935.7099-4-casey@schaufler-ca.com>
On 8/29/19 7:29 PM, Casey Schaufler wrote:
> From: Casey Schaufler <cschaufler@schaufler-ca.com>
>
> Move management of the key->security blob out of the
> individual security modules and into the security
> infrastructure. Instead of allocating the blobs from within
> the modules the modules tell the infrastructure how much
> space is required, and the space is allocated there.
As with the superblock blob, I don't see that this is needed for
stacking AA but I don't object to moving all the security blobs to
infrastructure management for consistency.
Reviewed-by: Stephen Smalley <sds@tycho.nsa.gov>
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: John Johansen <jojn.johansen@canonical.com>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
> include/linux/lsm_hooks.h | 1 +
> security/security.c | 40 ++++++++++++++++++++++++++++++-
> security/selinux/hooks.c | 23 +++++-------------
> security/selinux/include/objsec.h | 7 ++++++
> security/smack/smack.h | 7 ++++++
> security/smack/smack_lsm.c | 33 ++++++++++++-------------
> 6 files changed, 75 insertions(+), 36 deletions(-)
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index b353482ea348..3fe39abccc8f 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -2050,6 +2050,7 @@ struct lsm_blob_sizes {
> int lbs_sock;
> int lbs_superblock;
> int lbs_ipc;
> + int lbs_key;
> int lbs_msg_msg;
> int lbs_task;
> };
> diff --git a/security/security.c b/security/security.c
> index 2c0834db7976..7cfedb90210a 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -172,6 +172,9 @@ static void __init lsm_set_blob_sizes(struct lsm_blob_sizes *needed)
> blob_sizes.lbs_inode = sizeof(struct rcu_head);
> lsm_set_blob_size(&needed->lbs_inode, &blob_sizes.lbs_inode);
> lsm_set_blob_size(&needed->lbs_ipc, &blob_sizes.lbs_ipc);
> +#ifdef CONFIG_KEYS
> + lsm_set_blob_size(&needed->lbs_key, &blob_sizes.lbs_key);
> +#endif
> lsm_set_blob_size(&needed->lbs_msg_msg, &blob_sizes.lbs_msg_msg);
> lsm_set_blob_size(&needed->lbs_sock, &blob_sizes.lbs_sock);
> lsm_set_blob_size(&needed->lbs_superblock, &blob_sizes.lbs_superblock);
> @@ -307,6 +310,9 @@ static void __init ordered_lsm_init(void)
> init_debug("file blob size = %d\n", blob_sizes.lbs_file);
> init_debug("inode blob size = %d\n", blob_sizes.lbs_inode);
> init_debug("ipc blob size = %d\n", blob_sizes.lbs_ipc);
> +#ifdef CONFIG_KEYS
> + init_debug("key blob size = %d\n", blob_sizes.lbs_key);
> +#endif /* CONFIG_KEYS */
> init_debug("msg_msg blob size = %d\n", blob_sizes.lbs_msg_msg);
> init_debug("sock blob size = %d\n", blob_sizes.lbs_sock);
> init_debug("superblock blob size = %d\n", blob_sizes.lbs_superblock);
> @@ -573,6 +579,29 @@ static int lsm_ipc_alloc(struct kern_ipc_perm *kip)
> return 0;
> }
>
> +#ifdef CONFIG_KEYS
> +/**
> + * lsm_key_alloc - allocate a composite key blob
> + * @key: the key that needs a blob
> + *
> + * Allocate the key blob for all the modules
> + *
> + * Returns 0, or -ENOMEM if memory can't be allocated.
> + */
> +static int lsm_key_alloc(struct key *key)
> +{
> + if (blob_sizes.lbs_key == 0) {
> + key->security = NULL;
> + return 0;
> + }
> +
> + key->security = kzalloc(blob_sizes.lbs_key, GFP_KERNEL);
> + if (key->security == NULL)
> + return -ENOMEM;
> + return 0;
> +}
> +#endif /* CONFIG_KEYS */
> +
> /**
> * lsm_msg_msg_alloc - allocate a composite msg_msg blob
> * @mp: the msg_msg that needs a blob
> @@ -2339,12 +2368,21 @@ EXPORT_SYMBOL(security_skb_classify_flow);
> int security_key_alloc(struct key *key, const struct cred *cred,
> unsigned long flags)
> {
> - return call_int_hook(key_alloc, 0, key, cred, flags);
> + int rc = lsm_key_alloc(key);
> +
> + if (unlikely(rc))
> + return rc;
> + rc = call_int_hook(key_alloc, 0, key, cred, flags);
> + if (unlikely(rc))
> + security_key_free(key);
> + return rc;
> }
>
> void security_key_free(struct key *key)
> {
> call_void_hook(key_free, key);
> + kfree(key->security);
> + key->security = NULL;
> }
>
> int security_key_permission(key_ref_t key_ref,
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 5d74ed35b728..c83ec2652eda 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -6353,11 +6353,7 @@ static int selinux_key_alloc(struct key *k, const struct cred *cred,
> unsigned long flags)
> {
> const struct task_security_struct *tsec;
> - struct key_security_struct *ksec;
> -
> - ksec = kzalloc(sizeof(struct key_security_struct), GFP_KERNEL);
> - if (!ksec)
> - return -ENOMEM;
> + struct key_security_struct *ksec = selinux_key(k);
>
> tsec = selinux_cred(cred);
> if (tsec->keycreate_sid)
> @@ -6365,18 +6361,9 @@ static int selinux_key_alloc(struct key *k, const struct cred *cred,
> else
> ksec->sid = tsec->sid;
>
> - k->security = ksec;
> return 0;
> }
>
> -static void selinux_key_free(struct key *k)
> -{
> - struct key_security_struct *ksec = k->security;
> -
> - k->security = NULL;
> - kfree(ksec);
> -}
> -
> static int selinux_key_permission(key_ref_t key_ref,
> const struct cred *cred,
> unsigned perm)
> @@ -6394,7 +6381,7 @@ static int selinux_key_permission(key_ref_t key_ref,
> sid = cred_sid(cred);
>
> key = key_ref_to_ptr(key_ref);
> - ksec = key->security;
> + ksec = selinux_key(key);
>
> return avc_has_perm(&selinux_state,
> sid, ksec->sid, SECCLASS_KEY, perm, NULL);
> @@ -6402,7 +6389,7 @@ static int selinux_key_permission(key_ref_t key_ref,
>
> static int selinux_key_getsecurity(struct key *key, char **_buffer)
> {
> - struct key_security_struct *ksec = key->security;
> + struct key_security_struct *ksec = selinux_key(key);
> char *context = NULL;
> unsigned len;
> int rc;
> @@ -6627,6 +6614,9 @@ struct lsm_blob_sizes selinux_blob_sizes __lsm_ro_after_init = {
> .lbs_file = sizeof(struct file_security_struct),
> .lbs_inode = sizeof(struct inode_security_struct),
> .lbs_ipc = sizeof(struct ipc_security_struct),
> +#ifdef CONFIG_KEYS
> + .lbs_key = sizeof(struct key_security_struct),
> +#endif /* CONFIG_KEYS */
> .lbs_msg_msg = sizeof(struct msg_security_struct),
> .lbs_sock = sizeof(struct sk_security_struct),
> .lbs_superblock = sizeof(struct superblock_security_struct),
> @@ -6842,7 +6832,6 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
>
> #ifdef CONFIG_KEYS
> LSM_HOOK_INIT(key_alloc, selinux_key_alloc),
> - LSM_HOOK_INIT(key_free, selinux_key_free),
> LSM_HOOK_INIT(key_permission, selinux_key_permission),
> LSM_HOOK_INIT(key_getsecurity, selinux_key_getsecurity),
> #endif
> diff --git a/security/selinux/include/objsec.h b/security/selinux/include/objsec.h
> index 29f02b8f8f31..3b78aa4ee98f 100644
> --- a/security/selinux/include/objsec.h
> +++ b/security/selinux/include/objsec.h
> @@ -194,6 +194,13 @@ static inline struct superblock_security_struct *selinux_superblock(
> return superblock->s_security + selinux_blob_sizes.lbs_superblock;
> }
>
> +#ifdef CONFIG_KEYS
> +static inline struct key_security_struct *selinux_key(const struct key *key)
> +{
> + return key->security + selinux_blob_sizes.lbs_key;
> +}
> +#endif /* CONFIG_KEYS */
> +
> static inline struct sk_security_struct *selinux_sock(const struct sock *sock)
> {
> return sock->sk_security + selinux_blob_sizes.lbs_sock;
> diff --git a/security/smack/smack.h b/security/smack/smack.h
> index 4ac4bf3310d7..7cc3a3382fee 100644
> --- a/security/smack/smack.h
> +++ b/security/smack/smack.h
> @@ -386,6 +386,13 @@ static inline struct superblock_smack *smack_superblock(
> return superblock->s_security + smack_blob_sizes.lbs_superblock;
> }
>
> +#ifdef CONFIG_KEYS
> +static inline struct smack_known **smack_key(const struct key *key)
> +{
> + return key->security + smack_blob_sizes.lbs_key;
> +}
> +#endif /* CONFIG_KEYS */
> +
> /*
> * Is the directory transmuting?
> */
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index fd69e1bd841b..e9560b078efe 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -4179,23 +4179,13 @@ static void smack_inet_csk_clone(struct sock *sk,
> static int smack_key_alloc(struct key *key, const struct cred *cred,
> unsigned long flags)
> {
> + struct smack_known **blob = smack_key(key);
> struct smack_known *skp = smk_of_task(smack_cred(cred));
>
> - key->security = skp;
> + *blob = skp;
> return 0;
> }
>
> -/**
> - * smack_key_free - Clear the key security blob
> - * @key: the object
> - *
> - * Clear the blob pointer
> - */
> -static void smack_key_free(struct key *key)
> -{
> - key->security = NULL;
> -}
> -
> /**
> * smack_key_permission - Smack access on a key
> * @key_ref: gets to the object
> @@ -4208,6 +4198,8 @@ static void smack_key_free(struct key *key)
> static int smack_key_permission(key_ref_t key_ref,
> const struct cred *cred, unsigned perm)
> {
> + struct smack_known **blob;
> + struct smack_known *skp;
> struct key *keyp;
> struct smk_audit_info ad;
> struct smack_known *tkp = smk_of_task(smack_cred(cred));
> @@ -4227,7 +4219,9 @@ static int smack_key_permission(key_ref_t key_ref,
> * If the key hasn't been initialized give it access so that
> * it may do so.
> */
> - if (keyp->security == NULL)
> + blob = smack_key(keyp);
> + skp = *blob;
> + if (skp == NULL)
> return 0;
> /*
> * This should not occur
> @@ -4247,8 +4241,8 @@ static int smack_key_permission(key_ref_t key_ref,
> request |= MAY_READ;
> if (perm & (KEY_NEED_WRITE | KEY_NEED_LINK | KEY_NEED_SETATTR))
> request |= MAY_WRITE;
> - rc = smk_access(tkp, keyp->security, request, &ad);
> - rc = smk_bu_note("key access", tkp, keyp->security, request, rc);
> + rc = smk_access(tkp, skp, request, &ad);
> + rc = smk_bu_note("key access", tkp, skp, request, rc);
> return rc;
> }
>
> @@ -4263,11 +4257,12 @@ static int smack_key_permission(key_ref_t key_ref,
> */
> static int smack_key_getsecurity(struct key *key, char **_buffer)
> {
> - struct smack_known *skp = key->security;
> + struct smack_known **blob = smack_key(key);
> + struct smack_known *skp = *blob;
> size_t length;
> char *copy;
>
> - if (key->security == NULL) {
> + if (skp == NULL) {
> *_buffer = NULL;
> return 0;
> }
> @@ -4550,6 +4545,9 @@ struct lsm_blob_sizes smack_blob_sizes __lsm_ro_after_init = {
> .lbs_file = sizeof(struct smack_known *),
> .lbs_inode = sizeof(struct inode_smack),
> .lbs_ipc = sizeof(struct smack_known *),
> +#ifdef CONFIG_KEYS
> + .lbs_key = sizeof(struct smack_known *),
> +#endif /* CONFIG_KEYS */
> .lbs_msg_msg = sizeof(struct smack_known *),
> .lbs_sock = sizeof(struct socket_smack),
> .lbs_superblock = sizeof(struct superblock_smack),
> @@ -4671,7 +4669,6 @@ static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
> /* key management security hooks */
> #ifdef CONFIG_KEYS
> LSM_HOOK_INIT(key_alloc, smack_key_alloc),
> - LSM_HOOK_INIT(key_free, smack_key_free),
> LSM_HOOK_INIT(key_permission, smack_key_permission),
> LSM_HOOK_INIT(key_getsecurity, smack_key_getsecurity),
> #endif /* CONFIG_KEYS */
>
^ permalink raw reply
* Re: [PATCH v8 02/28] LSM: Infrastructure management of the sock security
From: Stephen Smalley @ 2019-09-16 18:42 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul
In-Reply-To: <20190829232935.7099-3-casey@schaufler-ca.com>
On 8/29/19 7:29 PM, Casey Schaufler wrote:
> Move management of the sock->sk_security blob out
> of the individual security modules and into the security
> infrastructure. Instead of allocating the blobs from within
> the modules the modules tell the infrastructure how much
> space is required, and the space is allocated there.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: John Johansen <john.johansen@canonical.com>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
One oddity noted below, but it isn't introduced by this patch so you can
add my:
Reviewed-by: Stephen Smalley <sds@tycho.nsa.gov>
> ---
> include/linux/lsm_hooks.h | 1 +
> security/apparmor/include/net.h | 6 ++-
> security/apparmor/lsm.c | 38 ++++-----------
> security/security.c | 36 +++++++++++++-
> security/selinux/hooks.c | 78 +++++++++++++++----------------
> security/selinux/include/objsec.h | 5 ++
> security/selinux/netlabel.c | 23 ++++-----
> security/smack/smack.h | 5 ++
> security/smack/smack_lsm.c | 64 ++++++++++++-------------
> security/smack/smack_netfilter.c | 8 ++--
> 10 files changed, 144 insertions(+), 120 deletions(-)
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index f9222a04968d..b353482ea348 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -2047,6 +2047,7 @@ struct lsm_blob_sizes {
> int lbs_cred;
> int lbs_file;
> int lbs_inode;
> + int lbs_sock;
> int lbs_superblock;
> int lbs_ipc;
> int lbs_msg_msg;
> diff --git a/security/apparmor/include/net.h b/security/apparmor/include/net.h
> index 7334ac966d01..adac04e3b3cc 100644
> --- a/security/apparmor/include/net.h
> +++ b/security/apparmor/include/net.h
> @@ -55,7 +55,11 @@ struct aa_sk_ctx {
> struct aa_label *peer;
> };
>
> -#define SK_CTX(X) ((X)->sk_security)
> +static inline struct aa_sk_ctx *aa_sock(const struct sock *sk)
> +{
> + return sk->sk_security + apparmor_blob_sizes.lbs_sock;
> +}
> +
> #define SOCK_ctx(X) SOCK_INODE(X)->i_security
This use of i_security looks suspicious, but SOCK_ctx doesn't appear to
be used presently. Probably should be removed in a separate patch.
> #define DEFINE_AUDIT_NET(NAME, OP, SK, F, T, P) \
> struct lsm_network_audit NAME ## _net = { .sk = (SK), \
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index 49d664ddff44..2716e7731279 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -757,33 +757,15 @@ static int apparmor_task_kill(struct task_struct *target, struct kernel_siginfo
> return error;
> }
>
> -/**
> - * apparmor_sk_alloc_security - allocate and attach the sk_security field
> - */
> -static int apparmor_sk_alloc_security(struct sock *sk, int family, gfp_t flags)
> -{
> - struct aa_sk_ctx *ctx;
> -
> - ctx = kzalloc(sizeof(*ctx), flags);
> - if (!ctx)
> - return -ENOMEM;
> -
> - SK_CTX(sk) = ctx;
> -
> - return 0;
> -}
> -
> /**
> * apparmor_sk_free_security - free the sk_security field
> */
> static void apparmor_sk_free_security(struct sock *sk)
> {
> - struct aa_sk_ctx *ctx = SK_CTX(sk);
> + struct aa_sk_ctx *ctx = aa_sock(sk);
>
> - SK_CTX(sk) = NULL;
> aa_put_label(ctx->label);
> aa_put_label(ctx->peer);
> - kfree(ctx);
> }
>
> /**
> @@ -792,8 +774,8 @@ static void apparmor_sk_free_security(struct sock *sk)
> static void apparmor_sk_clone_security(const struct sock *sk,
> struct sock *newsk)
> {
> - struct aa_sk_ctx *ctx = SK_CTX(sk);
> - struct aa_sk_ctx *new = SK_CTX(newsk);
> + struct aa_sk_ctx *ctx = aa_sock(sk);
> + struct aa_sk_ctx *new = aa_sock(newsk);
>
> new->label = aa_get_label(ctx->label);
> new->peer = aa_get_label(ctx->peer);
> @@ -844,7 +826,7 @@ static int apparmor_socket_post_create(struct socket *sock, int family,
> label = aa_get_current_label();
>
> if (sock->sk) {
> - struct aa_sk_ctx *ctx = SK_CTX(sock->sk);
> + struct aa_sk_ctx *ctx = aa_sock(sock->sk);
>
> aa_put_label(ctx->label);
> ctx->label = aa_get_label(label);
> @@ -1029,7 +1011,7 @@ static int apparmor_socket_shutdown(struct socket *sock, int how)
> */
> static int apparmor_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb)
> {
> - struct aa_sk_ctx *ctx = SK_CTX(sk);
> + struct aa_sk_ctx *ctx = aa_sock(sk);
>
> if (!skb->secmark)
> return 0;
> @@ -1042,7 +1024,7 @@ static int apparmor_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb)
>
> static struct aa_label *sk_peer_label(struct sock *sk)
> {
> - struct aa_sk_ctx *ctx = SK_CTX(sk);
> + struct aa_sk_ctx *ctx = aa_sock(sk);
>
> if (ctx->peer)
> return ctx->peer;
> @@ -1126,7 +1108,7 @@ static int apparmor_socket_getpeersec_dgram(struct socket *sock,
> */
> static void apparmor_sock_graft(struct sock *sk, struct socket *parent)
> {
> - struct aa_sk_ctx *ctx = SK_CTX(sk);
> + struct aa_sk_ctx *ctx = aa_sock(sk);
>
> if (!ctx->label)
> ctx->label = aa_get_current_label();
> @@ -1136,7 +1118,7 @@ static void apparmor_sock_graft(struct sock *sk, struct socket *parent)
> static int apparmor_inet_conn_request(struct sock *sk, struct sk_buff *skb,
> struct request_sock *req)
> {
> - struct aa_sk_ctx *ctx = SK_CTX(sk);
> + struct aa_sk_ctx *ctx = aa_sock(sk);
>
> if (!skb->secmark)
> return 0;
> @@ -1153,6 +1135,7 @@ struct lsm_blob_sizes apparmor_blob_sizes __lsm_ro_after_init = {
> .lbs_cred = sizeof(struct aa_task_ctx *),
> .lbs_file = sizeof(struct aa_file_ctx),
> .lbs_task = sizeof(struct aa_task_ctx),
> + .lbs_sock = sizeof(struct aa_sk_ctx),
> };
>
> static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = {
> @@ -1189,7 +1172,6 @@ static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = {
> LSM_HOOK_INIT(getprocattr, apparmor_getprocattr),
> LSM_HOOK_INIT(setprocattr, apparmor_setprocattr),
>
> - LSM_HOOK_INIT(sk_alloc_security, apparmor_sk_alloc_security),
> LSM_HOOK_INIT(sk_free_security, apparmor_sk_free_security),
> LSM_HOOK_INIT(sk_clone_security, apparmor_sk_clone_security),
>
> @@ -1581,7 +1563,7 @@ static unsigned int apparmor_ip_postroute(void *priv,
> if (sk == NULL)
> return NF_ACCEPT;
>
> - ctx = SK_CTX(sk);
> + ctx = aa_sock(sk);
> if (!apparmor_secmark_check(ctx->label, OP_SENDMSG, AA_MAY_SEND,
> skb->secmark, sk))
> return NF_ACCEPT;
> diff --git a/security/security.c b/security/security.c
> index 86198e303203..2c0834db7976 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -32,6 +32,7 @@
> #include <linux/string.h>
> #include <linux/msg.h>
> #include <net/flow.h>
> +#include <net/sock.h>
>
> #define MAX_LSM_EVM_XATTR 2
>
> @@ -172,6 +173,7 @@ static void __init lsm_set_blob_sizes(struct lsm_blob_sizes *needed)
> lsm_set_blob_size(&needed->lbs_inode, &blob_sizes.lbs_inode);
> lsm_set_blob_size(&needed->lbs_ipc, &blob_sizes.lbs_ipc);
> lsm_set_blob_size(&needed->lbs_msg_msg, &blob_sizes.lbs_msg_msg);
> + lsm_set_blob_size(&needed->lbs_sock, &blob_sizes.lbs_sock);
> lsm_set_blob_size(&needed->lbs_superblock, &blob_sizes.lbs_superblock);
> lsm_set_blob_size(&needed->lbs_task, &blob_sizes.lbs_task);
> }
> @@ -306,6 +308,7 @@ static void __init ordered_lsm_init(void)
> init_debug("inode blob size = %d\n", blob_sizes.lbs_inode);
> init_debug("ipc blob size = %d\n", blob_sizes.lbs_ipc);
> init_debug("msg_msg blob size = %d\n", blob_sizes.lbs_msg_msg);
> + init_debug("sock blob size = %d\n", blob_sizes.lbs_sock);
> init_debug("superblock blob size = %d\n", blob_sizes.lbs_superblock);
> init_debug("task blob size = %d\n", blob_sizes.lbs_task);
>
> @@ -605,6 +608,28 @@ static void __init lsm_early_task(struct task_struct *task)
> panic("%s: Early task alloc failed.\n", __func__);
> }
>
> +/**
> + * lsm_sock_alloc - allocate a composite sock blob
> + * @sock: the sock that needs a blob
> + * @priority: allocation mode
> + *
> + * Allocate the sock blob for all the modules
> + *
> + * Returns 0, or -ENOMEM if memory can't be allocated.
> + */
> +static int lsm_sock_alloc(struct sock *sock, gfp_t priority)
> +{
> + if (blob_sizes.lbs_sock == 0) {
> + sock->sk_security = NULL;
> + return 0;
> + }
> +
> + sock->sk_security = kzalloc(blob_sizes.lbs_sock, priority);
> + if (sock->sk_security == NULL)
> + return -ENOMEM;
> + return 0;
> +}
> +
> /**
> * lsm_superblock_alloc - allocate a composite superblock blob
> * @sb: the superblock that needs a blob
> @@ -2048,12 +2073,21 @@ EXPORT_SYMBOL(security_socket_getpeersec_dgram);
>
> int security_sk_alloc(struct sock *sk, int family, gfp_t priority)
> {
> - return call_int_hook(sk_alloc_security, 0, sk, family, priority);
> + int rc = lsm_sock_alloc(sk, priority);
> +
> + if (unlikely(rc))
> + return rc;
> + rc = call_int_hook(sk_alloc_security, 0, sk, family, priority);
> + if (unlikely(rc))
> + security_sk_free(sk);
> + return rc;
> }
>
> void security_sk_free(struct sock *sk)
> {
> call_void_hook(sk_free_security, sk);
> + kfree(sk->sk_security);
> + sk->sk_security = NULL;
> }
>
> void security_sk_clone(const struct sock *sk, struct sock *newsk)
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 7478d8eda00a..5d74ed35b728 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -4319,7 +4319,7 @@ static int socket_sockcreate_sid(const struct task_security_struct *tsec,
>
> static int sock_has_perm(struct sock *sk, u32 perms)
> {
> - struct sk_security_struct *sksec = sk->sk_security;
> + struct sk_security_struct *sksec = selinux_sock(sk);
> struct common_audit_data ad;
> struct lsm_network_audit net = {0,};
>
> @@ -4376,7 +4376,7 @@ static int selinux_socket_post_create(struct socket *sock, int family,
> isec->initialized = LABEL_INITIALIZED;
>
> if (sock->sk) {
> - sksec = sock->sk->sk_security;
> + sksec = selinux_sock(sock->sk);
> sksec->sclass = sclass;
> sksec->sid = sid;
> /* Allows detection of the first association on this socket */
> @@ -4392,8 +4392,8 @@ static int selinux_socket_post_create(struct socket *sock, int family,
> static int selinux_socket_socketpair(struct socket *socka,
> struct socket *sockb)
> {
> - struct sk_security_struct *sksec_a = socka->sk->sk_security;
> - struct sk_security_struct *sksec_b = sockb->sk->sk_security;
> + struct sk_security_struct *sksec_a = selinux_sock(socka->sk);
> + struct sk_security_struct *sksec_b = selinux_sock(sockb->sk);
>
> sksec_a->peer_sid = sksec_b->sid;
> sksec_b->peer_sid = sksec_a->sid;
> @@ -4408,7 +4408,7 @@ static int selinux_socket_socketpair(struct socket *socka,
> static int selinux_socket_bind(struct socket *sock, struct sockaddr *address, int addrlen)
> {
> struct sock *sk = sock->sk;
> - struct sk_security_struct *sksec = sk->sk_security;
> + struct sk_security_struct *sksec = selinux_sock(sk);
> u16 family;
> int err;
>
> @@ -4540,7 +4540,7 @@ static int selinux_socket_connect_helper(struct socket *sock,
> struct sockaddr *address, int addrlen)
> {
> struct sock *sk = sock->sk;
> - struct sk_security_struct *sksec = sk->sk_security;
> + struct sk_security_struct *sksec = selinux_sock(sk);
> int err;
>
> err = sock_has_perm(sk, SOCKET__CONNECT);
> @@ -4711,9 +4711,9 @@ static int selinux_socket_unix_stream_connect(struct sock *sock,
> struct sock *other,
> struct sock *newsk)
> {
> - struct sk_security_struct *sksec_sock = sock->sk_security;
> - struct sk_security_struct *sksec_other = other->sk_security;
> - struct sk_security_struct *sksec_new = newsk->sk_security;
> + struct sk_security_struct *sksec_sock = selinux_sock(sock);
> + struct sk_security_struct *sksec_other = selinux_sock(other);
> + struct sk_security_struct *sksec_new = selinux_sock(newsk);
> struct common_audit_data ad;
> struct lsm_network_audit net = {0,};
> int err;
> @@ -4745,8 +4745,8 @@ static int selinux_socket_unix_stream_connect(struct sock *sock,
> static int selinux_socket_unix_may_send(struct socket *sock,
> struct socket *other)
> {
> - struct sk_security_struct *ssec = sock->sk->sk_security;
> - struct sk_security_struct *osec = other->sk->sk_security;
> + struct sk_security_struct *ssec = selinux_sock(sock->sk);
> + struct sk_security_struct *osec = selinux_sock(other->sk);
> struct common_audit_data ad;
> struct lsm_network_audit net = {0,};
>
> @@ -4788,7 +4788,7 @@ static int selinux_sock_rcv_skb_compat(struct sock *sk, struct sk_buff *skb,
> u16 family)
> {
> int err = 0;
> - struct sk_security_struct *sksec = sk->sk_security;
> + struct sk_security_struct *sksec = selinux_sock(sk);
> u32 sk_sid = sksec->sid;
> struct common_audit_data ad;
> struct lsm_network_audit net = {0,};
> @@ -4821,7 +4821,7 @@ static int selinux_sock_rcv_skb_compat(struct sock *sk, struct sk_buff *skb,
> static int selinux_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb)
> {
> int err;
> - struct sk_security_struct *sksec = sk->sk_security;
> + struct sk_security_struct *sksec = selinux_sock(sk);
> u16 family = sk->sk_family;
> u32 sk_sid = sksec->sid;
> struct common_audit_data ad;
> @@ -4889,13 +4889,15 @@ static int selinux_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb)
> return err;
> }
>
> -static int selinux_socket_getpeersec_stream(struct socket *sock, char __user *optval,
> - int __user *optlen, unsigned len)
> +static int selinux_socket_getpeersec_stream(struct socket *sock,
> + char __user *optval,
> + int __user *optlen,
> + unsigned int len)
> {
> int err = 0;
> char *scontext;
> u32 scontext_len;
> - struct sk_security_struct *sksec = sock->sk->sk_security;
> + struct sk_security_struct *sksec = selinux_sock(sock->sk);
> u32 peer_sid = SECSID_NULL;
>
> if (sksec->sclass == SECCLASS_UNIX_STREAM_SOCKET ||
> @@ -4955,34 +4957,27 @@ static int selinux_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *
>
> static int selinux_sk_alloc_security(struct sock *sk, int family, gfp_t priority)
> {
> - struct sk_security_struct *sksec;
> -
> - sksec = kzalloc(sizeof(*sksec), priority);
> - if (!sksec)
> - return -ENOMEM;
> + struct sk_security_struct *sksec = selinux_sock(sk);
>
> sksec->peer_sid = SECINITSID_UNLABELED;
> sksec->sid = SECINITSID_UNLABELED;
> sksec->sclass = SECCLASS_SOCKET;
> selinux_netlbl_sk_security_reset(sksec);
> - sk->sk_security = sksec;
>
> return 0;
> }
>
> static void selinux_sk_free_security(struct sock *sk)
> {
> - struct sk_security_struct *sksec = sk->sk_security;
> + struct sk_security_struct *sksec = selinux_sock(sk);
>
> - sk->sk_security = NULL;
> selinux_netlbl_sk_security_free(sksec);
> - kfree(sksec);
> }
>
> static void selinux_sk_clone_security(const struct sock *sk, struct sock *newsk)
> {
> - struct sk_security_struct *sksec = sk->sk_security;
> - struct sk_security_struct *newsksec = newsk->sk_security;
> + struct sk_security_struct *sksec = selinux_sock(sk);
> + struct sk_security_struct *newsksec = selinux_sock(newsk);
>
> newsksec->sid = sksec->sid;
> newsksec->peer_sid = sksec->peer_sid;
> @@ -4996,7 +4991,7 @@ static void selinux_sk_getsecid(struct sock *sk, u32 *secid)
> if (!sk)
> *secid = SECINITSID_ANY_SOCKET;
> else {
> - struct sk_security_struct *sksec = sk->sk_security;
> + struct sk_security_struct *sksec = selinux_sock(sk);
>
> *secid = sksec->sid;
> }
> @@ -5006,7 +5001,7 @@ static void selinux_sock_graft(struct sock *sk, struct socket *parent)
> {
> struct inode_security_struct *isec =
> inode_security_novalidate(SOCK_INODE(parent));
> - struct sk_security_struct *sksec = sk->sk_security;
> + struct sk_security_struct *sksec = selinux_sock(sk);
>
> if (sk->sk_family == PF_INET || sk->sk_family == PF_INET6 ||
> sk->sk_family == PF_UNIX)
> @@ -5021,7 +5016,7 @@ static void selinux_sock_graft(struct sock *sk, struct socket *parent)
> static int selinux_sctp_assoc_request(struct sctp_endpoint *ep,
> struct sk_buff *skb)
> {
> - struct sk_security_struct *sksec = ep->base.sk->sk_security;
> + struct sk_security_struct *sksec = selinux_sock(ep->base.sk);
> struct common_audit_data ad;
> struct lsm_network_audit net = {0,};
> u8 peerlbl_active;
> @@ -5172,8 +5167,8 @@ static int selinux_sctp_bind_connect(struct sock *sk, int optname,
> static void selinux_sctp_sk_clone(struct sctp_endpoint *ep, struct sock *sk,
> struct sock *newsk)
> {
> - struct sk_security_struct *sksec = sk->sk_security;
> - struct sk_security_struct *newsksec = newsk->sk_security;
> + struct sk_security_struct *sksec = selinux_sock(sk);
> + struct sk_security_struct *newsksec = selinux_sock(newsk);
>
> /* If policy does not support SECCLASS_SCTP_SOCKET then call
> * the non-sctp clone version.
> @@ -5190,7 +5185,7 @@ static void selinux_sctp_sk_clone(struct sctp_endpoint *ep, struct sock *sk,
> static int selinux_inet_conn_request(struct sock *sk, struct sk_buff *skb,
> struct request_sock *req)
> {
> - struct sk_security_struct *sksec = sk->sk_security;
> + struct sk_security_struct *sksec = selinux_sock(sk);
> int err;
> u16 family = req->rsk_ops->family;
> u32 connsid;
> @@ -5211,7 +5206,7 @@ static int selinux_inet_conn_request(struct sock *sk, struct sk_buff *skb,
> static void selinux_inet_csk_clone(struct sock *newsk,
> const struct request_sock *req)
> {
> - struct sk_security_struct *newsksec = newsk->sk_security;
> + struct sk_security_struct *newsksec = selinux_sock(newsk);
>
> newsksec->sid = req->secid;
> newsksec->peer_sid = req->peer_secid;
> @@ -5228,7 +5223,7 @@ static void selinux_inet_csk_clone(struct sock *newsk,
> static void selinux_inet_conn_established(struct sock *sk, struct sk_buff *skb)
> {
> u16 family = sk->sk_family;
> - struct sk_security_struct *sksec = sk->sk_security;
> + struct sk_security_struct *sksec = selinux_sock(sk);
>
> /* handle mapped IPv4 packets arriving via IPv6 sockets */
> if (family == PF_INET6 && skb->protocol == htons(ETH_P_IP))
> @@ -5312,7 +5307,7 @@ static int selinux_tun_dev_attach_queue(void *security)
> static int selinux_tun_dev_attach(struct sock *sk, void *security)
> {
> struct tun_security_struct *tunsec = security;
> - struct sk_security_struct *sksec = sk->sk_security;
> + struct sk_security_struct *sksec = selinux_sock(sk);
>
> /* we don't currently perform any NetLabel based labeling here and it
> * isn't clear that we would want to do so anyway; while we could apply
> @@ -5353,7 +5348,7 @@ static int selinux_nlmsg_perm(struct sock *sk, struct sk_buff *skb)
> int err = 0;
> u32 perm;
> struct nlmsghdr *nlh;
> - struct sk_security_struct *sksec = sk->sk_security;
> + struct sk_security_struct *sksec = selinux_sock(sk);
>
> if (skb->len < NLMSG_HDRLEN) {
> err = -EINVAL;
> @@ -5494,7 +5489,7 @@ static unsigned int selinux_ip_output(struct sk_buff *skb,
> return NF_ACCEPT;
>
> /* standard practice, label using the parent socket */
> - sksec = sk->sk_security;
> + sksec = selinux_sock(sk);
> sid = sksec->sid;
> } else
> sid = SECINITSID_KERNEL;
> @@ -5533,7 +5528,7 @@ static unsigned int selinux_ip_postroute_compat(struct sk_buff *skb,
>
> if (sk == NULL)
> return NF_ACCEPT;
> - sksec = sk->sk_security;
> + sksec = selinux_sock(sk);
>
> ad.type = LSM_AUDIT_DATA_NET;
> ad.u.net = &net;
> @@ -5625,7 +5620,7 @@ static unsigned int selinux_ip_postroute(struct sk_buff *skb,
> u32 skb_sid;
> struct sk_security_struct *sksec;
>
> - sksec = sk->sk_security;
> + sksec = selinux_sock(sk);
> if (selinux_skb_peerlbl_sid(skb, family, &skb_sid))
> return NF_DROP;
> /* At this point, if the returned skb peerlbl is SECSID_NULL
> @@ -5654,7 +5649,7 @@ static unsigned int selinux_ip_postroute(struct sk_buff *skb,
> } else {
> /* Locally generated packet, fetch the security label from the
> * associated socket. */
> - struct sk_security_struct *sksec = sk->sk_security;
> + struct sk_security_struct *sksec = selinux_sock(sk);
> peer_sid = sksec->sid;
> secmark_perm = PACKET__SEND;
> }
> @@ -6633,6 +6628,7 @@ struct lsm_blob_sizes selinux_blob_sizes __lsm_ro_after_init = {
> .lbs_inode = sizeof(struct inode_security_struct),
> .lbs_ipc = sizeof(struct ipc_security_struct),
> .lbs_msg_msg = sizeof(struct msg_security_struct),
> + .lbs_sock = sizeof(struct sk_security_struct),
> .lbs_superblock = sizeof(struct superblock_security_struct),
> };
>
> diff --git a/security/selinux/include/objsec.h b/security/selinux/include/objsec.h
> index d08d7e5d2f93..29f02b8f8f31 100644
> --- a/security/selinux/include/objsec.h
> +++ b/security/selinux/include/objsec.h
> @@ -194,4 +194,9 @@ static inline struct superblock_security_struct *selinux_superblock(
> return superblock->s_security + selinux_blob_sizes.lbs_superblock;
> }
>
> +static inline struct sk_security_struct *selinux_sock(const struct sock *sock)
> +{
> + return sock->sk_security + selinux_blob_sizes.lbs_sock;
> +}
> +
> #endif /* _SELINUX_OBJSEC_H_ */
> diff --git a/security/selinux/netlabel.c b/security/selinux/netlabel.c
> index 186e727b737b..c40914a157b7 100644
> --- a/security/selinux/netlabel.c
> +++ b/security/selinux/netlabel.c
> @@ -31,6 +31,7 @@
> #include <linux/gfp.h>
> #include <linux/ip.h>
> #include <linux/ipv6.h>
> +#include <linux/lsm_hooks.h>
> #include <net/sock.h>
> #include <net/netlabel.h>
> #include <net/ip.h>
> @@ -81,7 +82,7 @@ static int selinux_netlbl_sidlookup_cached(struct sk_buff *skb,
> static struct netlbl_lsm_secattr *selinux_netlbl_sock_genattr(struct sock *sk)
> {
> int rc;
> - struct sk_security_struct *sksec = sk->sk_security;
> + struct sk_security_struct *sksec = selinux_sock(sk);
> struct netlbl_lsm_secattr *secattr;
>
> if (sksec->nlbl_secattr != NULL)
> @@ -114,7 +115,7 @@ static struct netlbl_lsm_secattr *selinux_netlbl_sock_getattr(
> const struct sock *sk,
> u32 sid)
> {
> - struct sk_security_struct *sksec = sk->sk_security;
> + struct sk_security_struct *sksec = selinux_sock(sk);
> struct netlbl_lsm_secattr *secattr = sksec->nlbl_secattr;
>
> if (secattr == NULL)
> @@ -249,7 +250,7 @@ int selinux_netlbl_skbuff_setsid(struct sk_buff *skb,
> * being labeled by it's parent socket, if it is just exit */
> sk = skb_to_full_sk(skb);
> if (sk != NULL) {
> - struct sk_security_struct *sksec = sk->sk_security;
> + struct sk_security_struct *sksec = selinux_sock(sk);
>
> if (sksec->nlbl_state != NLBL_REQSKB)
> return 0;
> @@ -287,7 +288,7 @@ int selinux_netlbl_sctp_assoc_request(struct sctp_endpoint *ep,
> {
> int rc;
> struct netlbl_lsm_secattr secattr;
> - struct sk_security_struct *sksec = ep->base.sk->sk_security;
> + struct sk_security_struct *sksec = selinux_sock(ep->base.sk);
> struct sockaddr *addr;
> struct sockaddr_in addr4;
> #if IS_ENABLED(CONFIG_IPV6)
> @@ -370,7 +371,7 @@ int selinux_netlbl_inet_conn_request(struct request_sock *req, u16 family)
> */
> void selinux_netlbl_inet_csk_clone(struct sock *sk, u16 family)
> {
> - struct sk_security_struct *sksec = sk->sk_security;
> + struct sk_security_struct *sksec = selinux_sock(sk);
>
> if (family == PF_INET)
> sksec->nlbl_state = NLBL_LABELED;
> @@ -388,8 +389,8 @@ void selinux_netlbl_inet_csk_clone(struct sock *sk, u16 family)
> */
> void selinux_netlbl_sctp_sk_clone(struct sock *sk, struct sock *newsk)
> {
> - struct sk_security_struct *sksec = sk->sk_security;
> - struct sk_security_struct *newsksec = newsk->sk_security;
> + struct sk_security_struct *sksec = selinux_sock(sk);
> + struct sk_security_struct *newsksec = selinux_sock(newsk);
>
> newsksec->nlbl_state = sksec->nlbl_state;
> }
> @@ -407,7 +408,7 @@ void selinux_netlbl_sctp_sk_clone(struct sock *sk, struct sock *newsk)
> int selinux_netlbl_socket_post_create(struct sock *sk, u16 family)
> {
> int rc;
> - struct sk_security_struct *sksec = sk->sk_security;
> + struct sk_security_struct *sksec = selinux_sock(sk);
> struct netlbl_lsm_secattr *secattr;
>
> if (family != PF_INET && family != PF_INET6)
> @@ -522,7 +523,7 @@ int selinux_netlbl_socket_setsockopt(struct socket *sock,
> {
> int rc = 0;
> struct sock *sk = sock->sk;
> - struct sk_security_struct *sksec = sk->sk_security;
> + struct sk_security_struct *sksec = selinux_sock(sk);
> struct netlbl_lsm_secattr secattr;
>
> if (selinux_netlbl_option(level, optname) &&
> @@ -560,7 +561,7 @@ static int selinux_netlbl_socket_connect_helper(struct sock *sk,
> struct sockaddr *addr)
> {
> int rc;
> - struct sk_security_struct *sksec = sk->sk_security;
> + struct sk_security_struct *sksec = selinux_sock(sk);
> struct netlbl_lsm_secattr *secattr;
>
> /* connected sockets are allowed to disconnect when the address family
> @@ -599,7 +600,7 @@ static int selinux_netlbl_socket_connect_helper(struct sock *sk,
> int selinux_netlbl_socket_connect_locked(struct sock *sk,
> struct sockaddr *addr)
> {
> - struct sk_security_struct *sksec = sk->sk_security;
> + struct sk_security_struct *sksec = selinux_sock(sk);
>
> if (sksec->nlbl_state != NLBL_REQSKB &&
> sksec->nlbl_state != NLBL_CONNLABELED)
> diff --git a/security/smack/smack.h b/security/smack/smack.h
> index caecbcba9942..4ac4bf3310d7 100644
> --- a/security/smack/smack.h
> +++ b/security/smack/smack.h
> @@ -375,6 +375,11 @@ static inline struct smack_known **smack_ipc(const struct kern_ipc_perm *ipc)
> return ipc->security + smack_blob_sizes.lbs_ipc;
> }
>
> +static inline struct socket_smack *smack_sock(const struct sock *sock)
> +{
> + return sock->sk_security + smack_blob_sizes.lbs_sock;
> +}
> +
> static inline struct superblock_smack *smack_superblock(
> const struct super_block *superblock)
> {
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 807eff2ccce9..fd69e1bd841b 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -1439,7 +1439,7 @@ static int smack_inode_getsecurity(struct inode *inode,
> if (sock == NULL || sock->sk == NULL)
> return -EOPNOTSUPP;
>
> - ssp = sock->sk->sk_security;
> + ssp = smack_sock(sock->sk);
>
> if (strcmp(name, XATTR_SMACK_IPIN) == 0)
> isp = ssp->smk_in;
> @@ -1821,7 +1821,7 @@ static int smack_file_receive(struct file *file)
>
> if (inode->i_sb->s_magic == SOCKFS_MAGIC) {
> sock = SOCKET_I(inode);
> - ssp = sock->sk->sk_security;
> + ssp = smack_sock(sock->sk);
> tsp = smack_cred(current_cred());
> /*
> * If the receiving process can't write to the
> @@ -2231,11 +2231,7 @@ static void smack_task_to_inode(struct task_struct *p, struct inode *inode)
> static int smack_sk_alloc_security(struct sock *sk, int family, gfp_t gfp_flags)
> {
> struct smack_known *skp = smk_of_current();
> - struct socket_smack *ssp;
> -
> - ssp = kzalloc(sizeof(struct socket_smack), gfp_flags);
> - if (ssp == NULL)
> - return -ENOMEM;
> + struct socket_smack *ssp = smack_sock(sk);
>
> /*
> * Sockets created by kernel threads receive web label.
> @@ -2249,11 +2245,10 @@ static int smack_sk_alloc_security(struct sock *sk, int family, gfp_t gfp_flags)
> }
> ssp->smk_packet = NULL;
>
> - sk->sk_security = ssp;
> -
> return 0;
> }
>
> +#ifdef SMACK_IPV6_PORT_LABELING
> /**
> * smack_sk_free_security - Free a socket blob
> * @sk: the socket
> @@ -2262,7 +2257,6 @@ static int smack_sk_alloc_security(struct sock *sk, int family, gfp_t gfp_flags)
> */
> static void smack_sk_free_security(struct sock *sk)
> {
> -#ifdef SMACK_IPV6_PORT_LABELING
> struct smk_port_label *spp;
>
> if (sk->sk_family == PF_INET6) {
> @@ -2275,9 +2269,8 @@ static void smack_sk_free_security(struct sock *sk)
> }
> rcu_read_unlock();
> }
> -#endif
> - kfree(sk->sk_security);
> }
> +#endif
>
> /**
> * smack_ipv4host_label - check host based restrictions
> @@ -2395,7 +2388,7 @@ static struct smack_known *smack_ipv6host_label(struct sockaddr_in6 *sip)
> static int smack_netlabel(struct sock *sk, int labeled)
> {
> struct smack_known *skp;
> - struct socket_smack *ssp = sk->sk_security;
> + struct socket_smack *ssp = smack_sock(sk);
> int rc = 0;
>
> /*
> @@ -2440,7 +2433,7 @@ static int smack_netlabel_send(struct sock *sk, struct sockaddr_in *sap)
> int rc;
> int sk_lbl;
> struct smack_known *hkp;
> - struct socket_smack *ssp = sk->sk_security;
> + struct socket_smack *ssp = smack_sock(sk);
> struct smk_audit_info ad;
>
> rcu_read_lock();
> @@ -2516,7 +2509,7 @@ static void smk_ipv6_port_label(struct socket *sock, struct sockaddr *address)
> {
> struct sock *sk = sock->sk;
> struct sockaddr_in6 *addr6;
> - struct socket_smack *ssp = sock->sk->sk_security;
> + struct socket_smack *ssp = smack_sock(sock->sk);
> struct smk_port_label *spp;
> unsigned short port = 0;
>
> @@ -2603,7 +2596,7 @@ static int smk_ipv6_port_check(struct sock *sk, struct sockaddr_in6 *address,
> int act)
> {
> struct smk_port_label *spp;
> - struct socket_smack *ssp = sk->sk_security;
> + struct socket_smack *ssp = smack_sock(sk);
> struct smack_known *skp = NULL;
> unsigned short port;
> struct smack_known *object;
> @@ -2697,7 +2690,7 @@ static int smack_inode_setsecurity(struct inode *inode, const char *name,
> if (sock == NULL || sock->sk == NULL)
> return -EOPNOTSUPP;
>
> - ssp = sock->sk->sk_security;
> + ssp = smack_sock(sock->sk);
>
> if (strcmp(name, XATTR_SMACK_IPIN) == 0)
> ssp->smk_in = skp;
> @@ -2745,7 +2738,7 @@ static int smack_socket_post_create(struct socket *sock, int family,
> * Sockets created by kernel threads receive web label.
> */
> if (unlikely(current->flags & PF_KTHREAD)) {
> - ssp = sock->sk->sk_security;
> + ssp = smack_sock(sock->sk);
> ssp->smk_in = &smack_known_web;
> ssp->smk_out = &smack_known_web;
> }
> @@ -2770,8 +2763,8 @@ static int smack_socket_post_create(struct socket *sock, int family,
> static int smack_socket_socketpair(struct socket *socka,
> struct socket *sockb)
> {
> - struct socket_smack *asp = socka->sk->sk_security;
> - struct socket_smack *bsp = sockb->sk->sk_security;
> + struct socket_smack *asp = smack_sock(socka->sk);
> + struct socket_smack *bsp = smack_sock(sockb->sk);
>
> asp->smk_packet = bsp->smk_out;
> bsp->smk_packet = asp->smk_out;
> @@ -2825,7 +2818,7 @@ static int smack_socket_connect(struct socket *sock, struct sockaddr *sap,
> return 0;
>
> #ifdef SMACK_IPV6_SECMARK_LABELING
> - ssp = sock->sk->sk_security;
> + ssp = smack_sock(sock->sk);
> #endif
>
> switch (sock->sk->sk_family) {
> @@ -3566,9 +3559,9 @@ static int smack_unix_stream_connect(struct sock *sock,
> {
> struct smack_known *skp;
> struct smack_known *okp;
> - struct socket_smack *ssp = sock->sk_security;
> - struct socket_smack *osp = other->sk_security;
> - struct socket_smack *nsp = newsk->sk_security;
> + struct socket_smack *ssp = smack_sock(sock);
> + struct socket_smack *osp = smack_sock(other);
> + struct socket_smack *nsp = smack_sock(newsk);
> struct smk_audit_info ad;
> int rc = 0;
> #ifdef CONFIG_AUDIT
> @@ -3614,8 +3607,8 @@ static int smack_unix_stream_connect(struct sock *sock,
> */
> static int smack_unix_may_send(struct socket *sock, struct socket *other)
> {
> - struct socket_smack *ssp = sock->sk->sk_security;
> - struct socket_smack *osp = other->sk->sk_security;
> + struct socket_smack *ssp = smack_sock(sock->sk);
> + struct socket_smack *osp = smack_sock(other->sk);
> struct smk_audit_info ad;
> int rc;
>
> @@ -3652,7 +3645,7 @@ static int smack_socket_sendmsg(struct socket *sock, struct msghdr *msg,
> struct sockaddr_in6 *sap = (struct sockaddr_in6 *) msg->msg_name;
> #endif
> #ifdef SMACK_IPV6_SECMARK_LABELING
> - struct socket_smack *ssp = sock->sk->sk_security;
> + struct socket_smack *ssp = smack_sock(sock->sk);
> struct smack_known *rsp;
> #endif
> int rc = 0;
> @@ -3817,7 +3810,7 @@ static int smk_skb_to_addr_ipv6(struct sk_buff *skb, struct sockaddr_in6 *sip)
> static int smack_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb)
> {
> struct netlbl_lsm_secattr secattr;
> - struct socket_smack *ssp = sk->sk_security;
> + struct socket_smack *ssp = smack_sock(sk);
> struct smack_known *skp = NULL;
> int rc = 0;
> struct smk_audit_info ad;
> @@ -3934,7 +3927,7 @@ static int smack_socket_getpeersec_stream(struct socket *sock,
> int slen = 1;
> int rc = 0;
>
> - ssp = sock->sk->sk_security;
> + ssp = smack_sock(sock->sk);
> if (ssp->smk_packet != NULL) {
> rcp = ssp->smk_packet->smk_known;
> slen = strlen(rcp) + 1;
> @@ -3984,7 +3977,7 @@ static int smack_socket_getpeersec_dgram(struct socket *sock,
>
> switch (family) {
> case PF_UNIX:
> - ssp = sock->sk->sk_security;
> + ssp = smack_sock(sock->sk);
> s = ssp->smk_out->smk_secid;
> break;
> case PF_INET:
> @@ -3997,7 +3990,7 @@ static int smack_socket_getpeersec_dgram(struct socket *sock,
> * Translate what netlabel gave us.
> */
> if (sock != NULL && sock->sk != NULL)
> - ssp = sock->sk->sk_security;
> + ssp = smack_sock(sock->sk);
> netlbl_secattr_init(&secattr);
> rc = netlbl_skbuff_getattr(skb, family, &secattr);
> if (rc == 0) {
> @@ -4035,7 +4028,7 @@ static void smack_sock_graft(struct sock *sk, struct socket *parent)
> (sk->sk_family != PF_INET && sk->sk_family != PF_INET6))
> return;
>
> - ssp = sk->sk_security;
> + ssp = smack_sock(sk);
> ssp->smk_in = skp;
> ssp->smk_out = skp;
> /* cssp->smk_packet is already set in smack_inet_csk_clone() */
> @@ -4055,7 +4048,7 @@ static int smack_inet_conn_request(struct sock *sk, struct sk_buff *skb,
> {
> u16 family = sk->sk_family;
> struct smack_known *skp;
> - struct socket_smack *ssp = sk->sk_security;
> + struct socket_smack *ssp = smack_sock(sk);
> struct netlbl_lsm_secattr secattr;
> struct sockaddr_in addr;
> struct iphdr *hdr;
> @@ -4154,7 +4147,7 @@ static int smack_inet_conn_request(struct sock *sk, struct sk_buff *skb,
> static void smack_inet_csk_clone(struct sock *sk,
> const struct request_sock *req)
> {
> - struct socket_smack *ssp = sk->sk_security;
> + struct socket_smack *ssp = smack_sock(sk);
> struct smack_known *skp;
>
> if (req->peer_secid != 0) {
> @@ -4558,6 +4551,7 @@ struct lsm_blob_sizes smack_blob_sizes __lsm_ro_after_init = {
> .lbs_inode = sizeof(struct inode_smack),
> .lbs_ipc = sizeof(struct smack_known *),
> .lbs_msg_msg = sizeof(struct smack_known *),
> + .lbs_sock = sizeof(struct socket_smack),
> .lbs_superblock = sizeof(struct superblock_smack),
> };
>
> @@ -4667,7 +4661,9 @@ static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
> LSM_HOOK_INIT(socket_getpeersec_stream, smack_socket_getpeersec_stream),
> LSM_HOOK_INIT(socket_getpeersec_dgram, smack_socket_getpeersec_dgram),
> LSM_HOOK_INIT(sk_alloc_security, smack_sk_alloc_security),
> +#ifdef SMACK_IPV6_PORT_LABELING
> LSM_HOOK_INIT(sk_free_security, smack_sk_free_security),
> +#endif
> LSM_HOOK_INIT(sock_graft, smack_sock_graft),
> LSM_HOOK_INIT(inet_conn_request, smack_inet_conn_request),
> LSM_HOOK_INIT(inet_csk_clone, smack_inet_csk_clone),
> diff --git a/security/smack/smack_netfilter.c b/security/smack/smack_netfilter.c
> index e36d17835d4f..701a1cc1bdcc 100644
> --- a/security/smack/smack_netfilter.c
> +++ b/security/smack/smack_netfilter.c
> @@ -31,8 +31,8 @@ static unsigned int smack_ipv6_output(void *priv,
> struct socket_smack *ssp;
> struct smack_known *skp;
>
> - if (sk && sk->sk_security) {
> - ssp = sk->sk_security;
> + if (sk && smack_sock(sk)) {
> + ssp = smack_sock(sk);
> skp = ssp->smk_out;
> skb->secmark = skp->smk_secid;
> }
> @@ -49,8 +49,8 @@ static unsigned int smack_ipv4_output(void *priv,
> struct socket_smack *ssp;
> struct smack_known *skp;
>
> - if (sk && sk->sk_security) {
> - ssp = sk->sk_security;
> + if (sk && smack_sock(sk)) {
> + ssp = smack_sock(sk);
> skp = ssp->smk_out;
> skb->secmark = skp->smk_secid;
> }
>
^ permalink raw reply
* Re: [PATCH v8 01/28] LSM: Infrastructure management of the superblock
From: Stephen Smalley @ 2019-09-16 18:19 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul
In-Reply-To: <20190829232935.7099-2-casey@schaufler-ca.com>
On 8/29/19 7:29 PM, Casey Schaufler wrote:
> Move management of the superblock->sb_security blob out
> of the individual security modules and into the security
> infrastructure. Instead of allocating the blobs from within
> the modules the modules tell the infrastructure how much
> space is required, and the space is allocated there.
This doesn't appear to be needed to stack AA? That said, I have no
objections to converting all of the security blobs over to
infrastructure management just to be consistent.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: John Johansen <john.johansen@canonical.com>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
Reviewed-by: Stephen Smalley <sds@tycho.nsa.gov>
> ---
> include/linux/lsm_hooks.h | 1 +
> security/security.c | 46 ++++++++++++++++++++----
> security/selinux/hooks.c | 58 ++++++++++++-------------------
> security/selinux/include/objsec.h | 6 ++++
> security/selinux/ss/services.c | 3 +-
> security/smack/smack.h | 6 ++++
> security/smack/smack_lsm.c | 35 +++++--------------
> 7 files changed, 85 insertions(+), 70 deletions(-)
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index a240a3fc5fc4..f9222a04968d 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -2047,6 +2047,7 @@ struct lsm_blob_sizes {
> int lbs_cred;
> int lbs_file;
> int lbs_inode;
> + int lbs_superblock;
> int lbs_ipc;
> int lbs_msg_msg;
> int lbs_task;
> diff --git a/security/security.c b/security/security.c
> index 23cbb1a295a3..86198e303203 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -172,6 +172,7 @@ static void __init lsm_set_blob_sizes(struct lsm_blob_sizes *needed)
> lsm_set_blob_size(&needed->lbs_inode, &blob_sizes.lbs_inode);
> lsm_set_blob_size(&needed->lbs_ipc, &blob_sizes.lbs_ipc);
> lsm_set_blob_size(&needed->lbs_msg_msg, &blob_sizes.lbs_msg_msg);
> + lsm_set_blob_size(&needed->lbs_superblock, &blob_sizes.lbs_superblock);
> lsm_set_blob_size(&needed->lbs_task, &blob_sizes.lbs_task);
> }
>
> @@ -300,12 +301,13 @@ static void __init ordered_lsm_init(void)
> for (lsm = ordered_lsms; *lsm; lsm++)
> prepare_lsm(*lsm);
>
> - init_debug("cred blob size = %d\n", blob_sizes.lbs_cred);
> - init_debug("file blob size = %d\n", blob_sizes.lbs_file);
> - init_debug("inode blob size = %d\n", blob_sizes.lbs_inode);
> - init_debug("ipc blob size = %d\n", blob_sizes.lbs_ipc);
> - init_debug("msg_msg blob size = %d\n", blob_sizes.lbs_msg_msg);
> - init_debug("task blob size = %d\n", blob_sizes.lbs_task);
> + init_debug("cred blob size = %d\n", blob_sizes.lbs_cred);
> + init_debug("file blob size = %d\n", blob_sizes.lbs_file);
> + init_debug("inode blob size = %d\n", blob_sizes.lbs_inode);
> + init_debug("ipc blob size = %d\n", blob_sizes.lbs_ipc);
> + init_debug("msg_msg blob size = %d\n", blob_sizes.lbs_msg_msg);
> + init_debug("superblock blob size = %d\n", blob_sizes.lbs_superblock);
> + init_debug("task blob size = %d\n", blob_sizes.lbs_task);
>
> /*
> * Create any kmem_caches needed for blobs
> @@ -603,6 +605,27 @@ static void __init lsm_early_task(struct task_struct *task)
> panic("%s: Early task alloc failed.\n", __func__);
> }
>
> +/**
> + * lsm_superblock_alloc - allocate a composite superblock blob
> + * @sb: the superblock that needs a blob
> + *
> + * Allocate the superblock blob for all the modules
> + *
> + * Returns 0, or -ENOMEM if memory can't be allocated.
> + */
> +static int lsm_superblock_alloc(struct super_block *sb)
> +{
> + if (blob_sizes.lbs_superblock == 0) {
> + sb->s_security = NULL;
> + return 0;
> + }
> +
> + sb->s_security = kzalloc(blob_sizes.lbs_superblock, GFP_KERNEL);
> + if (sb->s_security == NULL)
> + return -ENOMEM;
> + return 0;
> +}
> +
> /*
> * Hook list operation macros.
> *
> @@ -776,12 +799,21 @@ int security_fs_context_parse_param(struct fs_context *fc, struct fs_parameter *
>
> int security_sb_alloc(struct super_block *sb)
> {
> - return call_int_hook(sb_alloc_security, 0, sb);
> + int rc = lsm_superblock_alloc(sb);
> +
> + if (unlikely(rc))
> + return rc;
> + rc = call_int_hook(sb_alloc_security, 0, sb);
> + if (unlikely(rc))
> + security_sb_free(sb);
> + return rc;
> }
>
> void security_sb_free(struct super_block *sb)
> {
> call_void_hook(sb_free_security, sb);
> + kfree(sb->s_security);
> + sb->s_security = NULL;
> }
>
> void security_free_mnt_opts(void **mnt_opts)
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 1d0b37af2444..7478d8eda00a 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -335,7 +335,7 @@ static void inode_free_security(struct inode *inode)
>
> if (!isec)
> return;
> - sbsec = inode->i_sb->s_security;
> + sbsec = selinux_superblock(inode->i_sb);
> /*
> * As not all inode security structures are in a list, we check for
> * empty list outside of the lock to make sure that we won't waste
> @@ -366,11 +366,7 @@ static int file_alloc_security(struct file *file)
>
> static int superblock_alloc_security(struct super_block *sb)
> {
> - struct superblock_security_struct *sbsec;
> -
> - sbsec = kzalloc(sizeof(struct superblock_security_struct), GFP_KERNEL);
> - if (!sbsec)
> - return -ENOMEM;
> + struct superblock_security_struct *sbsec = selinux_superblock(sb);
>
> mutex_init(&sbsec->lock);
> INIT_LIST_HEAD(&sbsec->isec_head);
> @@ -379,18 +375,10 @@ static int superblock_alloc_security(struct super_block *sb)
> sbsec->sid = SECINITSID_UNLABELED;
> sbsec->def_sid = SECINITSID_FILE;
> sbsec->mntpoint_sid = SECINITSID_UNLABELED;
> - sb->s_security = sbsec;
>
> return 0;
> }
>
> -static void superblock_free_security(struct super_block *sb)
> -{
> - struct superblock_security_struct *sbsec = sb->s_security;
> - sb->s_security = NULL;
> - kfree(sbsec);
> -}
> -
> struct selinux_mnt_opts {
> const char *fscontext, *context, *rootcontext, *defcontext;
> };
> @@ -507,7 +495,7 @@ static int selinux_is_genfs_special_handling(struct super_block *sb)
>
> static int selinux_is_sblabel_mnt(struct super_block *sb)
> {
> - struct superblock_security_struct *sbsec = sb->s_security;
> + struct superblock_security_struct *sbsec = selinux_superblock(sb);
>
> /*
> * IMPORTANT: Double-check logic in this function when adding a new
> @@ -535,7 +523,7 @@ static int selinux_is_sblabel_mnt(struct super_block *sb)
>
> static int sb_finish_set_opts(struct super_block *sb)
> {
> - struct superblock_security_struct *sbsec = sb->s_security;
> + struct superblock_security_struct *sbsec = selinux_superblock(sb);
> struct dentry *root = sb->s_root;
> struct inode *root_inode = d_backing_inode(root);
> int rc = 0;
> @@ -648,7 +636,7 @@ static int selinux_set_mnt_opts(struct super_block *sb,
> unsigned long *set_kern_flags)
> {
> const struct cred *cred = current_cred();
> - struct superblock_security_struct *sbsec = sb->s_security;
> + struct superblock_security_struct *sbsec = selinux_superblock(sb);
> struct dentry *root = sbsec->sb->s_root;
> struct selinux_mnt_opts *opts = mnt_opts;
> struct inode_security_struct *root_isec;
> @@ -881,8 +869,8 @@ static int selinux_set_mnt_opts(struct super_block *sb,
> static int selinux_cmp_sb_context(const struct super_block *oldsb,
> const struct super_block *newsb)
> {
> - struct superblock_security_struct *old = oldsb->s_security;
> - struct superblock_security_struct *new = newsb->s_security;
> + struct superblock_security_struct *old = selinux_superblock(oldsb);
> + struct superblock_security_struct *new = selinux_superblock(newsb);
> char oldflags = old->flags & SE_MNTMASK;
> char newflags = new->flags & SE_MNTMASK;
>
> @@ -914,8 +902,9 @@ static int selinux_sb_clone_mnt_opts(const struct super_block *oldsb,
> unsigned long *set_kern_flags)
> {
> int rc = 0;
> - const struct superblock_security_struct *oldsbsec = oldsb->s_security;
> - struct superblock_security_struct *newsbsec = newsb->s_security;
> + const struct superblock_security_struct *oldsbsec =
> + selinux_superblock(oldsb);
> + struct superblock_security_struct *newsbsec = selinux_superblock(newsb);
>
> int set_fscontext = (oldsbsec->flags & FSCONTEXT_MNT);
> int set_context = (oldsbsec->flags & CONTEXT_MNT);
> @@ -1085,7 +1074,7 @@ static int show_sid(struct seq_file *m, u32 sid)
>
> static int selinux_sb_show_options(struct seq_file *m, struct super_block *sb)
> {
> - struct superblock_security_struct *sbsec = sb->s_security;
> + struct superblock_security_struct *sbsec = selinux_superblock(sb);
> int rc;
>
> if (!(sbsec->flags & SE_SBINITIALIZED))
> @@ -1377,7 +1366,7 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent
> if (isec->sclass == SECCLASS_FILE)
> isec->sclass = inode_mode_to_security_class(inode->i_mode);
>
> - sbsec = inode->i_sb->s_security;
> + sbsec = selinux_superblock(inode->i_sb);
> if (!(sbsec->flags & SE_SBINITIALIZED)) {
> /* Defer initialization until selinux_complete_init,
> after the initial policy is loaded and the security
> @@ -1767,7 +1756,8 @@ selinux_determine_inode_label(const struct task_security_struct *tsec,
> const struct qstr *name, u16 tclass,
> u32 *_new_isid)
> {
> - const struct superblock_security_struct *sbsec = dir->i_sb->s_security;
> + const struct superblock_security_struct *sbsec =
> + selinux_superblock(dir->i_sb);
>
> if ((sbsec->flags & SE_SBINITIALIZED) &&
> (sbsec->behavior == SECURITY_FS_USE_MNTPOINT)) {
> @@ -1798,7 +1788,7 @@ static int may_create(struct inode *dir,
> int rc;
>
> dsec = inode_security(dir);
> - sbsec = dir->i_sb->s_security;
> + sbsec = selinux_superblock(dir->i_sb);
>
> sid = tsec->sid;
>
> @@ -1947,7 +1937,7 @@ static int superblock_has_perm(const struct cred *cred,
> struct superblock_security_struct *sbsec;
> u32 sid = cred_sid(cred);
>
> - sbsec = sb->s_security;
> + sbsec = selinux_superblock(sb);
> return avc_has_perm(&selinux_state,
> sid, sbsec->sid, SECCLASS_FILESYSTEM, perms, ad);
> }
> @@ -2578,11 +2568,6 @@ static int selinux_sb_alloc_security(struct super_block *sb)
> return superblock_alloc_security(sb);
> }
>
> -static void selinux_sb_free_security(struct super_block *sb)
> -{
> - superblock_free_security(sb);
> -}
> -
> static inline int opt_len(const char *s)
> {
> bool open_quote = false;
> @@ -2653,7 +2638,7 @@ static int selinux_sb_eat_lsm_opts(char *options, void **mnt_opts)
> static int selinux_sb_remount(struct super_block *sb, void *mnt_opts)
> {
> struct selinux_mnt_opts *opts = mnt_opts;
> - struct superblock_security_struct *sbsec = sb->s_security;
> + struct superblock_security_struct *sbsec = selinux_superblock(sb);
> u32 sid;
> int rc;
>
> @@ -2877,7 +2862,7 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
> int rc;
> char *context;
>
> - sbsec = dir->i_sb->s_security;
> + sbsec = selinux_superblock(dir->i_sb);
>
> newsid = tsec->create_sid;
>
> @@ -3115,7 +3100,7 @@ static int selinux_inode_setxattr(struct dentry *dentry, const char *name,
> return dentry_has_perm(current_cred(), dentry, FILE__SETATTR);
> }
>
> - sbsec = inode->i_sb->s_security;
> + sbsec = selinux_superblock(inode->i_sb);
> if (!(sbsec->flags & SBLABEL_MNT))
> return -EOPNOTSUPP;
>
> @@ -3296,13 +3281,14 @@ static int selinux_inode_setsecurity(struct inode *inode, const char *name,
> const void *value, size_t size, int flags)
> {
> struct inode_security_struct *isec = inode_security_novalidate(inode);
> - struct superblock_security_struct *sbsec = inode->i_sb->s_security;
> + struct superblock_security_struct *sbsec;
> u32 newsid;
> int rc;
>
> if (strcmp(name, XATTR_SELINUX_SUFFIX))
> return -EOPNOTSUPP;
>
> + sbsec = selinux_superblock(inode->i_sb);
> if (!(sbsec->flags & SBLABEL_MNT))
> return -EOPNOTSUPP;
>
> @@ -6647,6 +6633,7 @@ struct lsm_blob_sizes selinux_blob_sizes __lsm_ro_after_init = {
> .lbs_inode = sizeof(struct inode_security_struct),
> .lbs_ipc = sizeof(struct ipc_security_struct),
> .lbs_msg_msg = sizeof(struct msg_security_struct),
> + .lbs_superblock = sizeof(struct superblock_security_struct),
> };
>
> static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
> @@ -6675,7 +6662,6 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
> LSM_HOOK_INIT(fs_context_parse_param, selinux_fs_context_parse_param),
>
> LSM_HOOK_INIT(sb_alloc_security, selinux_sb_alloc_security),
> - LSM_HOOK_INIT(sb_free_security, selinux_sb_free_security),
> LSM_HOOK_INIT(sb_eat_lsm_opts, selinux_sb_eat_lsm_opts),
> LSM_HOOK_INIT(sb_free_mnt_opts, selinux_free_mnt_opts),
> LSM_HOOK_INIT(sb_remount, selinux_sb_remount),
> diff --git a/security/selinux/include/objsec.h b/security/selinux/include/objsec.h
> index 231262d8eac9..d08d7e5d2f93 100644
> --- a/security/selinux/include/objsec.h
> +++ b/security/selinux/include/objsec.h
> @@ -188,4 +188,10 @@ static inline struct ipc_security_struct *selinux_ipc(
> return ipc->security + selinux_blob_sizes.lbs_ipc;
> }
>
> +static inline struct superblock_security_struct *selinux_superblock(
> + const struct super_block *superblock)
> +{
> + return superblock->s_security + selinux_blob_sizes.lbs_superblock;
> +}
> +
> #endif /* _SELINUX_OBJSEC_H_ */
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index ec62918521b1..e3f5d6aece66 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -50,6 +50,7 @@
> #include <linux/audit.h>
> #include <linux/mutex.h>
> #include <linux/vmalloc.h>
> +#include <linux/lsm_hooks.h>
> #include <net/netlabel.h>
>
> #include "flask.h"
> @@ -2751,7 +2752,7 @@ int security_fs_use(struct selinux_state *state, struct super_block *sb)
> struct sidtab *sidtab;
> int rc = 0;
> struct ocontext *c;
> - struct superblock_security_struct *sbsec = sb->s_security;
> + struct superblock_security_struct *sbsec = selinux_superblock(sb);
> const char *fstype = sb->s_type->name;
>
> read_lock(&state->ss->policy_rwlock);
> diff --git a/security/smack/smack.h b/security/smack/smack.h
> index cf52af77d15e..caecbcba9942 100644
> --- a/security/smack/smack.h
> +++ b/security/smack/smack.h
> @@ -375,6 +375,12 @@ static inline struct smack_known **smack_ipc(const struct kern_ipc_perm *ipc)
> return ipc->security + smack_blob_sizes.lbs_ipc;
> }
>
> +static inline struct superblock_smack *smack_superblock(
> + const struct super_block *superblock)
> +{
> + return superblock->s_security + smack_blob_sizes.lbs_superblock;
> +}
> +
> /*
> * Is the directory transmuting?
> */
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 5c1613519d5a..807eff2ccce9 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -540,12 +540,7 @@ static int smack_syslog(int typefrom_file)
> */
> static int smack_sb_alloc_security(struct super_block *sb)
> {
> - struct superblock_smack *sbsp;
> -
> - sbsp = kzalloc(sizeof(struct superblock_smack), GFP_KERNEL);
> -
> - if (sbsp == NULL)
> - return -ENOMEM;
> + struct superblock_smack *sbsp = smack_superblock(sb);
>
> sbsp->smk_root = &smack_known_floor;
> sbsp->smk_default = &smack_known_floor;
> @@ -554,22 +549,10 @@ static int smack_sb_alloc_security(struct super_block *sb)
> /*
> * SMK_SB_INITIALIZED will be zero from kzalloc.
> */
> - sb->s_security = sbsp;
>
> return 0;
> }
>
> -/**
> - * smack_sb_free_security - free a superblock blob
> - * @sb: the superblock getting the blob
> - *
> - */
> -static void smack_sb_free_security(struct super_block *sb)
> -{
> - kfree(sb->s_security);
> - sb->s_security = NULL;
> -}
> -
> struct smack_mnt_opts {
> const char *fsdefault, *fsfloor, *fshat, *fsroot, *fstransmute;
> };
> @@ -781,7 +764,7 @@ static int smack_set_mnt_opts(struct super_block *sb,
> {
> struct dentry *root = sb->s_root;
> struct inode *inode = d_backing_inode(root);
> - struct superblock_smack *sp = sb->s_security;
> + struct superblock_smack *sp = smack_superblock(sb);
> struct inode_smack *isp;
> struct smack_known *skp;
> struct smack_mnt_opts *opts = mnt_opts;
> @@ -880,7 +863,7 @@ static int smack_set_mnt_opts(struct super_block *sb,
> */
> static int smack_sb_statfs(struct dentry *dentry)
> {
> - struct superblock_smack *sbp = dentry->d_sb->s_security;
> + struct superblock_smack *sbp = smack_superblock(dentry->d_sb);
> int rc;
> struct smk_audit_info ad;
>
> @@ -917,7 +900,7 @@ static int smack_bprm_set_creds(struct linux_binprm *bprm)
> if (isp->smk_task == NULL || isp->smk_task == bsp->smk_task)
> return 0;
>
> - sbsp = inode->i_sb->s_security;
> + sbsp = smack_superblock(inode->i_sb);
> if ((sbsp->smk_flags & SMK_SB_UNTRUSTED) &&
> isp->smk_task != sbsp->smk_root)
> return 0;
> @@ -1168,7 +1151,7 @@ static int smack_inode_rename(struct inode *old_inode,
> */
> static int smack_inode_permission(struct inode *inode, int mask)
> {
> - struct superblock_smack *sbsp = inode->i_sb->s_security;
> + struct superblock_smack *sbsp = smack_superblock(inode->i_sb);
> struct smk_audit_info ad;
> int no_block = mask & MAY_NOT_BLOCK;
> int rc;
> @@ -1410,7 +1393,7 @@ static int smack_inode_removexattr(struct dentry *dentry, const char *name)
> */
> if (strcmp(name, XATTR_NAME_SMACK) == 0) {
> struct super_block *sbp = dentry->d_sb;
> - struct superblock_smack *sbsp = sbp->s_security;
> + struct superblock_smack *sbsp = smack_superblock(sbp);
>
> isp->smk_inode = sbsp->smk_default;
> } else if (strcmp(name, XATTR_NAME_SMACKEXEC) == 0)
> @@ -1680,7 +1663,7 @@ static int smack_mmap_file(struct file *file,
> isp = smack_inode(file_inode(file));
> if (isp->smk_mmap == NULL)
> return 0;
> - sbsp = file_inode(file)->i_sb->s_security;
> + sbsp = smack_superblock(file_inode(file)->i_sb);
> if (sbsp->smk_flags & SMK_SB_UNTRUSTED &&
> isp->smk_mmap != sbsp->smk_root)
> return -EACCES;
> @@ -3288,7 +3271,7 @@ static void smack_d_instantiate(struct dentry *opt_dentry, struct inode *inode)
> goto unlockandout;
>
> sbp = inode->i_sb;
> - sbsp = sbp->s_security;
> + sbsp = smack_superblock(sbp);
> /*
> * We're going to use the superblock default label
> * if there's no label on the file.
> @@ -4575,6 +4558,7 @@ struct lsm_blob_sizes smack_blob_sizes __lsm_ro_after_init = {
> .lbs_inode = sizeof(struct inode_smack),
> .lbs_ipc = sizeof(struct smack_known *),
> .lbs_msg_msg = sizeof(struct smack_known *),
> + .lbs_superblock = sizeof(struct superblock_smack),
> };
>
> static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
> @@ -4586,7 +4570,6 @@ static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
> LSM_HOOK_INIT(fs_context_parse_param, smack_fs_context_parse_param),
>
> LSM_HOOK_INIT(sb_alloc_security, smack_sb_alloc_security),
> - LSM_HOOK_INIT(sb_free_security, smack_sb_free_security),
> LSM_HOOK_INIT(sb_free_mnt_opts, smack_free_mnt_opts),
> LSM_HOOK_INIT(sb_eat_lsm_opts, smack_sb_eat_lsm_opts),
> LSM_HOOK_INIT(sb_statfs, smack_sb_statfs),
>
^ permalink raw reply
* [PATCH] LSM: SafeSetID: Stop releasing uninitialized ruleset
From: mortonm @ 2019-09-16 15:29 UTC (permalink / raw)
To: linux-security-module; +Cc: Micah Morton, Jann Horn
From: Micah Morton <mortonm@chromium.org>
The first time a rule set is configured for SafeSetID, we shouldn't be
trying to release the previously configured ruleset, since there isn't
one. Currently, the pointer that would point to a previously configured
ruleset is uninitialized on first rule set configuration, leading to a
crash when we try to call release_ruleset with that pointer.
Acked-by: Jann Horn <jannh@google.com>
Signed-off-by: Micah Morton <mortonm@chromium.org>
---
security/safesetid/securityfs.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c
index d568e17dd773..74a13d432ed8 100644
--- a/security/safesetid/securityfs.c
+++ b/security/safesetid/securityfs.c
@@ -187,7 +187,8 @@ static ssize_t handle_policy_update(struct file *file,
out_free_buf:
kfree(buf);
out_free_pol:
- release_ruleset(pol);
+ if (pol)
+ release_ruleset(pol);
return err;
}
--
2.23.0.237.gc6a4ce50a0-goog
^ permalink raw reply related
* Re: [RFC v1 12/14] krsi: Add an eBPF helper function to get the value of an env variable
From: KP Singh @ 2019-09-16 13:00 UTC (permalink / raw)
To: Yonghong Song
Cc: KP Singh, linux-kernel@vger.kernel.org, bpf@vger.kernel.org,
linux-security-module@vger.kernel.org, Alexei Starovoitov,
Daniel Borkmann, James Morris, Kees Cook, Thomas Garnier,
Michael Halcrow, Paul Turner, Brendan Gregg, Jann Horn,
Matthew Garrett, Christian Brauner, Mickaël Salaün,
Florent Revest, Martin Lau, Song Liu, Serge E. Hallyn,
Mauro Carvalho Chehab, David S. Miller, Greg Kroah-Hartman,
Nicolas Ferre, Stanislav Fomichev, Quentin Monnet, Andrey Ignatov,
Joe Stringer
In-Reply-To: <0a5386c9-3dbd-1ed8-d94c-d866c6369743@fb.com>
Thanks for reviewing!
On 15-Sep 00:16, Yonghong Song wrote:
>
>
> On 9/10/19 12:55 PM, KP Singh wrote:
> > From: KP Singh <kpsingh@google.com>
>
> This patch cannot apply cleanly.
>
> -bash-4.4$ git apply ~/p12.txt
> error: patch failed: include/uapi/linux/bpf.h:2715
> error: include/uapi/linux/bpf.h: patch does not apply
> error: patch failed: tools/include/uapi/linux/bpf.h:2715
> error: tools/include/uapi/linux/bpf.h: patch does not apply
> -bash-4.4$
I am not sure why this is happening, I tried:
git clone \
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git && \
cd linux-next && \
git checkout -b review v5.3-rc6 && \
wget -P /tmp https://lore.kernel.org/patchwork/series/410101/mbox && \
git am /tmp/mbox
and it worked.
This seems to work too:
patch -p1 < <file>.patch
Can you try with "git am" please?
>
> >
> > The helper returns the value of the environment variable in the buffer
> > that is passed to it. If the var is set multiple times, the helper
> > returns all the values as null separated strings.
> >
> > If the buffer is too short for these values, the helper tries to fill it
> > the best it can and guarantees that the value returned in the buffer
> > is always null terminated. After the buffer is filled, the helper keeps
> > counting the number of times the environment variable is set in the
> > envp.
> >
> > The return value of the helper is an u64 value which carries two pieces
> > of information.
> >
> > * The upper 32 bits are a u32 value signifying the number of times
> > the environment variable is set in the envp.
>
> Not sure how useful this 'upper 32' bit value is. What user expected to do?
>
> Another option is to have upper 32 bits encode the required buffer size
> to hold all values. This may cause some kind of user space action, e.g.,
> to replace the program with new program with larger per cpu map value size?
>
The upper 32-bit value is actually an important part of the LSM's MAC
policy. It allows the user to:
- Return an -EPERM when if the environment variable is set more than
once.
- Log a warning (this is what we are doing in the example) so
this is flagged as a potential malicious actor.
> > * The lower 32 bits are a s32 value signifying the number of bytes
> > written to the buffer or an error code. >
> > Since the value of the environment variable can be very long and exceed
> > what can be allocated on the BPF stack, a per-cpu array can be used
> > instead:
> >
> > struct bpf_map_def SEC("maps") env_map = {
> > .type = BPF_MAP_TYPE_PERCPU_ARRAY,
> > .key_size = sizeof(u32),
> > .value_size = 4096,
> > .max_entries = 1,
> > };
>
> Could you use use map definition with SEC(".maps")?
Sure, I added this example program in the commit message. Will update
it to be more canonical. Thanks!
>
> >
> > SEC("prgrm")
> > int bpf_prog1(void *ctx)
> > {
> > u32 map_id = 0;
> > u64 times_ret;
> > s32 ret;
> > char name[48] = "LD_PRELOAD";
>
> Reverse Christmas tree coding style, here and other places?
Will happily fix it.
However, I did not find it mentioned in the style guide:
https://www.kernel.org/doc/html/v4.10/process/coding-style.html
https://elixir.bootlin.com/linux/v4.6/source/Documentation/CodingStyle
Is there one specific to BPF?
>
> >
> > char *map_value = bpf_map_lookup_elem(&env_map, &map_id);
> > if (!map_value)
> > return 0;
> >
> > // Read the lower 32 bits for the return value
> > times_ret = krsi_get_env_var(ctx, name, 48, map_value, 4096);
> > ret = times_ret & 0xffffffff;
> > if (ret < 0)
> > return ret;
> > return 0;
> > }
> >
> > Signed-off-by: KP Singh <kpsingh@google.com>
> > ---
> > include/uapi/linux/bpf.h | 42 ++++++-
> > security/krsi/ops.c | 129 ++++++++++++++++++++++
> > tools/include/uapi/linux/bpf.h | 42 ++++++-
> > tools/testing/selftests/bpf/bpf_helpers.h | 3 +
> > 4 files changed, 214 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 32ab38f1a2fe..a4ef07956e07 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -2715,6 +2715,45 @@ union bpf_attr {
> > * **-EPERM** if no permission to send the *sig*.
> > *
> > * **-EAGAIN** if bpf program can try again.
> > + *
> > + * u64 krsi_get_env_var(void *ctx, char *name, char *buf,
> > + * size_t name_len, size_t buf_len)
>
> This signature is not the same as the later
> krsi_get_env_var(...) helper definition.
> BPF_CALL_5(krsi_get_env_var, struct krsi_ctx *, ctx, char *, name, u32,
> n_size,
> char *, dest, u32, size)
>
I did this because the krsi_ctx is not exposed to the userspace and
allows KRSI to modify the context without worrying about breaking
userspace.
That said, I could mark it as a (void *) here and cast it internally.
I guess that would be better/cleaner?
> > + * Description
> > + * This helper can be used as a part of the
> > + * process_execution hook of the KRSI LSM in
> > + * programs of type BPF_PROG_TYPE_KRSI.
> > + *
> > + * The helper returns the value of the environment
> > + * variable with the provided "name" for process that's
> > + * going to be executed in the passed buffer, "buf". If the var
> > + * is set multiple times, the helper returns all
> > + * the values as null separated strings.
> > + *
> > + * If the buffer is too short for these values, the helper
> > + * tries to fill it the best it can and guarantees that the value
> > + * returned in the buffer is always null terminated.
> > + * After the buffer is filled, the helper keeps counting the number
> > + * of times the environment variable is set in the envp.
> > + *
> > + * Return:
> > + *
> > + * The return value of the helper is an u64 value
> > + * which carries two pieces of information:
> > + *
> > + * The upper 32 bits are a u32 value signifying
> > + * the number of times the environment variable
> > + * is set in the envp.
> > + * The lower 32 bits are an s32 value signifying
> > + * the number of bytes written to the buffer or an error code:
> > + *
> > + * **-ENOMEM** if the kernel is unable to allocate memory
> > + * for pinning the argv and envv.
> > + *
> > + * **-E2BIG** if the value is larger than the size of the
> > + * destination buffer. The higher bits will still
> > + * the number of times the variable was set in the envp.
>
> The -E2BIG is returned because buffer sizee is not big enough.
> Another possible error code is -ENOSPC, which typically indicates
> buffer size not big enough.
Sure, I am fine with using either.
>
> > + *
> > + * **-EINVAL** if name is not a NULL terminated string.
> > */
> > #define __BPF_FUNC_MAPPER(FN) \
> > FN(unspec), \
> > @@ -2826,7 +2865,8 @@ union bpf_attr {
> > FN(strtoul), \
> > FN(sk_storage_get), \
> > FN(sk_storage_delete), \
> > - FN(send_signal),
> > + FN(send_signal), \
> > + FN(krsi_get_env_var),
> >
> > /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> > * function eBPF program intends to call
> > diff --git a/security/krsi/ops.c b/security/krsi/ops.c
> > index 1f4df920139c..1db94dfaac15 100644
> > --- a/security/krsi/ops.c
> > +++ b/security/krsi/ops.c
> > @@ -6,6 +6,8 @@
> > #include <linux/bpf.h>
> > #include <linux/security.h>
> > #include <linux/krsi.h>
> > +#include <linux/binfmts.h>
> > +#include <linux/highmem.h>
> >
> > #include "krsi_init.h"
> > #include "krsi_fs.h"
> > @@ -162,6 +164,131 @@ static bool krsi_prog_is_valid_access(int off, int size,
> > return false;
> > }
> >
> > +static char *array_next_entry(char *array, unsigned long *offset,
> > + unsigned long end)
> > +{
> > + char *entry;
> > + unsigned long current_offset = *offset;
> > +
> > + if (current_offset >= end)
> > + return NULL;
> > +
> > + /*
> > + * iterate on the array till the null byte is encountered
> > + * and check for any overflows.
> > + */
> > + entry = array + current_offset;
> > + while (array[current_offset]) {
> > + if (unlikely(++current_offset >= end))
> > + return NULL;
> > + }
> > +
> > + /*
> > + * Point the offset to the next element in the array.
> > + */
> > + *offset = current_offset + 1;
> > +
> > + return entry;
> > +}
> > +
> > +static u64 get_env_var(struct krsi_ctx *ctx, char *name, char *dest,
> > + u32 n_size, u32 size)
> > +{
> > + s32 ret = 0;
> > + u32 num_vars = 0;
> > + int i, name_len;
> > + struct linux_binprm *bprm = ctx->bprm_ctx.bprm;
> > + int argc = bprm->argc;
> > + int envc = bprm->envc;
> > + unsigned long end = ctx->bprm_ctx.max_arg_offset;
> > + unsigned long offset = bprm->p % PAGE_SIZE;
>
> why we need bprm->p % PAGE_SIZE instead of bprm->p?
bprm->p points to the top of the memory and it's not an offset.
The pinned buffer contains the pages for the (argv+env) and the
brpm->p % PAGE_SIZE is the offset into the first page where the
(argv+envv) starts.
>
> > + char *buf = ctx->bprm_ctx.arg_pages;
> > + char *curr_dest = dest;
> > + char *entry;
> > +
> > + if (unlikely(!buf))
> > + return -ENOMEM;
> > +
> > + for (i = 0; i < argc; i++) {
> > + entry = array_next_entry(buf, &offset, end);
> > + if (!entry)
> > + return 0;
> > + }
> > +
> > + name_len = strlen(name);
> > + for (i = 0; i < envc; i++) {
> > + entry = array_next_entry(buf, &offset, end);
> > + if (!entry)
> > + return 0;
>
> If the buf is "LD_PRELOAD=a.so\0LD_PRELOAD=b.so" and argc=0,
> we may skip the first entry?
I think I need to rename the "array_next_entry" function / document it
better.
The function updates the offset to the next location and the
returns the entry at the current offset.
So, in the first instance:
// offset is the offset into the first page.
entry = buf + offset;
offset = <updated value to the next entry>.
>
>
> > +
> > + if (!strncmp(entry, name, name_len)) {
> > + num_vars++;
>
> There helper permits n_size = 0 (ARG_CONST_SIZE_OR_ZERO).
> in this case, name_len = 0, strncmp(entry, name, name_len) will be always 0.
Thanks, you are right, It does not make sense to have name_len = 0. I
will change it to ARG_CONST_SIZE.
>
> > +
> > + /*
> > + * There is no need to do further copying
> > + * if the buffer is already full. Just count how many
> > + * times the environment variable is set.
> > + */
> > + if (ret == -E2BIG)
> > + continue;
> > +
> > + if (entry[name_len] != '=')
> > + continue;
> > +
> > + /*
> > + * Move the buf pointer by name_len + 1
> > + * (for the "=" sign)
> > + */
> > + entry += name_len + 1;
> > + ret = strlcpy(curr_dest, entry, size);
> > +
> > + if (ret >= size) {
> > + ret = -E2BIG;
>
> Here, we have a partial copy. Should you instead nullify (memset) it as
> it is not really invalid one?
The function does specify that the it will return a null terminated
value even if an -E2BIG is returned so that user does get a truncated
value. It's better to give the user some data. (I mentioned this in
the documentation for the helper).
>
> > + continue;
> > + }
> > +
> > + /*
> > + * strlcpy just returns the length of the string copied.
> > + * The remaining space needs to account for the added
> > + * null character.
> > + */
> > + curr_dest += ret + 1;
> > + size -= ret + 1;
> > + /*
> > + * Update ret to be the current number of bytes written
> > + * to the destination
> > + */
> > + ret = curr_dest - dest;
> > + }
> > + }
> > +
> > + return (u64) num_vars << 32 | (u32) ret;
> > +}
> > +
> > +BPF_CALL_5(krsi_get_env_var, struct krsi_ctx *, ctx, char *, name, u32, n_size,
> > + char *, dest, u32, size)
> > +{
> > + char *name_end;
> > +
> > + name_end = memchr(name, '\0', n_size);
> > + if (!name_end)
> > + return -EINVAL;
> > +
> > + memset(dest, 0, size);
This memset ensures the buffer is zeroed out (incase the buffer is
fully / partially empty).
> > + return get_env_var(ctx, name, dest, n_size, size);
> > +}
> > +
> > +static const struct bpf_func_proto krsi_get_env_var_proto = {
> > + .func = krsi_get_env_var,
> > + .gpl_only = true,
> > + .ret_type = RET_INTEGER,
> > + .arg1_type = ARG_PTR_TO_CTX,
> > + .arg2_type = ARG_PTR_TO_MEM,
> > + .arg3_type = ARG_CONST_SIZE_OR_ZERO,
> > + .arg4_type = ARG_PTR_TO_UNINIT_MEM,
> > + .arg5_type = ARG_CONST_SIZE_OR_ZERO,
> > +};
> > +
> > BPF_CALL_5(krsi_event_output, void *, log,
> > struct bpf_map *, map, u64, flags, void *, data, u64, size)
> > {
> > @@ -192,6 +319,8 @@ static const struct bpf_func_proto *krsi_prog_func_proto(enum bpf_func_id
> > return &bpf_map_lookup_elem_proto;
> > case BPF_FUNC_get_current_pid_tgid:
> > return &bpf_get_current_pid_tgid_proto;
> > + case BPF_FUNC_krsi_get_env_var:
> > + return &krsi_get_env_var_proto;
> > case BPF_FUNC_perf_event_output:
> > return &krsi_event_output_proto;
> > default:
> > diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> > index 32ab38f1a2fe..a4ef07956e07 100644
> > --- a/tools/include/uapi/linux/bpf.h
> > +++ b/tools/include/uapi/linux/bpf.h
> > @@ -2715,6 +2715,45 @@ union bpf_attr {
> > * **-EPERM** if no permission to send the *sig*.
> > *
> > * **-EAGAIN** if bpf program can try again.
> > + *
> > + * u64 krsi_get_env_var(void *ctx, char *name, char *buf,
> > + * size_t name_len, size_t buf_len)
>
> Inconsistent helper definitions.
As discussed above, I can change the BPF_CALL_5 declaration to have
a (void *) and cast to the krsi_ctx in the helper itself.
- KP
>
> > + * Description
> > + * This helper can be used as a part of the
> > + * process_execution hook of the KRSI LSM in
> > + * programs of type BPF_PROG_TYPE_KRSI.
> > + *
> > + * The helper returns the value of the environment
> > + * variable with the provided "name" for process that's
> > + * going to be executed in the passed buffer, "buf". If the var
> > + * is set multiple times, the helper returns all
> > + * the values as null separated strings.
> > + *
> > + * If the buffer is too short for these values, the helper
> > + * tries to fill it the best it can and guarantees that the value
> > + * returned in the buffer is always null terminated.
> > + * After the buffer is filled, the helper keeps counting the number
> > + * of times the environment variable is set in the envp.
> > + *
> > + * Return:
> > + *
> > + * The return value of the helper is an u64 value
> > + * which carries two pieces of information:
> > + *
> > + * The upper 32 bits are a u32 value signifying
> > + * the number of times the environment variable
> > + * is set in the envp.
> > + * The lower 32 bits are an s32 value signifying
> > + * the number of bytes written to the buffer or an error code:
> > + *
> > + * **-ENOMEM** if the kernel is unable to allocate memory
> > + * for pinning the argv and envv.
> > + *
> > + * **-E2BIG** if the value is larger than the size of the
> > + * destination buffer. The higher bits will still
> > + * the number of times the variable was set in the envp.
> > + *
> > + * **-EINVAL** if name is not a NULL terminated string.
> > */
> > #define __BPF_FUNC_MAPPER(FN) \
> > FN(unspec), \
> > @@ -2826,7 +2865,8 @@ union bpf_attr {
> > FN(strtoul), \
> > FN(sk_storage_get), \
> > FN(sk_storage_delete), \
> > - FN(send_signal),
> > + FN(send_signal), \
> > + FN(krsi_get_env_var),
> >
> > /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> > * function eBPF program intends to call
> > diff --git a/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h
> > index f804f210244e..ecebdb772a9d 100644
> > --- a/tools/testing/selftests/bpf/bpf_helpers.h
> > +++ b/tools/testing/selftests/bpf/bpf_helpers.h
> > @@ -303,6 +303,9 @@ static int (*bpf_get_numa_node_id)(void) =
> > static int (*bpf_probe_read_str)(void *ctx, __u32 size,
> > const void *unsafe_ptr) =
> > (void *) BPF_FUNC_probe_read_str;
> > +static unsigned long long (*krsi_get_env_var)(void *ctx,
> > + void *name, __u32 n_size, void *buf, __u32 size) =
> > + (void *) BPF_FUNC_krsi_get_env_var;
> > static unsigned int (*bpf_get_socket_uid)(void *ctx) =
> > (void *) BPF_FUNC_get_socket_uid;
> > static unsigned int (*bpf_set_hash)(void *ctx, __u32 hash) =
> >
^ permalink raw reply
* Re: [PATCH 1/3] ima: keep the integrity state of open files up to date
From: Janne Karhunen @ 2019-09-16 11:45 UTC (permalink / raw)
To: Eric Biggers
Cc: linux-integrity, linux-security-module, Mimi Zohar, linux-mm,
viro, Konsta Karsisto
In-Reply-To: <20190915202433.GC1704@sol.localdomain>
On Sun, Sep 15, 2019 at 11:24 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > > This still doesn't make it crash-safe. So why is it okay?
> >
> > If Android is the load, this makes it crash safe 99% of the time and
> > that is considerably better than 0% of the time.
> >
>
> Who will use it if it isn't 100% safe?
I suppose anyone using mutable data with IMA appraise should, unless
they have a redundant power supply and a kernel that never crashes. In
a way this is like asking if the ima-appraise should be there for
mutable data at all. All this is doing is that it improves the crash
recovery reliability without taking anything away.
Anyway, I think I'm getting along with my understanding of the page
writeback slowly and the journal support will eventually be there at
least as an add-on patch for those that want to use it and really need
the last 0.n% reliability. Note that even without that patch you can
build ima-appraise based systems that are 99.999% reliable just by
having the patch we're discussing here. Without it you would be orders
of magnitude worse off. All we are doing is that we give it a fairly
good chance to recover instead of giving up without even trying.
That said, I'm not sure the 100% crash recovery is ever guaranteed in
any Linux system. We just have to do what we can, no?
--
Janne
^ permalink raw reply
* [Patch v6 4/4] KEYS: trusted: Move TPM2 trusted keys code
From: Sumit Garg @ 2019-09-16 10:34 UTC (permalink / raw)
To: jarkko.sakkinen, dhowells, peterhuewe
Cc: keyrings, linux-integrity, linux-crypto, linux-security-module,
herbert, davem, jgg, arnd, gregkh, jejb, zohar, jmorris, serge,
jsnitsel, linux-kernel, daniel.thompson, Sumit Garg
In-Reply-To: <1568630064-14887-1-git-send-email-sumit.garg@linaro.org>
Move TPM2 trusted keys code to trusted keys subsystem. The reason
being it's better to consolidate all the trusted keys code to a single
location so that it can be maintained sanely.
Suggested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
drivers/char/tpm/tpm-chip.c | 1 +
drivers/char/tpm/tpm-interface.c | 56 -----
drivers/char/tpm/tpm.h | 16 --
drivers/char/tpm/tpm2-cmd.c | 308 +------------------------
include/keys/trusted_tpm.h | 7 +
include/linux/tpm.h | 56 +++--
security/keys/trusted-keys/Makefile | 1 +
security/keys/trusted-keys/trusted_tpm2.c | 368 ++++++++++++++++++++++++++++++
8 files changed, 419 insertions(+), 394 deletions(-)
create mode 100644 security/keys/trusted-keys/trusted_tpm2.c
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 3d6d394..3a2eb11 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -252,6 +252,7 @@ struct tpm_chip *tpm_find_get_ops(struct tpm_chip *chip)
return NULL;
return chip;
}
+EXPORT_SYMBOL_GPL(tpm_find_get_ops);
/**
* tpm_dev_release() - free chip memory and the device number
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index d9ace54..c7eeb40 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -458,62 +458,6 @@ int tpm_get_random(struct tpm_chip *chip, u8 *out, size_t max)
}
EXPORT_SYMBOL_GPL(tpm_get_random);
-/**
- * tpm_seal_trusted() - seal a trusted key payload
- * @chip: a &struct tpm_chip instance, %NULL for the default chip
- * @options: authentication values and other options
- * @payload: the key data in clear and encrypted form
- *
- * Note: only TPM 2.0 chip are supported. TPM 1.x implementation is located in
- * the keyring subsystem.
- *
- * Return: same as with tpm_transmit_cmd()
- */
-int tpm_seal_trusted(struct tpm_chip *chip, struct trusted_key_payload *payload,
- struct trusted_key_options *options)
-{
- int rc;
-
- chip = tpm_find_get_ops(chip);
- if (!chip || !(chip->flags & TPM_CHIP_FLAG_TPM2))
- return -ENODEV;
-
- rc = tpm2_seal_trusted(chip, payload, options);
-
- tpm_put_ops(chip);
- return rc;
-}
-EXPORT_SYMBOL_GPL(tpm_seal_trusted);
-
-/**
- * tpm_unseal_trusted() - unseal a trusted key
- * @chip: a &struct tpm_chip instance, %NULL for the default chip
- * @options: authentication values and other options
- * @payload: the key data in clear and encrypted form
- *
- * Note: only TPM 2.0 chip are supported. TPM 1.x implementation is located in
- * the keyring subsystem.
- *
- * Return: same as with tpm_transmit_cmd()
- */
-int tpm_unseal_trusted(struct tpm_chip *chip,
- struct trusted_key_payload *payload,
- struct trusted_key_options *options)
-{
- int rc;
-
- chip = tpm_find_get_ops(chip);
- if (!chip || !(chip->flags & TPM_CHIP_FLAG_TPM2))
- return -ENODEV;
-
- rc = tpm2_unseal_trusted(chip, payload, options);
-
- tpm_put_ops(chip);
-
- return rc;
-}
-EXPORT_SYMBOL_GPL(tpm_unseal_trusted);
-
static int __init tpm_init(void)
{
int rc;
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 28403a4..5d5e707 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -164,8 +164,6 @@ extern const struct file_operations tpmrm_fops;
extern struct idr dev_nums_idr;
ssize_t tpm_transmit(struct tpm_chip *chip, u8 *buf, size_t bufsiz);
-ssize_t tpm_transmit_cmd(struct tpm_chip *chip, struct tpm_buf *buf,
- size_t min_rsp_body_length, const char *desc);
int tpm_get_timeouts(struct tpm_chip *);
int tpm_auto_startup(struct tpm_chip *chip);
@@ -193,9 +191,7 @@ static inline void tpm_msleep(unsigned int delay_msec)
int tpm_chip_start(struct tpm_chip *chip);
void tpm_chip_stop(struct tpm_chip *chip);
-struct tpm_chip *tpm_find_get_ops(struct tpm_chip *chip);
__must_check int tpm_try_get_ops(struct tpm_chip *chip);
-void tpm_put_ops(struct tpm_chip *chip);
struct tpm_chip *tpm_chip_alloc(struct device *dev,
const struct tpm_class_ops *ops);
@@ -215,24 +211,12 @@ static inline void tpm_add_ppi(struct tpm_chip *chip)
}
#endif
-static inline u32 tpm2_rc_value(u32 rc)
-{
- return (rc & BIT(7)) ? rc & 0xff : rc;
-}
-
int tpm2_get_timeouts(struct tpm_chip *chip);
int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
struct tpm_digest *digest, u16 *digest_size_ptr);
int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
struct tpm_digest *digests);
int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max);
-void tpm2_flush_context(struct tpm_chip *chip, u32 handle);
-int tpm2_seal_trusted(struct tpm_chip *chip,
- struct trusted_key_payload *payload,
- struct trusted_key_options *options);
-int tpm2_unseal_trusted(struct tpm_chip *chip,
- struct trusted_key_payload *payload,
- struct trusted_key_options *options);
ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id,
u32 *value, const char *desc);
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index ba9acae..08e2d989 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -13,20 +13,6 @@
#include "tpm.h"
#include <crypto/hash_info.h>
-#include <keys/trusted-type.h>
-
-enum tpm2_object_attributes {
- TPM2_OA_USER_WITH_AUTH = BIT(6),
-};
-
-enum tpm2_session_attributes {
- TPM2_SA_CONTINUE_SESSION = BIT(0),
-};
-
-struct tpm2_hash {
- unsigned int crypto_id;
- unsigned int tpm_id;
-};
static struct tpm2_hash tpm2_hash_map[] = {
{HASH_ALGO_SHA1, TPM_ALG_SHA1},
@@ -376,299 +362,7 @@ void tpm2_flush_context(struct tpm_chip *chip, u32 handle)
tpm_transmit_cmd(chip, &buf, 0, "flushing context");
tpm_buf_destroy(&buf);
}
-
-/**
- * tpm_buf_append_auth() - append TPMS_AUTH_COMMAND to the buffer.
- *
- * @buf: an allocated tpm_buf instance
- * @session_handle: session handle
- * @nonce: the session nonce, may be NULL if not used
- * @nonce_len: the session nonce length, may be 0 if not used
- * @attributes: the session attributes
- * @hmac: the session HMAC or password, may be NULL if not used
- * @hmac_len: the session HMAC or password length, maybe 0 if not used
- */
-static void tpm2_buf_append_auth(struct tpm_buf *buf, u32 session_handle,
- const u8 *nonce, u16 nonce_len,
- u8 attributes,
- const u8 *hmac, u16 hmac_len)
-{
- tpm_buf_append_u32(buf, 9 + nonce_len + hmac_len);
- tpm_buf_append_u32(buf, session_handle);
- tpm_buf_append_u16(buf, nonce_len);
-
- if (nonce && nonce_len)
- tpm_buf_append(buf, nonce, nonce_len);
-
- tpm_buf_append_u8(buf, attributes);
- tpm_buf_append_u16(buf, hmac_len);
-
- if (hmac && hmac_len)
- tpm_buf_append(buf, hmac, hmac_len);
-}
-
-/**
- * tpm2_seal_trusted() - seal the payload of a trusted key
- *
- * @chip: TPM chip to use
- * @payload: the key data in clear and encrypted form
- * @options: authentication values and other options
- *
- * Return: < 0 on error and 0 on success.
- */
-int tpm2_seal_trusted(struct tpm_chip *chip,
- struct trusted_key_payload *payload,
- struct trusted_key_options *options)
-{
- unsigned int blob_len;
- struct tpm_buf buf;
- u32 hash;
- int i;
- int rc;
-
- for (i = 0; i < ARRAY_SIZE(tpm2_hash_map); i++) {
- if (options->hash == tpm2_hash_map[i].crypto_id) {
- hash = tpm2_hash_map[i].tpm_id;
- break;
- }
- }
-
- if (i == ARRAY_SIZE(tpm2_hash_map))
- return -EINVAL;
-
- rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_CREATE);
- if (rc)
- return rc;
-
- tpm_buf_append_u32(&buf, options->keyhandle);
- tpm2_buf_append_auth(&buf, TPM2_RS_PW,
- NULL /* nonce */, 0,
- 0 /* session_attributes */,
- options->keyauth /* hmac */,
- TPM_DIGEST_SIZE);
-
- /* sensitive */
- tpm_buf_append_u16(&buf, 4 + TPM_DIGEST_SIZE + payload->key_len + 1);
-
- tpm_buf_append_u16(&buf, TPM_DIGEST_SIZE);
- tpm_buf_append(&buf, options->blobauth, TPM_DIGEST_SIZE);
- tpm_buf_append_u16(&buf, payload->key_len + 1);
- tpm_buf_append(&buf, payload->key, payload->key_len);
- tpm_buf_append_u8(&buf, payload->migratable);
-
- /* public */
- tpm_buf_append_u16(&buf, 14 + options->policydigest_len);
- tpm_buf_append_u16(&buf, TPM_ALG_KEYEDHASH);
- tpm_buf_append_u16(&buf, hash);
-
- /* policy */
- if (options->policydigest_len) {
- tpm_buf_append_u32(&buf, 0);
- tpm_buf_append_u16(&buf, options->policydigest_len);
- tpm_buf_append(&buf, options->policydigest,
- options->policydigest_len);
- } else {
- tpm_buf_append_u32(&buf, TPM2_OA_USER_WITH_AUTH);
- tpm_buf_append_u16(&buf, 0);
- }
-
- /* public parameters */
- tpm_buf_append_u16(&buf, TPM_ALG_NULL);
- tpm_buf_append_u16(&buf, 0);
-
- /* outside info */
- tpm_buf_append_u16(&buf, 0);
-
- /* creation PCR */
- tpm_buf_append_u32(&buf, 0);
-
- if (buf.flags & TPM_BUF_OVERFLOW) {
- rc = -E2BIG;
- goto out;
- }
-
- rc = tpm_transmit_cmd(chip, &buf, 4, "sealing data");
- if (rc)
- goto out;
-
- blob_len = be32_to_cpup((__be32 *) &buf.data[TPM_HEADER_SIZE]);
- if (blob_len > MAX_BLOB_SIZE) {
- rc = -E2BIG;
- goto out;
- }
- if (tpm_buf_length(&buf) < TPM_HEADER_SIZE + 4 + blob_len) {
- rc = -EFAULT;
- goto out;
- }
-
- memcpy(payload->blob, &buf.data[TPM_HEADER_SIZE + 4], blob_len);
- payload->blob_len = blob_len;
-
-out:
- tpm_buf_destroy(&buf);
-
- if (rc > 0) {
- if (tpm2_rc_value(rc) == TPM2_RC_HASH)
- rc = -EINVAL;
- else
- rc = -EPERM;
- }
-
- return rc;
-}
-
-/**
- * tpm2_load_cmd() - execute a TPM2_Load command
- *
- * @chip: TPM chip to use
- * @payload: the key data in clear and encrypted form
- * @options: authentication values and other options
- * @blob_handle: returned blob handle
- *
- * Return: 0 on success.
- * -E2BIG on wrong payload size.
- * -EPERM on tpm error status.
- * < 0 error from tpm_transmit_cmd.
- */
-static int tpm2_load_cmd(struct tpm_chip *chip,
- struct trusted_key_payload *payload,
- struct trusted_key_options *options,
- u32 *blob_handle)
-{
- struct tpm_buf buf;
- unsigned int private_len;
- unsigned int public_len;
- unsigned int blob_len;
- int rc;
-
- private_len = be16_to_cpup((__be16 *) &payload->blob[0]);
- if (private_len > (payload->blob_len - 2))
- return -E2BIG;
-
- public_len = be16_to_cpup((__be16 *) &payload->blob[2 + private_len]);
- blob_len = private_len + public_len + 4;
- if (blob_len > payload->blob_len)
- return -E2BIG;
-
- rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_LOAD);
- if (rc)
- return rc;
-
- tpm_buf_append_u32(&buf, options->keyhandle);
- tpm2_buf_append_auth(&buf, TPM2_RS_PW,
- NULL /* nonce */, 0,
- 0 /* session_attributes */,
- options->keyauth /* hmac */,
- TPM_DIGEST_SIZE);
-
- tpm_buf_append(&buf, payload->blob, blob_len);
-
- if (buf.flags & TPM_BUF_OVERFLOW) {
- rc = -E2BIG;
- goto out;
- }
-
- rc = tpm_transmit_cmd(chip, &buf, 4, "loading blob");
- if (!rc)
- *blob_handle = be32_to_cpup(
- (__be32 *) &buf.data[TPM_HEADER_SIZE]);
-
-out:
- tpm_buf_destroy(&buf);
-
- if (rc > 0)
- rc = -EPERM;
-
- return rc;
-}
-
-/**
- * tpm2_unseal_cmd() - execute a TPM2_Unload command
- *
- * @chip: TPM chip to use
- * @payload: the key data in clear and encrypted form
- * @options: authentication values and other options
- * @blob_handle: blob handle
- *
- * Return: 0 on success
- * -EPERM on tpm error status
- * < 0 error from tpm_transmit_cmd
- */
-static int tpm2_unseal_cmd(struct tpm_chip *chip,
- struct trusted_key_payload *payload,
- struct trusted_key_options *options,
- u32 blob_handle)
-{
- struct tpm_buf buf;
- u16 data_len;
- u8 *data;
- int rc;
-
- rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_UNSEAL);
- if (rc)
- return rc;
-
- tpm_buf_append_u32(&buf, blob_handle);
- tpm2_buf_append_auth(&buf,
- options->policyhandle ?
- options->policyhandle : TPM2_RS_PW,
- NULL /* nonce */, 0,
- TPM2_SA_CONTINUE_SESSION,
- options->blobauth /* hmac */,
- TPM_DIGEST_SIZE);
-
- rc = tpm_transmit_cmd(chip, &buf, 6, "unsealing");
- if (rc > 0)
- rc = -EPERM;
-
- if (!rc) {
- data_len = be16_to_cpup(
- (__be16 *) &buf.data[TPM_HEADER_SIZE + 4]);
- if (data_len < MIN_KEY_SIZE || data_len > MAX_KEY_SIZE + 1) {
- rc = -EFAULT;
- goto out;
- }
-
- if (tpm_buf_length(&buf) < TPM_HEADER_SIZE + 6 + data_len) {
- rc = -EFAULT;
- goto out;
- }
- data = &buf.data[TPM_HEADER_SIZE + 6];
-
- memcpy(payload->key, data, data_len - 1);
- payload->key_len = data_len - 1;
- payload->migratable = data[data_len - 1];
- }
-
-out:
- tpm_buf_destroy(&buf);
- return rc;
-}
-
-/**
- * tpm2_unseal_trusted() - unseal the payload of a trusted key
- *
- * @chip: TPM chip to use
- * @payload: the key data in clear and encrypted form
- * @options: authentication values and other options
- *
- * Return: Same as with tpm_transmit_cmd.
- */
-int tpm2_unseal_trusted(struct tpm_chip *chip,
- struct trusted_key_payload *payload,
- struct trusted_key_options *options)
-{
- u32 blob_handle;
- int rc;
-
- rc = tpm2_load_cmd(chip, payload, options, &blob_handle);
- if (rc)
- return rc;
-
- rc = tpm2_unseal_cmd(chip, payload, options, blob_handle);
- tpm2_flush_context(chip, blob_handle);
- return rc;
-}
+EXPORT_SYMBOL_GPL(tpm2_flush_context);
struct tpm2_get_cap_out {
u8 more_data;
diff --git a/include/keys/trusted_tpm.h b/include/keys/trusted_tpm.h
index 7b9d7b4..6f6cd3d 100644
--- a/include/keys/trusted_tpm.h
+++ b/include/keys/trusted_tpm.h
@@ -40,6 +40,13 @@ int TSS_checkhmac1(unsigned char *buffer,
int trusted_tpm_send(unsigned char *cmd, size_t buflen);
int oiap(struct tpm_buf *tb, uint32_t *handle, unsigned char *nonce);
+int tpm_seal_trusted(struct tpm_chip *chip,
+ struct trusted_key_payload *payload,
+ struct trusted_key_options *options);
+int tpm_unseal_trusted(struct tpm_chip *chip,
+ struct trusted_key_payload *payload,
+ struct trusted_key_options *options);
+
#define TPM_DEBUG 0
#if TPM_DEBUG
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 130c167..895179f 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -294,6 +294,19 @@ struct tpm_buf {
u8 *data;
};
+enum tpm2_object_attributes {
+ TPM2_OA_USER_WITH_AUTH = BIT(6),
+};
+
+enum tpm2_session_attributes {
+ TPM2_SA_CONTINUE_SESSION = BIT(0),
+};
+
+struct tpm2_hash {
+ unsigned int crypto_id;
+ unsigned int tpm_id;
+};
+
static inline void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal)
{
struct tpm_header *head = (struct tpm_header *)buf->data;
@@ -375,6 +388,11 @@ static inline void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value)
tpm_buf_append(buf, (u8 *) &value2, 4);
}
+static inline u32 tpm2_rc_value(u32 rc)
+{
+ return (rc & BIT(7)) ? rc & 0xff : rc;
+}
+
#if defined(CONFIG_TCG_TPM) || defined(CONFIG_TCG_TPM_MODULE)
extern int tpm_is_tpm2(struct tpm_chip *chip);
@@ -384,13 +402,12 @@ extern int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
struct tpm_digest *digests);
extern int tpm_send(struct tpm_chip *chip, void *cmd, size_t buflen);
extern int tpm_get_random(struct tpm_chip *chip, u8 *data, size_t max);
-extern int tpm_seal_trusted(struct tpm_chip *chip,
- struct trusted_key_payload *payload,
- struct trusted_key_options *options);
-extern int tpm_unseal_trusted(struct tpm_chip *chip,
- struct trusted_key_payload *payload,
- struct trusted_key_options *options);
extern struct tpm_chip *tpm_default_chip(void);
+extern struct tpm_chip *tpm_find_get_ops(struct tpm_chip *chip);
+extern void tpm_put_ops(struct tpm_chip *chip);
+extern ssize_t tpm_transmit_cmd(struct tpm_chip *chip, struct tpm_buf *buf,
+ size_t min_rsp_body_length, const char *desc);
+extern void tpm2_flush_context(struct tpm_chip *chip, u32 handle);
#else
static inline int tpm_is_tpm2(struct tpm_chip *chip)
{
@@ -418,21 +435,30 @@ static inline int tpm_get_random(struct tpm_chip *chip, u8 *data, size_t max)
return -ENODEV;
}
-static inline int tpm_seal_trusted(struct tpm_chip *chip,
- struct trusted_key_payload *payload,
- struct trusted_key_options *options)
+static inline struct tpm_chip *tpm_default_chip(void)
{
- return -ENODEV;
+ return NULL;
}
-static inline int tpm_unseal_trusted(struct tpm_chip *chip,
- struct trusted_key_payload *payload,
- struct trusted_key_options *options)
+
+static inline struct tpm_chip *tpm_find_get_ops(struct tpm_chip *chip)
+{
+ return NULL;
+}
+
+static inline void tpm_put_ops(struct tpm_chip *chip)
+{
+}
+
+static inline ssize_t tpm_transmit_cmd(struct tpm_chip *chip,
+ struct tpm_buf *buf,
+ size_t min_rsp_body_length,
+ const char *desc)
{
return -ENODEV;
}
-static inline struct tpm_chip *tpm_default_chip(void)
+
+static inline void tpm2_flush_context(struct tpm_chip *chip, u32 handle)
{
- return NULL;
}
#endif
#endif
diff --git a/security/keys/trusted-keys/Makefile b/security/keys/trusted-keys/Makefile
index 1a24680..7b73ceb 100644
--- a/security/keys/trusted-keys/Makefile
+++ b/security/keys/trusted-keys/Makefile
@@ -5,3 +5,4 @@
obj-$(CONFIG_TRUSTED_KEYS) += trusted.o
trusted-y += trusted_tpm1.o
+trusted-y += trusted_tpm2.o
diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
new file mode 100644
index 0000000..5477bb8
--- /dev/null
+++ b/security/keys/trusted-keys/trusted_tpm2.c
@@ -0,0 +1,368 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2004 IBM Corporation
+ * Copyright (C) 2014 Intel Corporation
+ */
+
+#include <linux/string.h>
+#include <linux/err.h>
+#include <linux/tpm.h>
+#include <linux/tpm_command.h>
+
+#include <keys/trusted-type.h>
+#include <keys/trusted_tpm.h>
+
+static struct tpm2_hash tpm2_hash_map[] = {
+ {HASH_ALGO_SHA1, TPM_ALG_SHA1},
+ {HASH_ALGO_SHA256, TPM_ALG_SHA256},
+ {HASH_ALGO_SHA384, TPM_ALG_SHA384},
+ {HASH_ALGO_SHA512, TPM_ALG_SHA512},
+ {HASH_ALGO_SM3_256, TPM_ALG_SM3_256},
+};
+
+/**
+ * tpm_buf_append_auth() - append TPMS_AUTH_COMMAND to the buffer.
+ *
+ * @buf: an allocated tpm_buf instance
+ * @session_handle: session handle
+ * @nonce: the session nonce, may be NULL if not used
+ * @nonce_len: the session nonce length, may be 0 if not used
+ * @attributes: the session attributes
+ * @hmac: the session HMAC or password, may be NULL if not used
+ * @hmac_len: the session HMAC or password length, maybe 0 if not used
+ */
+static void tpm2_buf_append_auth(struct tpm_buf *buf, u32 session_handle,
+ const u8 *nonce, u16 nonce_len,
+ u8 attributes,
+ const u8 *hmac, u16 hmac_len)
+{
+ tpm_buf_append_u32(buf, 9 + nonce_len + hmac_len);
+ tpm_buf_append_u32(buf, session_handle);
+ tpm_buf_append_u16(buf, nonce_len);
+
+ if (nonce && nonce_len)
+ tpm_buf_append(buf, nonce, nonce_len);
+
+ tpm_buf_append_u8(buf, attributes);
+ tpm_buf_append_u16(buf, hmac_len);
+
+ if (hmac && hmac_len)
+ tpm_buf_append(buf, hmac, hmac_len);
+}
+
+/**
+ * tpm2_seal_trusted() - seal the payload of a trusted key
+ *
+ * @chip: TPM chip to use
+ * @payload: the key data in clear and encrypted form
+ * @options: authentication values and other options
+ *
+ * Return: < 0 on error and 0 on success.
+ */
+int tpm2_seal_trusted(struct tpm_chip *chip,
+ struct trusted_key_payload *payload,
+ struct trusted_key_options *options)
+{
+ unsigned int blob_len;
+ struct tpm_buf buf;
+ u32 hash;
+ int i;
+ int rc;
+
+ for (i = 0; i < ARRAY_SIZE(tpm2_hash_map); i++) {
+ if (options->hash == tpm2_hash_map[i].crypto_id) {
+ hash = tpm2_hash_map[i].tpm_id;
+ break;
+ }
+ }
+
+ if (i == ARRAY_SIZE(tpm2_hash_map))
+ return -EINVAL;
+
+ rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_CREATE);
+ if (rc)
+ return rc;
+
+ tpm_buf_append_u32(&buf, options->keyhandle);
+ tpm2_buf_append_auth(&buf, TPM2_RS_PW,
+ NULL /* nonce */, 0,
+ 0 /* session_attributes */,
+ options->keyauth /* hmac */,
+ TPM_DIGEST_SIZE);
+
+ /* sensitive */
+ tpm_buf_append_u16(&buf, 4 + TPM_DIGEST_SIZE + payload->key_len + 1);
+
+ tpm_buf_append_u16(&buf, TPM_DIGEST_SIZE);
+ tpm_buf_append(&buf, options->blobauth, TPM_DIGEST_SIZE);
+ tpm_buf_append_u16(&buf, payload->key_len + 1);
+ tpm_buf_append(&buf, payload->key, payload->key_len);
+ tpm_buf_append_u8(&buf, payload->migratable);
+
+ /* public */
+ tpm_buf_append_u16(&buf, 14 + options->policydigest_len);
+ tpm_buf_append_u16(&buf, TPM_ALG_KEYEDHASH);
+ tpm_buf_append_u16(&buf, hash);
+
+ /* policy */
+ if (options->policydigest_len) {
+ tpm_buf_append_u32(&buf, 0);
+ tpm_buf_append_u16(&buf, options->policydigest_len);
+ tpm_buf_append(&buf, options->policydigest,
+ options->policydigest_len);
+ } else {
+ tpm_buf_append_u32(&buf, TPM2_OA_USER_WITH_AUTH);
+ tpm_buf_append_u16(&buf, 0);
+ }
+
+ /* public parameters */
+ tpm_buf_append_u16(&buf, TPM_ALG_NULL);
+ tpm_buf_append_u16(&buf, 0);
+
+ /* outside info */
+ tpm_buf_append_u16(&buf, 0);
+
+ /* creation PCR */
+ tpm_buf_append_u32(&buf, 0);
+
+ if (buf.flags & TPM_BUF_OVERFLOW) {
+ rc = -E2BIG;
+ goto out;
+ }
+
+ rc = tpm_transmit_cmd(chip, &buf, 4, "sealing data");
+ if (rc)
+ goto out;
+
+ blob_len = be32_to_cpup((__be32 *) &buf.data[TPM_HEADER_SIZE]);
+ if (blob_len > MAX_BLOB_SIZE) {
+ rc = -E2BIG;
+ goto out;
+ }
+ if (tpm_buf_length(&buf) < TPM_HEADER_SIZE + 4 + blob_len) {
+ rc = -EFAULT;
+ goto out;
+ }
+
+ memcpy(payload->blob, &buf.data[TPM_HEADER_SIZE + 4], blob_len);
+ payload->blob_len = blob_len;
+
+out:
+ tpm_buf_destroy(&buf);
+
+ if (rc > 0) {
+ if (tpm2_rc_value(rc) == TPM2_RC_HASH)
+ rc = -EINVAL;
+ else
+ rc = -EPERM;
+ }
+
+ return rc;
+}
+
+/**
+ * tpm_seal_trusted() - seal a trusted key payload
+ * @chip: a &struct tpm_chip instance, %NULL for the default chip
+ * @options: authentication values and other options
+ * @payload: the key data in clear and encrypted form
+ *
+ * Note: only TPM 2.0 chip are supported. TPM 1.x implementation is located in
+ * the keyring subsystem.
+ *
+ * Return: same as with tpm_transmit_cmd()
+ */
+int tpm_seal_trusted(struct tpm_chip *chip, struct trusted_key_payload *payload,
+ struct trusted_key_options *options)
+{
+ int rc;
+
+ chip = tpm_find_get_ops(chip);
+ if (!chip || !(chip->flags & TPM_CHIP_FLAG_TPM2))
+ return -ENODEV;
+
+ rc = tpm2_seal_trusted(chip, payload, options);
+
+ tpm_put_ops(chip);
+ return rc;
+}
+
+/**
+ * tpm2_load_cmd() - execute a TPM2_Load command
+ *
+ * @chip: TPM chip to use
+ * @payload: the key data in clear and encrypted form
+ * @options: authentication values and other options
+ * @blob_handle: returned blob handle
+ *
+ * Return: 0 on success.
+ * -E2BIG on wrong payload size.
+ * -EPERM on tpm error status.
+ * < 0 error from tpm_transmit_cmd.
+ */
+static int tpm2_load_cmd(struct tpm_chip *chip,
+ struct trusted_key_payload *payload,
+ struct trusted_key_options *options,
+ u32 *blob_handle)
+{
+ struct tpm_buf buf;
+ unsigned int private_len;
+ unsigned int public_len;
+ unsigned int blob_len;
+ int rc;
+
+ private_len = be16_to_cpup((__be16 *) &payload->blob[0]);
+ if (private_len > (payload->blob_len - 2))
+ return -E2BIG;
+
+ public_len = be16_to_cpup((__be16 *) &payload->blob[2 + private_len]);
+ blob_len = private_len + public_len + 4;
+ if (blob_len > payload->blob_len)
+ return -E2BIG;
+
+ rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_LOAD);
+ if (rc)
+ return rc;
+
+ tpm_buf_append_u32(&buf, options->keyhandle);
+ tpm2_buf_append_auth(&buf, TPM2_RS_PW,
+ NULL /* nonce */, 0,
+ 0 /* session_attributes */,
+ options->keyauth /* hmac */,
+ TPM_DIGEST_SIZE);
+
+ tpm_buf_append(&buf, payload->blob, blob_len);
+
+ if (buf.flags & TPM_BUF_OVERFLOW) {
+ rc = -E2BIG;
+ goto out;
+ }
+
+ rc = tpm_transmit_cmd(chip, &buf, 4, "loading blob");
+ if (!rc)
+ *blob_handle = be32_to_cpup(
+ (__be32 *) &buf.data[TPM_HEADER_SIZE]);
+
+out:
+ tpm_buf_destroy(&buf);
+
+ if (rc > 0)
+ rc = -EPERM;
+
+ return rc;
+}
+
+/**
+ * tpm2_unseal_cmd() - execute a TPM2_Unload command
+ *
+ * @chip: TPM chip to use
+ * @payload: the key data in clear and encrypted form
+ * @options: authentication values and other options
+ * @blob_handle: blob handle
+ *
+ * Return: 0 on success
+ * -EPERM on tpm error status
+ * < 0 error from tpm_transmit_cmd
+ */
+static int tpm2_unseal_cmd(struct tpm_chip *chip,
+ struct trusted_key_payload *payload,
+ struct trusted_key_options *options,
+ u32 blob_handle)
+{
+ struct tpm_buf buf;
+ u16 data_len;
+ u8 *data;
+ int rc;
+
+ rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_UNSEAL);
+ if (rc)
+ return rc;
+
+ tpm_buf_append_u32(&buf, blob_handle);
+ tpm2_buf_append_auth(&buf,
+ options->policyhandle ?
+ options->policyhandle : TPM2_RS_PW,
+ NULL /* nonce */, 0,
+ TPM2_SA_CONTINUE_SESSION,
+ options->blobauth /* hmac */,
+ TPM_DIGEST_SIZE);
+
+ rc = tpm_transmit_cmd(chip, &buf, 6, "unsealing");
+ if (rc > 0)
+ rc = -EPERM;
+
+ if (!rc) {
+ data_len = be16_to_cpup(
+ (__be16 *) &buf.data[TPM_HEADER_SIZE + 4]);
+ if (data_len < MIN_KEY_SIZE || data_len > MAX_KEY_SIZE + 1) {
+ rc = -EFAULT;
+ goto out;
+ }
+
+ if (tpm_buf_length(&buf) < TPM_HEADER_SIZE + 6 + data_len) {
+ rc = -EFAULT;
+ goto out;
+ }
+ data = &buf.data[TPM_HEADER_SIZE + 6];
+
+ memcpy(payload->key, data, data_len - 1);
+ payload->key_len = data_len - 1;
+ payload->migratable = data[data_len - 1];
+ }
+
+out:
+ tpm_buf_destroy(&buf);
+ return rc;
+}
+
+/**
+ * tpm2_unseal_trusted() - unseal the payload of a trusted key
+ *
+ * @chip: TPM chip to use
+ * @payload: the key data in clear and encrypted form
+ * @options: authentication values and other options
+ *
+ * Return: Same as with tpm_transmit_cmd.
+ */
+int tpm2_unseal_trusted(struct tpm_chip *chip,
+ struct trusted_key_payload *payload,
+ struct trusted_key_options *options)
+{
+ u32 blob_handle;
+ int rc;
+
+ rc = tpm2_load_cmd(chip, payload, options, &blob_handle);
+ if (rc)
+ return rc;
+
+ rc = tpm2_unseal_cmd(chip, payload, options, blob_handle);
+ tpm2_flush_context(chip, blob_handle);
+ return rc;
+}
+
+/**
+ * tpm_unseal_trusted() - unseal a trusted key
+ * @chip: a &struct tpm_chip instance, %NULL for the default chip
+ * @options: authentication values and other options
+ * @payload: the key data in clear and encrypted form
+ *
+ * Note: only TPM 2.0 chip are supported. TPM 1.x implementation is located in
+ * the keyring subsystem.
+ *
+ * Return: same as with tpm_transmit_cmd()
+ */
+int tpm_unseal_trusted(struct tpm_chip *chip,
+ struct trusted_key_payload *payload,
+ struct trusted_key_options *options)
+{
+ int rc;
+
+ chip = tpm_find_get_ops(chip);
+ if (!chip || !(chip->flags & TPM_CHIP_FLAG_TPM2))
+ return -ENODEV;
+
+ rc = tpm2_unseal_trusted(chip, payload, options);
+
+ tpm_put_ops(chip);
+
+ return rc;
+}
--
2.7.4
^ permalink raw reply related
* [Patch v6 3/4] KEYS: trusted: Create trusted keys subsystem
From: Sumit Garg @ 2019-09-16 10:34 UTC (permalink / raw)
To: jarkko.sakkinen, dhowells, peterhuewe
Cc: keyrings, linux-integrity, linux-crypto, linux-security-module,
herbert, davem, jgg, arnd, gregkh, jejb, zohar, jmorris, serge,
jsnitsel, linux-kernel, daniel.thompson, Sumit Garg
In-Reply-To: <1568630064-14887-1-git-send-email-sumit.garg@linaro.org>
Move existing code to trusted keys subsystem. Also, rename files with
"tpm" as suffix which provides the underlying implementation.
Suggested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
crypto/asymmetric_keys/asym_tpm.c | 2 +-
include/Kbuild | 1 -
include/keys/{trusted.h => trusted_tpm.h} | 7 +++++--
security/keys/Makefile | 2 +-
security/keys/trusted-keys/Makefile | 7 +++++++
security/keys/{trusted.c => trusted-keys/trusted_tpm1.c} | 2 +-
6 files changed, 15 insertions(+), 6 deletions(-)
rename include/keys/{trusted.h => trusted_tpm.h} (96%)
create mode 100644 security/keys/trusted-keys/Makefile
rename security/keys/{trusted.c => trusted-keys/trusted_tpm1.c} (99%)
diff --git a/crypto/asymmetric_keys/asym_tpm.c b/crypto/asymmetric_keys/asym_tpm.c
index a2b2a61..d16d893 100644
--- a/crypto/asymmetric_keys/asym_tpm.c
+++ b/crypto/asymmetric_keys/asym_tpm.c
@@ -13,7 +13,7 @@
#include <crypto/sha.h>
#include <asm/unaligned.h>
#include <keys/asymmetric-subtype.h>
-#include <keys/trusted.h>
+#include <keys/trusted_tpm.h>
#include <crypto/asym_tpm_subtype.h>
#include <crypto/public_key.h>
diff --git a/include/Kbuild b/include/Kbuild
index c38f0d4..a5801c0 100644
--- a/include/Kbuild
+++ b/include/Kbuild
@@ -65,7 +65,6 @@ header-test- += keys/asymmetric-subtype.h
header-test- += keys/asymmetric-type.h
header-test- += keys/big_key-type.h
header-test- += keys/request_key_auth-type.h
-header-test- += keys/trusted.h
header-test- += kvm/arm_arch_timer.h
header-test- += kvm/arm_pmu.h
header-test-$(CONFIG_ARM) += kvm/arm_psci.h
diff --git a/include/keys/trusted.h b/include/keys/trusted_tpm.h
similarity index 96%
rename from include/keys/trusted.h
rename to include/keys/trusted_tpm.h
index 29e3e9b..7b9d7b4 100644
--- a/include/keys/trusted.h
+++ b/include/keys/trusted_tpm.h
@@ -1,6 +1,9 @@
/* SPDX-License-Identifier: GPL-2.0 */
-#ifndef __TRUSTED_KEY_H
-#define __TRUSTED_KEY_H
+#ifndef __TRUSTED_TPM_H
+#define __TRUSTED_TPM_H
+
+#include <keys/trusted-type.h>
+#include <linux/tpm_command.h>
/* implementation specific TPM constants */
#define MAX_BUF_SIZE 1024
diff --git a/security/keys/Makefile b/security/keys/Makefile
index 9cef540..074f275 100644
--- a/security/keys/Makefile
+++ b/security/keys/Makefile
@@ -28,5 +28,5 @@ obj-$(CONFIG_ASYMMETRIC_KEY_TYPE) += keyctl_pkey.o
# Key types
#
obj-$(CONFIG_BIG_KEYS) += big_key.o
-obj-$(CONFIG_TRUSTED_KEYS) += trusted.o
+obj-$(CONFIG_TRUSTED_KEYS) += trusted-keys/
obj-$(CONFIG_ENCRYPTED_KEYS) += encrypted-keys/
diff --git a/security/keys/trusted-keys/Makefile b/security/keys/trusted-keys/Makefile
new file mode 100644
index 0000000..1a24680
--- /dev/null
+++ b/security/keys/trusted-keys/Makefile
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Makefile for trusted keys
+#
+
+obj-$(CONFIG_TRUSTED_KEYS) += trusted.o
+trusted-y += trusted_tpm1.o
diff --git a/security/keys/trusted.c b/security/keys/trusted-keys/trusted_tpm1.c
similarity index 99%
rename from security/keys/trusted.c
rename to security/keys/trusted-keys/trusted_tpm1.c
index 7071011..e3155fd 100644
--- a/security/keys/trusted.c
+++ b/security/keys/trusted-keys/trusted_tpm1.c
@@ -27,7 +27,7 @@
#include <linux/tpm.h>
#include <linux/tpm_command.h>
-#include <keys/trusted.h>
+#include <keys/trusted_tpm.h>
static const char hmac_alg[] = "hmac(sha1)";
static const char hash_alg[] = "sha1";
--
2.7.4
^ permalink raw reply related
* [Patch v6 2/4] KEYS: Use common tpm_buf for trusted and asymmetric keys
From: Sumit Garg @ 2019-09-16 10:34 UTC (permalink / raw)
To: jarkko.sakkinen, dhowells, peterhuewe
Cc: keyrings, linux-integrity, linux-crypto, linux-security-module,
herbert, davem, jgg, arnd, gregkh, jejb, zohar, jmorris, serge,
jsnitsel, linux-kernel, daniel.thompson, Sumit Garg
In-Reply-To: <1568630064-14887-1-git-send-email-sumit.garg@linaro.org>
Switch to utilize common heap based tpm_buf code for TPM based trusted
and asymmetric keys rather than using stack based tpm1_buf code. Also,
remove tpm1_buf code.
Suggested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
crypto/asymmetric_keys/asym_tpm.c | 107 ++++++++++++++++----------------------
include/keys/trusted.h | 37 +------------
security/keys/trusted.c | 98 +++++++++++++++-------------------
3 files changed, 89 insertions(+), 153 deletions(-)
diff --git a/crypto/asymmetric_keys/asym_tpm.c b/crypto/asymmetric_keys/asym_tpm.c
index b88968d..a2b2a61 100644
--- a/crypto/asymmetric_keys/asym_tpm.c
+++ b/crypto/asymmetric_keys/asym_tpm.c
@@ -21,17 +21,13 @@
#define TPM_ORD_LOADKEY2 65
#define TPM_ORD_UNBIND 30
#define TPM_ORD_SIGN 60
-#define TPM_LOADKEY2_SIZE 59
-#define TPM_FLUSHSPECIFIC_SIZE 18
-#define TPM_UNBIND_SIZE 63
-#define TPM_SIGN_SIZE 63
#define TPM_RT_KEY 0x00000001
/*
* Load a TPM key from the blob provided by userspace
*/
-static int tpm_loadkey2(struct tpm1_buf *tb,
+static int tpm_loadkey2(struct tpm_buf *tb,
uint32_t keyhandle, unsigned char *keyauth,
const unsigned char *keyblob, int keybloblen,
uint32_t *newhandle)
@@ -68,16 +64,13 @@ static int tpm_loadkey2(struct tpm1_buf *tb,
return ret;
/* build the request buffer */
- INIT_BUF(tb);
- store16(tb, TPM_TAG_RQU_AUTH1_COMMAND);
- store32(tb, TPM_LOADKEY2_SIZE + keybloblen);
- store32(tb, TPM_ORD_LOADKEY2);
- store32(tb, keyhandle);
- storebytes(tb, keyblob, keybloblen);
- store32(tb, authhandle);
- storebytes(tb, nonceodd, TPM_NONCE_SIZE);
- store8(tb, cont);
- storebytes(tb, authdata, SHA1_DIGEST_SIZE);
+ tpm_buf_reset(tb, TPM_TAG_RQU_AUTH1_COMMAND, TPM_ORD_LOADKEY2);
+ tpm_buf_append_u32(tb, keyhandle);
+ tpm_buf_append(tb, keyblob, keybloblen);
+ tpm_buf_append_u32(tb, authhandle);
+ tpm_buf_append(tb, nonceodd, TPM_NONCE_SIZE);
+ tpm_buf_append_u8(tb, cont);
+ tpm_buf_append(tb, authdata, SHA1_DIGEST_SIZE);
ret = trusted_tpm_send(tb->data, MAX_BUF_SIZE);
if (ret < 0) {
@@ -99,14 +92,11 @@ static int tpm_loadkey2(struct tpm1_buf *tb,
/*
* Execute the FlushSpecific TPM command
*/
-static int tpm_flushspecific(struct tpm1_buf *tb, uint32_t handle)
+static int tpm_flushspecific(struct tpm_buf *tb, uint32_t handle)
{
- INIT_BUF(tb);
- store16(tb, TPM_TAG_RQU_COMMAND);
- store32(tb, TPM_FLUSHSPECIFIC_SIZE);
- store32(tb, TPM_ORD_FLUSHSPECIFIC);
- store32(tb, handle);
- store32(tb, TPM_RT_KEY);
+ tpm_buf_reset(tb, TPM_TAG_RQU_COMMAND, TPM_ORD_FLUSHSPECIFIC);
+ tpm_buf_append_u32(tb, handle);
+ tpm_buf_append_u32(tb, TPM_RT_KEY);
return trusted_tpm_send(tb->data, MAX_BUF_SIZE);
}
@@ -115,7 +105,7 @@ static int tpm_flushspecific(struct tpm1_buf *tb, uint32_t handle)
* Decrypt a blob provided by userspace using a specific key handle.
* The handle is a well known handle or previously loaded by e.g. LoadKey2
*/
-static int tpm_unbind(struct tpm1_buf *tb,
+static int tpm_unbind(struct tpm_buf *tb,
uint32_t keyhandle, unsigned char *keyauth,
const unsigned char *blob, uint32_t bloblen,
void *out, uint32_t outlen)
@@ -155,17 +145,14 @@ static int tpm_unbind(struct tpm1_buf *tb,
return ret;
/* build the request buffer */
- INIT_BUF(tb);
- store16(tb, TPM_TAG_RQU_AUTH1_COMMAND);
- store32(tb, TPM_UNBIND_SIZE + bloblen);
- store32(tb, TPM_ORD_UNBIND);
- store32(tb, keyhandle);
- store32(tb, bloblen);
- storebytes(tb, blob, bloblen);
- store32(tb, authhandle);
- storebytes(tb, nonceodd, TPM_NONCE_SIZE);
- store8(tb, cont);
- storebytes(tb, authdata, SHA1_DIGEST_SIZE);
+ tpm_buf_reset(tb, TPM_TAG_RQU_AUTH1_COMMAND, TPM_ORD_UNBIND);
+ tpm_buf_append_u32(tb, keyhandle);
+ tpm_buf_append_u32(tb, bloblen);
+ tpm_buf_append(tb, blob, bloblen);
+ tpm_buf_append_u32(tb, authhandle);
+ tpm_buf_append(tb, nonceodd, TPM_NONCE_SIZE);
+ tpm_buf_append_u8(tb, cont);
+ tpm_buf_append(tb, authdata, SHA1_DIGEST_SIZE);
ret = trusted_tpm_send(tb->data, MAX_BUF_SIZE);
if (ret < 0) {
@@ -201,7 +188,7 @@ static int tpm_unbind(struct tpm1_buf *tb,
* up to key_length_in_bytes - 11 and not be limited to size 20 like the
* TPM_SS_RSASSAPKCS1v15_SHA1 signature scheme.
*/
-static int tpm_sign(struct tpm1_buf *tb,
+static int tpm_sign(struct tpm_buf *tb,
uint32_t keyhandle, unsigned char *keyauth,
const unsigned char *blob, uint32_t bloblen,
void *out, uint32_t outlen)
@@ -241,17 +228,14 @@ static int tpm_sign(struct tpm1_buf *tb,
return ret;
/* build the request buffer */
- INIT_BUF(tb);
- store16(tb, TPM_TAG_RQU_AUTH1_COMMAND);
- store32(tb, TPM_SIGN_SIZE + bloblen);
- store32(tb, TPM_ORD_SIGN);
- store32(tb, keyhandle);
- store32(tb, bloblen);
- storebytes(tb, blob, bloblen);
- store32(tb, authhandle);
- storebytes(tb, nonceodd, TPM_NONCE_SIZE);
- store8(tb, cont);
- storebytes(tb, authdata, SHA1_DIGEST_SIZE);
+ tpm_buf_reset(tb, TPM_TAG_RQU_AUTH1_COMMAND, TPM_ORD_SIGN);
+ tpm_buf_append_u32(tb, keyhandle);
+ tpm_buf_append_u32(tb, bloblen);
+ tpm_buf_append(tb, blob, bloblen);
+ tpm_buf_append_u32(tb, authhandle);
+ tpm_buf_append(tb, nonceodd, TPM_NONCE_SIZE);
+ tpm_buf_append_u8(tb, cont);
+ tpm_buf_append(tb, authdata, SHA1_DIGEST_SIZE);
ret = trusted_tpm_send(tb->data, MAX_BUF_SIZE);
if (ret < 0) {
@@ -519,7 +503,7 @@ static int tpm_key_decrypt(struct tpm_key *tk,
struct kernel_pkey_params *params,
const void *in, void *out)
{
- struct tpm1_buf *tb;
+ struct tpm_buf tb;
uint32_t keyhandle;
uint8_t srkauth[SHA1_DIGEST_SIZE];
uint8_t keyauth[SHA1_DIGEST_SIZE];
@@ -533,14 +517,14 @@ static int tpm_key_decrypt(struct tpm_key *tk,
if (strcmp(params->encoding, "pkcs1"))
return -ENOPKG;
- tb = kzalloc(sizeof(*tb), GFP_KERNEL);
- if (!tb)
- return -ENOMEM;
+ r = tpm_buf_init(&tb, 0, 0);
+ if (r)
+ return r;
/* TODO: Handle a non-all zero SRK authorization */
memset(srkauth, 0, sizeof(srkauth));
- r = tpm_loadkey2(tb, SRKHANDLE, srkauth,
+ r = tpm_loadkey2(&tb, SRKHANDLE, srkauth,
tk->blob, tk->blob_len, &keyhandle);
if (r < 0) {
pr_devel("loadkey2 failed (%d)\n", r);
@@ -550,16 +534,16 @@ static int tpm_key_decrypt(struct tpm_key *tk,
/* TODO: Handle a non-all zero key authorization */
memset(keyauth, 0, sizeof(keyauth));
- r = tpm_unbind(tb, keyhandle, keyauth,
+ r = tpm_unbind(&tb, keyhandle, keyauth,
in, params->in_len, out, params->out_len);
if (r < 0)
pr_devel("tpm_unbind failed (%d)\n", r);
- if (tpm_flushspecific(tb, keyhandle) < 0)
+ if (tpm_flushspecific(&tb, keyhandle) < 0)
pr_devel("flushspecific failed (%d)\n", r);
error:
- kzfree(tb);
+ tpm_buf_destroy(&tb);
pr_devel("<==%s() = %d\n", __func__, r);
return r;
}
@@ -643,7 +627,7 @@ static int tpm_key_sign(struct tpm_key *tk,
struct kernel_pkey_params *params,
const void *in, void *out)
{
- struct tpm1_buf *tb;
+ struct tpm_buf tb;
uint32_t keyhandle;
uint8_t srkauth[SHA1_DIGEST_SIZE];
uint8_t keyauth[SHA1_DIGEST_SIZE];
@@ -681,15 +665,14 @@ static int tpm_key_sign(struct tpm_key *tk,
goto error_free_asn1_wrapped;
}
- r = -ENOMEM;
- tb = kzalloc(sizeof(*tb), GFP_KERNEL);
- if (!tb)
+ r = tpm_buf_init(&tb, 0, 0);
+ if (r)
goto error_free_asn1_wrapped;
/* TODO: Handle a non-all zero SRK authorization */
memset(srkauth, 0, sizeof(srkauth));
- r = tpm_loadkey2(tb, SRKHANDLE, srkauth,
+ r = tpm_loadkey2(&tb, SRKHANDLE, srkauth,
tk->blob, tk->blob_len, &keyhandle);
if (r < 0) {
pr_devel("loadkey2 failed (%d)\n", r);
@@ -699,15 +682,15 @@ static int tpm_key_sign(struct tpm_key *tk,
/* TODO: Handle a non-all zero key authorization */
memset(keyauth, 0, sizeof(keyauth));
- r = tpm_sign(tb, keyhandle, keyauth, in, in_len, out, params->out_len);
+ r = tpm_sign(&tb, keyhandle, keyauth, in, in_len, out, params->out_len);
if (r < 0)
pr_devel("tpm_sign failed (%d)\n", r);
- if (tpm_flushspecific(tb, keyhandle) < 0)
+ if (tpm_flushspecific(&tb, keyhandle) < 0)
pr_devel("flushspecific failed (%d)\n", r);
error_free_tb:
- kzfree(tb);
+ tpm_buf_destroy(&tb);
error_free_asn1_wrapped:
kfree(asn1_wrapped);
pr_devel("<==%s() = %d\n", __func__, r);
diff --git a/include/keys/trusted.h b/include/keys/trusted.h
index 841ae11..29e3e9b 100644
--- a/include/keys/trusted.h
+++ b/include/keys/trusted.h
@@ -5,10 +5,6 @@
/* implementation specific TPM constants */
#define MAX_BUF_SIZE 1024
#define TPM_GETRANDOM_SIZE 14
-#define TPM_OSAP_SIZE 36
-#define TPM_OIAP_SIZE 10
-#define TPM_SEAL_SIZE 87
-#define TPM_UNSEAL_SIZE 104
#define TPM_SIZE_OFFSET 2
#define TPM_RETURN_OFFSET 6
#define TPM_DATA_OFFSET 10
@@ -17,13 +13,6 @@
#define LOAD32N(buffer, offset) (*(uint32_t *)&buffer[offset])
#define LOAD16(buffer, offset) (ntohs(*(uint16_t *)&buffer[offset]))
-struct tpm1_buf {
- int len;
- unsigned char data[MAX_BUF_SIZE];
-};
-
-#define INIT_BUF(tb) (tb->len = 0)
-
struct osapsess {
uint32_t handle;
unsigned char secret[SHA1_DIGEST_SIZE];
@@ -46,7 +35,7 @@ int TSS_checkhmac1(unsigned char *buffer,
unsigned int keylen, ...);
int trusted_tpm_send(unsigned char *cmd, size_t buflen);
-int oiap(struct tpm1_buf *tb, uint32_t *handle, unsigned char *nonce);
+int oiap(struct tpm_buf *tb, uint32_t *handle, unsigned char *nonce);
#define TPM_DEBUG 0
@@ -109,28 +98,4 @@ static inline void dump_tpm_buf(unsigned char *buf)
{
}
#endif
-
-static inline void store8(struct tpm1_buf *buf, const unsigned char value)
-{
- buf->data[buf->len++] = value;
-}
-
-static inline void store16(struct tpm1_buf *buf, const uint16_t value)
-{
- *(uint16_t *) & buf->data[buf->len] = htons(value);
- buf->len += sizeof value;
-}
-
-static inline void store32(struct tpm1_buf *buf, const uint32_t value)
-{
- *(uint32_t *) & buf->data[buf->len] = htonl(value);
- buf->len += sizeof value;
-}
-
-static inline void storebytes(struct tpm1_buf *buf, const unsigned char *in,
- const int len)
-{
- memcpy(buf->data + buf->len, in, len);
- buf->len += len;
-}
#endif
diff --git a/security/keys/trusted.c b/security/keys/trusted.c
index 4cfae208..7071011 100644
--- a/security/keys/trusted.c
+++ b/security/keys/trusted.c
@@ -395,7 +395,7 @@ static int pcrlock(const int pcrnum)
/*
* Create an object specific authorisation protocol (OSAP) session
*/
-static int osap(struct tpm1_buf *tb, struct osapsess *s,
+static int osap(struct tpm_buf *tb, struct osapsess *s,
const unsigned char *key, uint16_t type, uint32_t handle)
{
unsigned char enonce[TPM_NONCE_SIZE];
@@ -406,13 +406,10 @@ static int osap(struct tpm1_buf *tb, struct osapsess *s,
if (ret != TPM_NONCE_SIZE)
return ret;
- INIT_BUF(tb);
- store16(tb, TPM_TAG_RQU_COMMAND);
- store32(tb, TPM_OSAP_SIZE);
- store32(tb, TPM_ORD_OSAP);
- store16(tb, type);
- store32(tb, handle);
- storebytes(tb, ononce, TPM_NONCE_SIZE);
+ tpm_buf_reset(tb, TPM_TAG_RQU_COMMAND, TPM_ORD_OSAP);
+ tpm_buf_append_u16(tb, type);
+ tpm_buf_append_u32(tb, handle);
+ tpm_buf_append(tb, ononce, TPM_NONCE_SIZE);
ret = trusted_tpm_send(tb->data, MAX_BUF_SIZE);
if (ret < 0)
@@ -430,17 +427,14 @@ static int osap(struct tpm1_buf *tb, struct osapsess *s,
/*
* Create an object independent authorisation protocol (oiap) session
*/
-int oiap(struct tpm1_buf *tb, uint32_t *handle, unsigned char *nonce)
+int oiap(struct tpm_buf *tb, uint32_t *handle, unsigned char *nonce)
{
int ret;
if (!chip)
return -ENODEV;
- INIT_BUF(tb);
- store16(tb, TPM_TAG_RQU_COMMAND);
- store32(tb, TPM_OIAP_SIZE);
- store32(tb, TPM_ORD_OIAP);
+ tpm_buf_reset(tb, TPM_TAG_RQU_COMMAND, TPM_ORD_OIAP);
ret = trusted_tpm_send(tb->data, MAX_BUF_SIZE);
if (ret < 0)
return ret;
@@ -464,7 +458,7 @@ struct tpm_digests {
* Have the TPM seal(encrypt) the trusted key, possibly based on
* Platform Configuration Registers (PCRs). AUTH1 for sealing key.
*/
-static int tpm_seal(struct tpm1_buf *tb, uint16_t keytype,
+static int tpm_seal(struct tpm_buf *tb, uint16_t keytype,
uint32_t keyhandle, const unsigned char *keyauth,
const unsigned char *data, uint32_t datalen,
unsigned char *blob, uint32_t *bloblen,
@@ -535,20 +529,17 @@ static int tpm_seal(struct tpm1_buf *tb, uint16_t keytype,
goto out;
/* build and send the TPM request packet */
- INIT_BUF(tb);
- store16(tb, TPM_TAG_RQU_AUTH1_COMMAND);
- store32(tb, TPM_SEAL_SIZE + pcrinfosize + datalen);
- store32(tb, TPM_ORD_SEAL);
- store32(tb, keyhandle);
- storebytes(tb, td->encauth, SHA1_DIGEST_SIZE);
- store32(tb, pcrinfosize);
- storebytes(tb, pcrinfo, pcrinfosize);
- store32(tb, datalen);
- storebytes(tb, data, datalen);
- store32(tb, sess.handle);
- storebytes(tb, td->nonceodd, TPM_NONCE_SIZE);
- store8(tb, cont);
- storebytes(tb, td->pubauth, SHA1_DIGEST_SIZE);
+ tpm_buf_reset(tb, TPM_TAG_RQU_AUTH1_COMMAND, TPM_ORD_SEAL);
+ tpm_buf_append_u32(tb, keyhandle);
+ tpm_buf_append(tb, td->encauth, SHA1_DIGEST_SIZE);
+ tpm_buf_append_u32(tb, pcrinfosize);
+ tpm_buf_append(tb, pcrinfo, pcrinfosize);
+ tpm_buf_append_u32(tb, datalen);
+ tpm_buf_append(tb, data, datalen);
+ tpm_buf_append_u32(tb, sess.handle);
+ tpm_buf_append(tb, td->nonceodd, TPM_NONCE_SIZE);
+ tpm_buf_append_u8(tb, cont);
+ tpm_buf_append(tb, td->pubauth, SHA1_DIGEST_SIZE);
ret = trusted_tpm_send(tb->data, MAX_BUF_SIZE);
if (ret < 0)
@@ -579,7 +570,7 @@ static int tpm_seal(struct tpm1_buf *tb, uint16_t keytype,
/*
* use the AUTH2_COMMAND form of unseal, to authorize both key and blob
*/
-static int tpm_unseal(struct tpm1_buf *tb,
+static int tpm_unseal(struct tpm_buf *tb,
uint32_t keyhandle, const unsigned char *keyauth,
const unsigned char *blob, int bloblen,
const unsigned char *blobauth,
@@ -628,20 +619,17 @@ static int tpm_unseal(struct tpm1_buf *tb,
return ret;
/* build and send TPM request packet */
- INIT_BUF(tb);
- store16(tb, TPM_TAG_RQU_AUTH2_COMMAND);
- store32(tb, TPM_UNSEAL_SIZE + bloblen);
- store32(tb, TPM_ORD_UNSEAL);
- store32(tb, keyhandle);
- storebytes(tb, blob, bloblen);
- store32(tb, authhandle1);
- storebytes(tb, nonceodd, TPM_NONCE_SIZE);
- store8(tb, cont);
- storebytes(tb, authdata1, SHA1_DIGEST_SIZE);
- store32(tb, authhandle2);
- storebytes(tb, nonceodd, TPM_NONCE_SIZE);
- store8(tb, cont);
- storebytes(tb, authdata2, SHA1_DIGEST_SIZE);
+ tpm_buf_reset(tb, TPM_TAG_RQU_AUTH2_COMMAND, TPM_ORD_UNSEAL);
+ tpm_buf_append_u32(tb, keyhandle);
+ tpm_buf_append(tb, blob, bloblen);
+ tpm_buf_append_u32(tb, authhandle1);
+ tpm_buf_append(tb, nonceodd, TPM_NONCE_SIZE);
+ tpm_buf_append_u8(tb, cont);
+ tpm_buf_append(tb, authdata1, SHA1_DIGEST_SIZE);
+ tpm_buf_append_u32(tb, authhandle2);
+ tpm_buf_append(tb, nonceodd, TPM_NONCE_SIZE);
+ tpm_buf_append_u8(tb, cont);
+ tpm_buf_append(tb, authdata2, SHA1_DIGEST_SIZE);
ret = trusted_tpm_send(tb->data, MAX_BUF_SIZE);
if (ret < 0) {
@@ -670,23 +658,23 @@ static int tpm_unseal(struct tpm1_buf *tb,
static int key_seal(struct trusted_key_payload *p,
struct trusted_key_options *o)
{
- struct tpm1_buf *tb;
+ struct tpm_buf tb;
int ret;
- tb = kzalloc(sizeof *tb, GFP_KERNEL);
- if (!tb)
- return -ENOMEM;
+ ret = tpm_buf_init(&tb, 0, 0);
+ if (ret)
+ return ret;
/* include migratable flag at end of sealed key */
p->key[p->key_len] = p->migratable;
- ret = tpm_seal(tb, o->keytype, o->keyhandle, o->keyauth,
+ ret = tpm_seal(&tb, o->keytype, o->keyhandle, o->keyauth,
p->key, p->key_len + 1, p->blob, &p->blob_len,
o->blobauth, o->pcrinfo, o->pcrinfo_len);
if (ret < 0)
pr_info("trusted_key: srkseal failed (%d)\n", ret);
- kzfree(tb);
+ tpm_buf_destroy(&tb);
return ret;
}
@@ -696,14 +684,14 @@ static int key_seal(struct trusted_key_payload *p,
static int key_unseal(struct trusted_key_payload *p,
struct trusted_key_options *o)
{
- struct tpm1_buf *tb;
+ struct tpm_buf tb;
int ret;
- tb = kzalloc(sizeof *tb, GFP_KERNEL);
- if (!tb)
- return -ENOMEM;
+ ret = tpm_buf_init(&tb, 0, 0);
+ if (ret)
+ return ret;
- ret = tpm_unseal(tb, o->keyhandle, o->keyauth, p->blob, p->blob_len,
+ ret = tpm_unseal(&tb, o->keyhandle, o->keyauth, p->blob, p->blob_len,
o->blobauth, p->key, &p->key_len);
if (ret < 0)
pr_info("trusted_key: srkunseal failed (%d)\n", ret);
@@ -711,7 +699,7 @@ static int key_unseal(struct trusted_key_payload *p,
/* pull migratable flag out of sealed key */
p->migratable = p->key[--p->key_len];
- kzfree(tb);
+ tpm_buf_destroy(&tb);
return ret;
}
--
2.7.4
^ permalink raw reply related
* [Patch v6 1/4] tpm: Move tpm_buf code to include/linux/
From: Sumit Garg @ 2019-09-16 10:34 UTC (permalink / raw)
To: jarkko.sakkinen, dhowells, peterhuewe
Cc: keyrings, linux-integrity, linux-crypto, linux-security-module,
herbert, davem, jgg, arnd, gregkh, jejb, zohar, jmorris, serge,
jsnitsel, linux-kernel, daniel.thompson, Sumit Garg
In-Reply-To: <1568630064-14887-1-git-send-email-sumit.garg@linaro.org>
Move tpm_buf code to common include/linux/tpm.h header so that it can
be reused via other subsystems like trusted keys etc.
Also rename trusted keys and asymmetric keys usage of TPM 1.x buffer
implementation to tpm1_buf to avoid any compilation errors.
Suggested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
crypto/asymmetric_keys/asym_tpm.c | 12 +--
drivers/char/tpm/tpm.h | 214 --------------------------------------
include/keys/trusted.h | 12 +--
include/linux/tpm.h | 214 ++++++++++++++++++++++++++++++++++++++
security/keys/trusted.c | 12 +--
5 files changed, 232 insertions(+), 232 deletions(-)
diff --git a/crypto/asymmetric_keys/asym_tpm.c b/crypto/asymmetric_keys/asym_tpm.c
index 76d2ce3..b88968d 100644
--- a/crypto/asymmetric_keys/asym_tpm.c
+++ b/crypto/asymmetric_keys/asym_tpm.c
@@ -31,7 +31,7 @@
/*
* Load a TPM key from the blob provided by userspace
*/
-static int tpm_loadkey2(struct tpm_buf *tb,
+static int tpm_loadkey2(struct tpm1_buf *tb,
uint32_t keyhandle, unsigned char *keyauth,
const unsigned char *keyblob, int keybloblen,
uint32_t *newhandle)
@@ -99,7 +99,7 @@ static int tpm_loadkey2(struct tpm_buf *tb,
/*
* Execute the FlushSpecific TPM command
*/
-static int tpm_flushspecific(struct tpm_buf *tb, uint32_t handle)
+static int tpm_flushspecific(struct tpm1_buf *tb, uint32_t handle)
{
INIT_BUF(tb);
store16(tb, TPM_TAG_RQU_COMMAND);
@@ -115,7 +115,7 @@ static int tpm_flushspecific(struct tpm_buf *tb, uint32_t handle)
* Decrypt a blob provided by userspace using a specific key handle.
* The handle is a well known handle or previously loaded by e.g. LoadKey2
*/
-static int tpm_unbind(struct tpm_buf *tb,
+static int tpm_unbind(struct tpm1_buf *tb,
uint32_t keyhandle, unsigned char *keyauth,
const unsigned char *blob, uint32_t bloblen,
void *out, uint32_t outlen)
@@ -201,7 +201,7 @@ static int tpm_unbind(struct tpm_buf *tb,
* up to key_length_in_bytes - 11 and not be limited to size 20 like the
* TPM_SS_RSASSAPKCS1v15_SHA1 signature scheme.
*/
-static int tpm_sign(struct tpm_buf *tb,
+static int tpm_sign(struct tpm1_buf *tb,
uint32_t keyhandle, unsigned char *keyauth,
const unsigned char *blob, uint32_t bloblen,
void *out, uint32_t outlen)
@@ -519,7 +519,7 @@ static int tpm_key_decrypt(struct tpm_key *tk,
struct kernel_pkey_params *params,
const void *in, void *out)
{
- struct tpm_buf *tb;
+ struct tpm1_buf *tb;
uint32_t keyhandle;
uint8_t srkauth[SHA1_DIGEST_SIZE];
uint8_t keyauth[SHA1_DIGEST_SIZE];
@@ -643,7 +643,7 @@ static int tpm_key_sign(struct tpm_key *tk,
struct kernel_pkey_params *params,
const void *in, void *out)
{
- struct tpm_buf *tb;
+ struct tpm1_buf *tb;
uint32_t keyhandle;
uint8_t srkauth[SHA1_DIGEST_SIZE];
uint8_t keyauth[SHA1_DIGEST_SIZE];
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index a7fea3e..28403a4 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -25,7 +25,6 @@
#include <linux/platform_device.h>
#include <linux/io.h>
#include <linux/tpm.h>
-#include <linux/highmem.h>
#include <linux/tpm_eventlog.h>
#ifdef CONFIG_X86
@@ -58,123 +57,6 @@ enum tpm_addr {
#define TPM_ERR_DISABLED 0x7
#define TPM_ERR_INVALID_POSTINIT 38
-#define TPM_HEADER_SIZE 10
-
-enum tpm2_const {
- TPM2_PLATFORM_PCR = 24,
- TPM2_PCR_SELECT_MIN = ((TPM2_PLATFORM_PCR + 7) / 8),
-};
-
-enum tpm2_timeouts {
- TPM2_TIMEOUT_A = 750,
- TPM2_TIMEOUT_B = 2000,
- TPM2_TIMEOUT_C = 200,
- TPM2_TIMEOUT_D = 30,
- TPM2_DURATION_SHORT = 20,
- TPM2_DURATION_MEDIUM = 750,
- TPM2_DURATION_LONG = 2000,
- TPM2_DURATION_LONG_LONG = 300000,
- TPM2_DURATION_DEFAULT = 120000,
-};
-
-enum tpm2_structures {
- TPM2_ST_NO_SESSIONS = 0x8001,
- TPM2_ST_SESSIONS = 0x8002,
-};
-
-/* Indicates from what layer of the software stack the error comes from */
-#define TSS2_RC_LAYER_SHIFT 16
-#define TSS2_RESMGR_TPM_RC_LAYER (11 << TSS2_RC_LAYER_SHIFT)
-
-enum tpm2_return_codes {
- TPM2_RC_SUCCESS = 0x0000,
- TPM2_RC_HASH = 0x0083, /* RC_FMT1 */
- TPM2_RC_HANDLE = 0x008B,
- TPM2_RC_INITIALIZE = 0x0100, /* RC_VER1 */
- TPM2_RC_FAILURE = 0x0101,
- TPM2_RC_DISABLED = 0x0120,
- TPM2_RC_COMMAND_CODE = 0x0143,
- TPM2_RC_TESTING = 0x090A, /* RC_WARN */
- TPM2_RC_REFERENCE_H0 = 0x0910,
- TPM2_RC_RETRY = 0x0922,
-};
-
-enum tpm2_command_codes {
- TPM2_CC_FIRST = 0x011F,
- TPM2_CC_HIERARCHY_CONTROL = 0x0121,
- TPM2_CC_HIERARCHY_CHANGE_AUTH = 0x0129,
- TPM2_CC_CREATE_PRIMARY = 0x0131,
- TPM2_CC_SEQUENCE_COMPLETE = 0x013E,
- TPM2_CC_SELF_TEST = 0x0143,
- TPM2_CC_STARTUP = 0x0144,
- TPM2_CC_SHUTDOWN = 0x0145,
- TPM2_CC_NV_READ = 0x014E,
- TPM2_CC_CREATE = 0x0153,
- TPM2_CC_LOAD = 0x0157,
- TPM2_CC_SEQUENCE_UPDATE = 0x015C,
- TPM2_CC_UNSEAL = 0x015E,
- TPM2_CC_CONTEXT_LOAD = 0x0161,
- TPM2_CC_CONTEXT_SAVE = 0x0162,
- TPM2_CC_FLUSH_CONTEXT = 0x0165,
- TPM2_CC_VERIFY_SIGNATURE = 0x0177,
- TPM2_CC_GET_CAPABILITY = 0x017A,
- TPM2_CC_GET_RANDOM = 0x017B,
- TPM2_CC_PCR_READ = 0x017E,
- TPM2_CC_PCR_EXTEND = 0x0182,
- TPM2_CC_EVENT_SEQUENCE_COMPLETE = 0x0185,
- TPM2_CC_HASH_SEQUENCE_START = 0x0186,
- TPM2_CC_CREATE_LOADED = 0x0191,
- TPM2_CC_LAST = 0x0193, /* Spec 1.36 */
-};
-
-enum tpm2_permanent_handles {
- TPM2_RS_PW = 0x40000009,
-};
-
-enum tpm2_capabilities {
- TPM2_CAP_HANDLES = 1,
- TPM2_CAP_COMMANDS = 2,
- TPM2_CAP_PCRS = 5,
- TPM2_CAP_TPM_PROPERTIES = 6,
-};
-
-enum tpm2_properties {
- TPM_PT_TOTAL_COMMANDS = 0x0129,
-};
-
-enum tpm2_startup_types {
- TPM2_SU_CLEAR = 0x0000,
- TPM2_SU_STATE = 0x0001,
-};
-
-enum tpm2_cc_attrs {
- TPM2_CC_ATTR_CHANDLES = 25,
- TPM2_CC_ATTR_RHANDLE = 28,
-};
-
-#define TPM_VID_INTEL 0x8086
-#define TPM_VID_WINBOND 0x1050
-#define TPM_VID_STM 0x104A
-
-enum tpm_chip_flags {
- TPM_CHIP_FLAG_TPM2 = BIT(1),
- TPM_CHIP_FLAG_IRQ = BIT(2),
- TPM_CHIP_FLAG_VIRTUAL = BIT(3),
- TPM_CHIP_FLAG_HAVE_TIMEOUTS = BIT(4),
- TPM_CHIP_FLAG_ALWAYS_POWERED = BIT(5),
-};
-
-#define to_tpm_chip(d) container_of(d, struct tpm_chip, dev)
-
-struct tpm_header {
- __be16 tag;
- __be32 length;
- union {
- __be32 ordinal;
- __be32 return_code;
- };
-} __packed;
-
#define TPM_TAG_RQU_COMMAND 193
struct stclear_flags_t {
@@ -274,102 +156,6 @@ enum tpm_sub_capabilities {
* compiler warnings about stack frame size. */
#define TPM_MAX_RNG_DATA 128
-/* A string buffer type for constructing TPM commands. This is based on the
- * ideas of string buffer code in security/keys/trusted.h but is heap based
- * in order to keep the stack usage minimal.
- */
-
-enum tpm_buf_flags {
- TPM_BUF_OVERFLOW = BIT(0),
-};
-
-struct tpm_buf {
- struct page *data_page;
- unsigned int flags;
- u8 *data;
-};
-
-static inline void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal)
-{
- struct tpm_header *head = (struct tpm_header *)buf->data;
-
- head->tag = cpu_to_be16(tag);
- head->length = cpu_to_be32(sizeof(*head));
- head->ordinal = cpu_to_be32(ordinal);
-}
-
-static inline int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal)
-{
- buf->data_page = alloc_page(GFP_HIGHUSER);
- if (!buf->data_page)
- return -ENOMEM;
-
- buf->flags = 0;
- buf->data = kmap(buf->data_page);
- tpm_buf_reset(buf, tag, ordinal);
- return 0;
-}
-
-static inline void tpm_buf_destroy(struct tpm_buf *buf)
-{
- kunmap(buf->data_page);
- __free_page(buf->data_page);
-}
-
-static inline u32 tpm_buf_length(struct tpm_buf *buf)
-{
- struct tpm_header *head = (struct tpm_header *)buf->data;
-
- return be32_to_cpu(head->length);
-}
-
-static inline u16 tpm_buf_tag(struct tpm_buf *buf)
-{
- struct tpm_header *head = (struct tpm_header *)buf->data;
-
- return be16_to_cpu(head->tag);
-}
-
-static inline void tpm_buf_append(struct tpm_buf *buf,
- const unsigned char *new_data,
- unsigned int new_len)
-{
- struct tpm_header *head = (struct tpm_header *)buf->data;
- u32 len = tpm_buf_length(buf);
-
- /* Return silently if overflow has already happened. */
- if (buf->flags & TPM_BUF_OVERFLOW)
- return;
-
- if ((len + new_len) > PAGE_SIZE) {
- WARN(1, "tpm_buf: overflow\n");
- buf->flags |= TPM_BUF_OVERFLOW;
- return;
- }
-
- memcpy(&buf->data[len], new_data, new_len);
- head->length = cpu_to_be32(len + new_len);
-}
-
-static inline void tpm_buf_append_u8(struct tpm_buf *buf, const u8 value)
-{
- tpm_buf_append(buf, &value, 1);
-}
-
-static inline void tpm_buf_append_u16(struct tpm_buf *buf, const u16 value)
-{
- __be16 value2 = cpu_to_be16(value);
-
- tpm_buf_append(buf, (u8 *) &value2, 2);
-}
-
-static inline void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value)
-{
- __be32 value2 = cpu_to_be32(value);
-
- tpm_buf_append(buf, (u8 *) &value2, 4);
-}
-
extern struct class *tpm_class;
extern struct class *tpmrm_class;
extern dev_t tpm_devt;
diff --git a/include/keys/trusted.h b/include/keys/trusted.h
index 0071298..841ae11 100644
--- a/include/keys/trusted.h
+++ b/include/keys/trusted.h
@@ -17,7 +17,7 @@
#define LOAD32N(buffer, offset) (*(uint32_t *)&buffer[offset])
#define LOAD16(buffer, offset) (ntohs(*(uint16_t *)&buffer[offset]))
-struct tpm_buf {
+struct tpm1_buf {
int len;
unsigned char data[MAX_BUF_SIZE];
};
@@ -46,7 +46,7 @@ int TSS_checkhmac1(unsigned char *buffer,
unsigned int keylen, ...);
int trusted_tpm_send(unsigned char *cmd, size_t buflen);
-int oiap(struct tpm_buf *tb, uint32_t *handle, unsigned char *nonce);
+int oiap(struct tpm1_buf *tb, uint32_t *handle, unsigned char *nonce);
#define TPM_DEBUG 0
@@ -110,24 +110,24 @@ static inline void dump_tpm_buf(unsigned char *buf)
}
#endif
-static inline void store8(struct tpm_buf *buf, const unsigned char value)
+static inline void store8(struct tpm1_buf *buf, const unsigned char value)
{
buf->data[buf->len++] = value;
}
-static inline void store16(struct tpm_buf *buf, const uint16_t value)
+static inline void store16(struct tpm1_buf *buf, const uint16_t value)
{
*(uint16_t *) & buf->data[buf->len] = htons(value);
buf->len += sizeof value;
}
-static inline void store32(struct tpm_buf *buf, const uint32_t value)
+static inline void store32(struct tpm1_buf *buf, const uint32_t value)
{
*(uint32_t *) & buf->data[buf->len] = htonl(value);
buf->len += sizeof value;
}
-static inline void storebytes(struct tpm_buf *buf, const unsigned char *in,
+static inline void storebytes(struct tpm1_buf *buf, const unsigned char *in,
const int len)
{
memcpy(buf->data + buf->len, in, len);
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 53c0ea9..130c167 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -21,6 +21,7 @@
#include <linux/acpi.h>
#include <linux/cdev.h>
#include <linux/fs.h>
+#include <linux/highmem.h>
#include <crypto/hash_info.h>
#define TPM_DIGEST_SIZE 20 /* Max TPM v1.2 PCR size */
@@ -161,6 +162,219 @@ struct tpm_chip {
int locality;
};
+#define TPM_HEADER_SIZE 10
+
+enum tpm2_const {
+ TPM2_PLATFORM_PCR = 24,
+ TPM2_PCR_SELECT_MIN = ((TPM2_PLATFORM_PCR + 7) / 8),
+};
+
+enum tpm2_timeouts {
+ TPM2_TIMEOUT_A = 750,
+ TPM2_TIMEOUT_B = 2000,
+ TPM2_TIMEOUT_C = 200,
+ TPM2_TIMEOUT_D = 30,
+ TPM2_DURATION_SHORT = 20,
+ TPM2_DURATION_MEDIUM = 750,
+ TPM2_DURATION_LONG = 2000,
+ TPM2_DURATION_LONG_LONG = 300000,
+ TPM2_DURATION_DEFAULT = 120000,
+};
+
+enum tpm2_structures {
+ TPM2_ST_NO_SESSIONS = 0x8001,
+ TPM2_ST_SESSIONS = 0x8002,
+};
+
+/* Indicates from what layer of the software stack the error comes from */
+#define TSS2_RC_LAYER_SHIFT 16
+#define TSS2_RESMGR_TPM_RC_LAYER (11 << TSS2_RC_LAYER_SHIFT)
+
+enum tpm2_return_codes {
+ TPM2_RC_SUCCESS = 0x0000,
+ TPM2_RC_HASH = 0x0083, /* RC_FMT1 */
+ TPM2_RC_HANDLE = 0x008B,
+ TPM2_RC_INITIALIZE = 0x0100, /* RC_VER1 */
+ TPM2_RC_FAILURE = 0x0101,
+ TPM2_RC_DISABLED = 0x0120,
+ TPM2_RC_COMMAND_CODE = 0x0143,
+ TPM2_RC_TESTING = 0x090A, /* RC_WARN */
+ TPM2_RC_REFERENCE_H0 = 0x0910,
+ TPM2_RC_RETRY = 0x0922,
+};
+
+enum tpm2_command_codes {
+ TPM2_CC_FIRST = 0x011F,
+ TPM2_CC_HIERARCHY_CONTROL = 0x0121,
+ TPM2_CC_HIERARCHY_CHANGE_AUTH = 0x0129,
+ TPM2_CC_CREATE_PRIMARY = 0x0131,
+ TPM2_CC_SEQUENCE_COMPLETE = 0x013E,
+ TPM2_CC_SELF_TEST = 0x0143,
+ TPM2_CC_STARTUP = 0x0144,
+ TPM2_CC_SHUTDOWN = 0x0145,
+ TPM2_CC_NV_READ = 0x014E,
+ TPM2_CC_CREATE = 0x0153,
+ TPM2_CC_LOAD = 0x0157,
+ TPM2_CC_SEQUENCE_UPDATE = 0x015C,
+ TPM2_CC_UNSEAL = 0x015E,
+ TPM2_CC_CONTEXT_LOAD = 0x0161,
+ TPM2_CC_CONTEXT_SAVE = 0x0162,
+ TPM2_CC_FLUSH_CONTEXT = 0x0165,
+ TPM2_CC_VERIFY_SIGNATURE = 0x0177,
+ TPM2_CC_GET_CAPABILITY = 0x017A,
+ TPM2_CC_GET_RANDOM = 0x017B,
+ TPM2_CC_PCR_READ = 0x017E,
+ TPM2_CC_PCR_EXTEND = 0x0182,
+ TPM2_CC_EVENT_SEQUENCE_COMPLETE = 0x0185,
+ TPM2_CC_HASH_SEQUENCE_START = 0x0186,
+ TPM2_CC_CREATE_LOADED = 0x0191,
+ TPM2_CC_LAST = 0x0193, /* Spec 1.36 */
+};
+
+enum tpm2_permanent_handles {
+ TPM2_RS_PW = 0x40000009,
+};
+
+enum tpm2_capabilities {
+ TPM2_CAP_HANDLES = 1,
+ TPM2_CAP_COMMANDS = 2,
+ TPM2_CAP_PCRS = 5,
+ TPM2_CAP_TPM_PROPERTIES = 6,
+};
+
+enum tpm2_properties {
+ TPM_PT_TOTAL_COMMANDS = 0x0129,
+};
+
+enum tpm2_startup_types {
+ TPM2_SU_CLEAR = 0x0000,
+ TPM2_SU_STATE = 0x0001,
+};
+
+enum tpm2_cc_attrs {
+ TPM2_CC_ATTR_CHANDLES = 25,
+ TPM2_CC_ATTR_RHANDLE = 28,
+};
+
+#define TPM_VID_INTEL 0x8086
+#define TPM_VID_WINBOND 0x1050
+#define TPM_VID_STM 0x104A
+
+enum tpm_chip_flags {
+ TPM_CHIP_FLAG_TPM2 = BIT(1),
+ TPM_CHIP_FLAG_IRQ = BIT(2),
+ TPM_CHIP_FLAG_VIRTUAL = BIT(3),
+ TPM_CHIP_FLAG_HAVE_TIMEOUTS = BIT(4),
+ TPM_CHIP_FLAG_ALWAYS_POWERED = BIT(5),
+};
+
+#define to_tpm_chip(d) container_of(d, struct tpm_chip, dev)
+
+struct tpm_header {
+ __be16 tag;
+ __be32 length;
+ union {
+ __be32 ordinal;
+ __be32 return_code;
+ };
+} __packed;
+
+/* A string buffer type for constructing TPM commands. This is based on the
+ * ideas of string buffer code in security/keys/trusted.h but is heap based
+ * in order to keep the stack usage minimal.
+ */
+
+enum tpm_buf_flags {
+ TPM_BUF_OVERFLOW = BIT(0),
+};
+
+struct tpm_buf {
+ struct page *data_page;
+ unsigned int flags;
+ u8 *data;
+};
+
+static inline void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal)
+{
+ struct tpm_header *head = (struct tpm_header *)buf->data;
+
+ head->tag = cpu_to_be16(tag);
+ head->length = cpu_to_be32(sizeof(*head));
+ head->ordinal = cpu_to_be32(ordinal);
+}
+
+static inline int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal)
+{
+ buf->data_page = alloc_page(GFP_HIGHUSER);
+ if (!buf->data_page)
+ return -ENOMEM;
+
+ buf->flags = 0;
+ buf->data = kmap(buf->data_page);
+ tpm_buf_reset(buf, tag, ordinal);
+ return 0;
+}
+
+static inline void tpm_buf_destroy(struct tpm_buf *buf)
+{
+ kunmap(buf->data_page);
+ __free_page(buf->data_page);
+}
+
+static inline u32 tpm_buf_length(struct tpm_buf *buf)
+{
+ struct tpm_header *head = (struct tpm_header *)buf->data;
+
+ return be32_to_cpu(head->length);
+}
+
+static inline u16 tpm_buf_tag(struct tpm_buf *buf)
+{
+ struct tpm_header *head = (struct tpm_header *)buf->data;
+
+ return be16_to_cpu(head->tag);
+}
+
+static inline void tpm_buf_append(struct tpm_buf *buf,
+ const unsigned char *new_data,
+ unsigned int new_len)
+{
+ struct tpm_header *head = (struct tpm_header *)buf->data;
+ u32 len = tpm_buf_length(buf);
+
+ /* Return silently if overflow has already happened. */
+ if (buf->flags & TPM_BUF_OVERFLOW)
+ return;
+
+ if ((len + new_len) > PAGE_SIZE) {
+ WARN(1, "tpm_buf: overflow\n");
+ buf->flags |= TPM_BUF_OVERFLOW;
+ return;
+ }
+
+ memcpy(&buf->data[len], new_data, new_len);
+ head->length = cpu_to_be32(len + new_len);
+}
+
+static inline void tpm_buf_append_u8(struct tpm_buf *buf, const u8 value)
+{
+ tpm_buf_append(buf, &value, 1);
+}
+
+static inline void tpm_buf_append_u16(struct tpm_buf *buf, const u16 value)
+{
+ __be16 value2 = cpu_to_be16(value);
+
+ tpm_buf_append(buf, (u8 *) &value2, 2);
+}
+
+static inline void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value)
+{
+ __be32 value2 = cpu_to_be32(value);
+
+ tpm_buf_append(buf, (u8 *) &value2, 4);
+}
+
#if defined(CONFIG_TCG_TPM) || defined(CONFIG_TCG_TPM_MODULE)
extern int tpm_is_tpm2(struct tpm_chip *chip);
diff --git a/security/keys/trusted.c b/security/keys/trusted.c
index 1fbd778..4cfae208 100644
--- a/security/keys/trusted.c
+++ b/security/keys/trusted.c
@@ -395,7 +395,7 @@ static int pcrlock(const int pcrnum)
/*
* Create an object specific authorisation protocol (OSAP) session
*/
-static int osap(struct tpm_buf *tb, struct osapsess *s,
+static int osap(struct tpm1_buf *tb, struct osapsess *s,
const unsigned char *key, uint16_t type, uint32_t handle)
{
unsigned char enonce[TPM_NONCE_SIZE];
@@ -430,7 +430,7 @@ static int osap(struct tpm_buf *tb, struct osapsess *s,
/*
* Create an object independent authorisation protocol (oiap) session
*/
-int oiap(struct tpm_buf *tb, uint32_t *handle, unsigned char *nonce)
+int oiap(struct tpm1_buf *tb, uint32_t *handle, unsigned char *nonce)
{
int ret;
@@ -464,7 +464,7 @@ struct tpm_digests {
* Have the TPM seal(encrypt) the trusted key, possibly based on
* Platform Configuration Registers (PCRs). AUTH1 for sealing key.
*/
-static int tpm_seal(struct tpm_buf *tb, uint16_t keytype,
+static int tpm_seal(struct tpm1_buf *tb, uint16_t keytype,
uint32_t keyhandle, const unsigned char *keyauth,
const unsigned char *data, uint32_t datalen,
unsigned char *blob, uint32_t *bloblen,
@@ -579,7 +579,7 @@ static int tpm_seal(struct tpm_buf *tb, uint16_t keytype,
/*
* use the AUTH2_COMMAND form of unseal, to authorize both key and blob
*/
-static int tpm_unseal(struct tpm_buf *tb,
+static int tpm_unseal(struct tpm1_buf *tb,
uint32_t keyhandle, const unsigned char *keyauth,
const unsigned char *blob, int bloblen,
const unsigned char *blobauth,
@@ -670,7 +670,7 @@ static int tpm_unseal(struct tpm_buf *tb,
static int key_seal(struct trusted_key_payload *p,
struct trusted_key_options *o)
{
- struct tpm_buf *tb;
+ struct tpm1_buf *tb;
int ret;
tb = kzalloc(sizeof *tb, GFP_KERNEL);
@@ -696,7 +696,7 @@ static int key_seal(struct trusted_key_payload *p,
static int key_unseal(struct trusted_key_payload *p,
struct trusted_key_options *o)
{
- struct tpm_buf *tb;
+ struct tpm1_buf *tb;
int ret;
tb = kzalloc(sizeof *tb, GFP_KERNEL);
--
2.7.4
^ permalink raw reply related
* [Patch v6 0/4] Create and consolidate trusted keys subsystem
From: Sumit Garg @ 2019-09-16 10:34 UTC (permalink / raw)
To: jarkko.sakkinen, dhowells, peterhuewe
Cc: keyrings, linux-integrity, linux-crypto, linux-security-module,
herbert, davem, jgg, arnd, gregkh, jejb, zohar, jmorris, serge,
jsnitsel, linux-kernel, daniel.thompson, Sumit Garg
This patch-set does restructuring of trusted keys code to create and
consolidate trusted keys subsystem.
Also, patch #2 replaces tpm1_buf code used in security/keys/trusted.c and
crypto/asymmertic_keys/asym_tpm.c files to use the common tpm_buf code.
Changes in v6:
1. Switch TPM asymmetric code also to use common tpm_buf code. These
changes required patches #1 and #2 update, so I have dropped review
tags from those patches.
2. Incorporated miscellaneous comments from Jarkko.
Changes in v5:
1. Drop 5/5 patch as its more relavant along with TEE patch-set.
2. Add Reviewed-by tag for patch #2.
3. Fix build failure when "CONFIG_HEADER_TEST" and
"CONFIG_KERNEL_HEADER_TEST" config options are enabled.
4. Misc changes to rename files.
Changes in v4:
1. Separate patch for export of tpm_buf code to include/linux/tpm.h
2. Change TPM1.x trusted keys code to use common tpm_buf
3. Keep module name as trusted.ko only
Changes in v3:
Move TPM2 trusted keys code to trusted keys subsystem.
Changes in v2:
Split trusted keys abstraction patch for ease of review.
Sumit Garg (4):
tpm: Move tpm_buf code to include/linux/
KEYS: Use common tpm_buf for trusted and asymmetric keys
KEYS: trusted: Create trusted keys subsystem
KEYS: trusted: Move TPM2 trusted keys code
crypto/asymmetric_keys/asym_tpm.c | 101 +++---
drivers/char/tpm/tpm-chip.c | 1 +
drivers/char/tpm/tpm-interface.c | 56 ----
drivers/char/tpm/tpm.h | 230 -------------
drivers/char/tpm/tpm2-cmd.c | 308 +----------------
include/Kbuild | 1 -
include/keys/{trusted.h => trusted_tpm.h} | 49 +--
include/linux/tpm.h | 270 ++++++++++++++-
security/keys/Makefile | 2 +-
security/keys/trusted-keys/Makefile | 8 +
.../{trusted.c => trusted-keys/trusted_tpm1.c} | 92 +++---
security/keys/trusted-keys/trusted_tpm2.c | 368 +++++++++++++++++++++
12 files changed, 728 insertions(+), 758 deletions(-)
rename include/keys/{trusted.h => trusted_tpm.h} (77%)
create mode 100644 security/keys/trusted-keys/Makefile
rename security/keys/{trusted.c => trusted-keys/trusted_tpm1.c} (94%)
create mode 100644 security/keys/trusted-keys/trusted_tpm2.c
--
2.7.4
^ permalink raw reply
* Re: [PATCH 1/3] ima: keep the integrity state of open files up to date
From: Eric Biggers @ 2019-09-15 20:24 UTC (permalink / raw)
To: Janne Karhunen
Cc: linux-integrity, linux-security-module, Mimi Zohar, linux-mm,
viro, Konsta Karsisto
In-Reply-To: <CAE=NcraXOhGcPHh3cPxfaNjFXtPyDdSFa9hSrUSPfpFUmsxyMA@mail.gmail.com>
On Tue, Sep 10, 2019 at 10:04:53AM +0300, Janne Karhunen wrote:
> On Tue, Sep 10, 2019 at 12:39 AM Eric Biggers <ebiggers@kernel.org> wrote:
> > > Core file operations (open, close, sync, msync, truncate) are
> > > now allowed to update the measurement immediately. In order
> > > to maintain sufficient write performance for writes, add a
> > > latency tunable delayed work workqueue for computing the
> > > measurements.
> >
> > This still doesn't make it crash-safe. So why is it okay?
>
> If Android is the load, this makes it crash safe 99% of the time and
> that is considerably better than 0% of the time.
>
Who will use it if it isn't 100% safe?
- Eric
^ permalink raw reply
* Re: [RFC v1 14/14] krsi: Pin arg pages only when needed
From: Yonghong Song @ 2019-09-15 19:45 UTC (permalink / raw)
To: KP Singh
Cc: linux-kernel@vger.kernel.org, bpf@vger.kernel.org,
linux-security-module@vger.kernel.org, Alexei Starovoitov,
Daniel Borkmann, James Morris, Kees Cook, Thomas Garnier,
Michael Halcrow, Paul Turner, Brendan Gregg, Jann Horn,
Matthew Garrett, Christian Brauner, Mickaël Salaün,
Florent Revest, Martin Lau, Song Liu, Serge E. Hallyn,
Mauro Carvalho Chehab, David S. Miller, Greg Kroah-Hartman,
Nicolas Ferre, Stanislav Fomichev, Quentin Monnet, Andrey Ignatov,
Joe Stringer
In-Reply-To: <20190915014008.GA19558@chromium.org>
On 9/15/19 2:40 AM, KP Singh wrote:
> On 15-Sep 00:33, Yonghong Song wrote:
>>
>>
>> On 9/10/19 12:55 PM, KP Singh wrote:
>>> From: KP Singh <kpsingh@google.com>
>>>
>>> Adds a callback which is called when a new program is attached
>>> to a hook. The callback registered by the process_exection hook
>>> checks if a program that has calls to a helper that requires pages to
>>> be pinned (eg. krsi_get_env_var).
>>>
>>> Signed-off-by: KP Singh <kpsingh@google.com>
>>> ---
>>> include/linux/krsi.h | 1 +
>>> security/krsi/include/hooks.h | 5 ++-
>>> security/krsi/include/krsi_init.h | 7 ++++
>>> security/krsi/krsi.c | 62 ++++++++++++++++++++++++++++---
>>> security/krsi/ops.c | 10 ++++-
>>> 5 files changed, 77 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/include/linux/krsi.h b/include/linux/krsi.h
>>> index c7d1790d0c1f..e443d0309764 100644
>>> --- a/include/linux/krsi.h
>>> +++ b/include/linux/krsi.h
>>> @@ -7,6 +7,7 @@
>>>
>>> #ifdef CONFIG_SECURITY_KRSI
>>> int krsi_prog_attach(const union bpf_attr *attr, struct bpf_prog *prog);
>>> +extern const struct bpf_func_proto krsi_get_env_var_proto;
>>> #else
>>> static inline int krsi_prog_attach(const union bpf_attr *attr,
>>> struct bpf_prog *prog)
>>> diff --git a/security/krsi/include/hooks.h b/security/krsi/include/hooks.h
>>> index e070c452b5de..38293125ff99 100644
>>> --- a/security/krsi/include/hooks.h
>>> +++ b/security/krsi/include/hooks.h
>>> @@ -8,7 +8,7 @@
>>> *
>>> * Format:
>>> *
>>> - * KRSI_HOOK_INIT(TYPE, NAME, LSM_HOOK, KRSI_HOOK_FN)
>>> + * KRSI_HOOK_INIT(TYPE, NAME, LSM_HOOK, KRSI_HOOK_FN, CALLBACK)
>>> *
>>> * KRSI adds one layer of indirection between the name of the hook and the name
>>> * it exposes to the userspace in Security FS to prevent the userspace from
>>> @@ -18,4 +18,5 @@
>>> KRSI_HOOK_INIT(PROCESS_EXECUTION,
>>> process_execution,
>>> bprm_check_security,
>>> - krsi_process_execution)
>>> + krsi_process_execution,
>>> + krsi_process_execution_cb)
>>> diff --git a/security/krsi/include/krsi_init.h b/security/krsi/include/krsi_init.h
>>> index 6152847c3b08..99801d5b273a 100644
>>> --- a/security/krsi/include/krsi_init.h
>>> +++ b/security/krsi/include/krsi_init.h
>>> @@ -31,6 +31,8 @@ struct krsi_ctx {
>>> };
>>> };
>>>
>>> +typedef int (*krsi_prog_attach_t) (struct bpf_prog_array *);
>>> +
>>> /*
>>> * The LSM creates one file per hook.
>>> *
>>> @@ -61,6 +63,11 @@ struct krsi_hook {
>>> * The eBPF programs that are attached to this hook.
>>> */
>>> struct bpf_prog_array __rcu *progs;
>>> + /*
>>> + * The attach callback is called before a new program is attached
>>> + * to the hook and is passed the updated bpf_prog_array as an argument.
>>> + */
>>> + krsi_prog_attach_t attach_callback;
>>> };
>>>
>>> extern struct krsi_hook krsi_hooks_list[];
>>> diff --git a/security/krsi/krsi.c b/security/krsi/krsi.c
>>> index 00a7150c1b22..a4443d7aa150 100644
>>> --- a/security/krsi/krsi.c
>>> +++ b/security/krsi/krsi.c
>>> @@ -5,15 +5,65 @@
>>> #include <linux/bpf.h>
>>> #include <linux/binfmts.h>
>>> #include <linux/highmem.h>
>>> +#include <linux/krsi.h>
>>> #include <linux/mm.h>
>>>
>>> #include "krsi_init.h"
>>>
>>> +/*
>>> + * need_arg_pages is only updated in bprm_check_security_cb
>>> + * when a mutex on krsi_hook for bprm_check_security is already
>>> + * held. need_arg_pages avoids pinning pages when no program
>>> + * that needs them is attached to the hook.
>>> + */
>>> +static bool need_arg_pages;
>>> +
>>> +/*
>>> + * Checks if the instruction is a BPF_CALL to an eBPF helper located
>>> + * at the given address.
>>> + */
>>> +static inline bool bpf_is_call_to_func(struct bpf_insn *insn,
>>> + void *func_addr)
>>> +{
>>> + u8 opcode = BPF_OP(insn->code);
>>> +
>>> + if (opcode != BPF_CALL)
>>> + return false;
>>> +
>>> + if (insn->src_reg == BPF_PSEUDO_CALL)
>>> + return false;
>>> +
>>> + /*
>>> + * The BPF verifier updates the value of insn->imm from the
>>> + * enum bpf_func_id to the offset of the address of helper
>>> + * from the __bpf_call_base.
>>> + */
>>> + return __bpf_call_base + insn->imm == func_addr;
>>
>> In how many cases, krsi program does not have krsi_get_env_var() helper?
>
> It depends, if the user does not choose to use log environment
> variables or use the the value as a part of their MAC policy, the
> pinning of the pages is not needed.
Thanks. I just want to know whether we want to optimize such cases.
I am not a security expert, so I am okay with whatever decision you
made.
>
> Also, the pinning is needed since eBPF helpers cannot run a non-atomic
> context. It would not be needed if "sleepable eBPF" becomes a thing.
Indeed. 'sleepable BPF' might be also a good thing for realtime linux.
Some works need to address bpf area spin lock, per cpu variables, etc.
before that can happen.
>
> - KP
>
>>
>>> +}
>>> +
>>> +static int krsi_process_execution_cb(struct bpf_prog_array *array)
>>> +{
>>> + struct bpf_prog_array_item *item = array->items;
>>> + struct bpf_prog *p;
>>> + const struct bpf_func_proto *proto = &krsi_get_env_var_proto;
>>> + int i;
>>> +
>>> + while ((p = READ_ONCE(item->prog))) {
>>> + for (i = 0; i < p->len; i++) {
>>> + if (bpf_is_call_to_func(&p->insnsi[i], proto->func))
>>> + need_arg_pages = true;
>>> + }
>>> + item++;
>>> + }
>>> + return 0;
>>> +}
>>> +
>>> struct krsi_hook krsi_hooks_list[] = {
>>> - #define KRSI_HOOK_INIT(TYPE, NAME, H, I) \
>>> + #define KRSI_HOOK_INIT(TYPE, NAME, H, I, CB) \
>>> [TYPE] = { \
>>> .h_type = TYPE, \
>>> .name = #NAME, \
>>> + .attach_callback = CB, \
>>> },
>>> #include "hooks.h"
>>> #undef KRSI_HOOK_INIT
>>> @@ -75,9 +125,11 @@ static int krsi_process_execution(struct linux_binprm *bprm)
>>> .bprm = bprm,
>>> };
>>>
>>> - ret = pin_arg_pages(&ctx.bprm_ctx);
>>> - if (ret < 0)
>>> - goto out_arg_pages;
>>> + if (READ_ONCE(need_arg_pages)) {
>>> + ret = pin_arg_pages(&ctx.bprm_ctx);
>>> + if (ret < 0)
>>> + goto out_arg_pages;
>>> + }
>>>
>>> ret = krsi_run_progs(PROCESS_EXECUTION, &ctx);
>>> kfree(ctx.bprm_ctx.arg_pages);
>>> @@ -87,7 +139,7 @@ static int krsi_process_execution(struct linux_binprm *bprm)
>>> }
>>>
>>> static struct security_hook_list krsi_hooks[] __lsm_ro_after_init = {
>>> - #define KRSI_HOOK_INIT(T, N, HOOK, IMPL) LSM_HOOK_INIT(HOOK, IMPL),
>>> + #define KRSI_HOOK_INIT(T, N, HOOK, IMPL, CB) LSM_HOOK_INIT(HOOK, IMPL),
>>> #include "hooks.h"
>>> #undef KRSI_HOOK_INIT
>>> };
>>> diff --git a/security/krsi/ops.c b/security/krsi/ops.c
>>> index 1db94dfaac15..2de682371eff 100644
>>> --- a/security/krsi/ops.c
>>> +++ b/security/krsi/ops.c
>>> @@ -139,6 +139,14 @@ int krsi_prog_attach(const union bpf_attr *attr, struct bpf_prog *prog)
>>> goto unlock;
>>> }
>>>
>>> + if (h->attach_callback) {
>>> + ret = h->attach_callback(new_array);
>>> + if (ret < 0) {
>>> + bpf_prog_array_free(new_array);
>>> + goto unlock;
>>> + }
>>> + }
>>> +
>>> rcu_assign_pointer(h->progs, new_array);
>>> bpf_prog_array_free(old_array);
>>>
>>> @@ -278,7 +286,7 @@ BPF_CALL_5(krsi_get_env_var, struct krsi_ctx *, ctx, char *, name, u32, n_size,
>>> return get_env_var(ctx, name, dest, n_size, size);
>>> }
>>>
>>> -static const struct bpf_func_proto krsi_get_env_var_proto = {
>>> +const struct bpf_func_proto krsi_get_env_var_proto = {
>>> .func = krsi_get_env_var,
>>> .gpl_only = true,
>>> .ret_type = RET_INTEGER,
>>>
^ 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