* Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver
From: Jarkko Sakkinen @ 2020-06-26 13:40 UTC (permalink / raw)
To: Borislav Petkov
Cc: x86, linux-sgx, linux-kernel, linux-security-module,
Jethro Beekman, Haitao Huang, Chunyang Hui, Jordan Hand,
Nathaniel McCallum, Seth Moore, Sean Christopherson,
Suresh Siddha, akpm, andriy.shevchenko, asapek, cedric.xing,
chenalexchen, conradparker, cyhanish, dave.hansen, haitao.huang,
josh, kai.huang, kai.svahn, kmoy, ludloff, luto, nhorman,
puiterwijk, rientjes, tglx, yaozhangx
In-Reply-To: <20200625202520.GQ20319@zn.tnic>
On Thu, Jun 25, 2020 at 10:25:39PM +0200, Borislav Petkov wrote:
> On Thu, Jun 25, 2020 at 11:21:48PM +0300, Jarkko Sakkinen wrote:
> > Would be probably easier to review also this way because the commit kind
> > of rationalizes why things exist.
> >
> > What do you think?
>
> Sounds like a plan but you can do this for the next version - no need to
> do it now. I'll try to review this way, per ioctl as I said in my mail
> to Sean.
OK, sure I won't rush with a new version :-)
The code has been reworked so many times that it is somewhat easy to
make such split and I think it is one measure that an implementation is
somewhat sound when parts of functionality build up cleanly on top of
each other.
/Jarkko
^ permalink raw reply
* Re: [PATCH 00/14] Make the user mode driver code a better citizen
From: Eric W. Biederman @ 2020-06-26 13:48 UTC (permalink / raw)
To: Linus Torvalds
Cc: David Miller, Greg Kroah-Hartman, Tetsuo Handa,
Alexei Starovoitov, Kees Cook, Andrew Morton, Alexei Starovoitov,
Al Viro, bpf, linux-fsdevel, Daniel Borkmann, Jakub Kicinski,
Masahiro Yamada, Gary Lin, Bruno Meneguele, LSM List,
Casey Schaufler, Luis Chamberlain
In-Reply-To: <87pn9mgfc2.fsf_-_@x220.int.ebiederm.org>
Adding Luis Chamberlain as he maintains the user mode helper code.
Just so everyone who is relevant is at least aware of what is going on.
ebiederm@xmission.com (Eric W. Biederman) writes:
> Asking for people to fix their bugs in this user mode driver code has
> been remarkably unproductive. So here are my bug fixes.
>
> I have tested them by booting with the code compiled in and
> by killing "bpfilter_umh" and running iptables -vnL to restart
> the userspace driver.
>
> I have split the changes into small enough pieces so they should be
> easily readable and testable.
>
> The changes lean into the preexisting interfaces in the kernel and
> remove special cases for user mode driver code in favor of solutions
> that don't need special cases. This results in smaller code with
> fewer bugs.
>
> At a practical level this removes the maintenance burden of the
> user mode drivers from the user mode helper code and from exec as
> the special cases are removed.
>
> Similarly the LSM interaction bugs are fixed by not having unnecessary
> special cases for user mode drivers.
>
> Please let me know if you see any bugs. Once the code review is
> finished I plan to take this through my tree.
>
> Eric W. Biederman (14):
> umh: Capture the pid in umh_pipe_setup
> umh: Move setting PF_UMH into umh_pipe_setup
> umh: Rename the user mode driver helpers for clarity
> umh: Remove call_usermodehelper_setup_file.
> umh: Separate the user mode driver and the user mode helper support
> umd: For clarity rename umh_info umd_info
> umd: Rename umd_info.cmdline umd_info.driver_name
> umd: Transform fork_usermode_blob into fork_usermode_driver
> umh: Stop calling do_execve_file
> exec: Remove do_execve_file
> bpfilter: Move bpfilter_umh back into init data
> umd: Track user space drivers with struct pid
> bpfilter: Take advantage of the facilities of struct pid
> umd: Remove exit_umh
>
> fs/exec.c | 38 ++------
> include/linux/binfmts.h | 1 -
> include/linux/bpfilter.h | 7 +-
> include/linux/sched.h | 9 --
> include/linux/umd.h | 18 ++++
> include/linux/umh.h | 15 ----
> kernel/Makefile | 1 +
> kernel/exit.c | 1 -
> kernel/umd.c | 183 +++++++++++++++++++++++++++++++++++++++
> kernel/umh.c | 171 +-----------------------------------
> net/bpfilter/bpfilter_kern.c | 38 ++++----
> net/bpfilter/bpfilter_umh_blob.S | 2 +-
> net/ipv4/bpfilter/sockopt.c | 20 +++--
> 13 files changed, 249 insertions(+), 255 deletions(-)
>
> Eric
^ permalink raw reply
* Re: [PATCH 00/14] Make the user mode driver code a better citizen
From: Greg Kroah-Hartman @ 2020-06-26 14:10 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Linus Torvalds, David Miller, Tetsuo Handa, Alexei Starovoitov,
Kees Cook, Andrew Morton, Alexei Starovoitov, Al Viro, bpf,
linux-fsdevel, Daniel Borkmann, Jakub Kicinski, Masahiro Yamada,
Gary Lin, Bruno Meneguele, LSM List, Casey Schaufler
In-Reply-To: <87pn9mgfc2.fsf_-_@x220.int.ebiederm.org>
On Fri, Jun 26, 2020 at 07:51:41AM -0500, Eric W. Biederman wrote:
>
> Asking for people to fix their bugs in this user mode driver code has
> been remarkably unproductive. So here are my bug fixes.
>
> I have tested them by booting with the code compiled in and
> by killing "bpfilter_umh" and running iptables -vnL to restart
> the userspace driver.
>
> I have split the changes into small enough pieces so they should be
> easily readable and testable.
>
> The changes lean into the preexisting interfaces in the kernel and
> remove special cases for user mode driver code in favor of solutions
> that don't need special cases. This results in smaller code with
> fewer bugs.
>
> At a practical level this removes the maintenance burden of the
> user mode drivers from the user mode helper code and from exec as
> the special cases are removed.
>
> Similarly the LSM interaction bugs are fixed by not having unnecessary
> special cases for user mode drivers.
>
> Please let me know if you see any bugs. Once the code review is
> finished I plan to take this through my tree.
>
> Eric W. Biederman (14):
> umh: Capture the pid in umh_pipe_setup
> umh: Move setting PF_UMH into umh_pipe_setup
> umh: Rename the user mode driver helpers for clarity
> umh: Remove call_usermodehelper_setup_file.
> umh: Separate the user mode driver and the user mode helper support
> umd: For clarity rename umh_info umd_info
> umd: Rename umd_info.cmdline umd_info.driver_name
> umd: Transform fork_usermode_blob into fork_usermode_driver
> umh: Stop calling do_execve_file
> exec: Remove do_execve_file
> bpfilter: Move bpfilter_umh back into init data
> umd: Track user space drivers with struct pid
> bpfilter: Take advantage of the facilities of struct pid
> umd: Remove exit_umh
After a quick read, all looks sane to me, nice cleanups!
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
^ permalink raw reply
* Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver
From: Sean Christopherson @ 2020-06-26 14:16 UTC (permalink / raw)
To: Borislav Petkov
Cc: Jarkko Sakkinen, x86, linux-sgx, linux-kernel,
linux-security-module, Jethro Beekman, Haitao Huang, Chunyang Hui,
Jordan Hand, Nathaniel McCallum, Seth Moore, Suresh Siddha, akpm,
andriy.shevchenko, asapek, cedric.xing, chenalexchen,
conradparker, cyhanish, dave.hansen, haitao.huang, josh,
kai.huang, kai.svahn, kmoy, ludloff, luto, nhorman, puiterwijk,
rientjes, tglx, yaozhangx
In-Reply-To: <20200626091419.GB27151@zn.tnic>
On Fri, Jun 26, 2020 at 11:14:19AM +0200, Borislav Petkov wrote:
> On Thu, Jun 18, 2020 at 01:08:33AM +0300, Jarkko Sakkinen wrote:
> > +static int sgx_encl_create(struct sgx_encl *encl, struct sgx_secs *secs)
> > +{
> > + unsigned long encl_size = secs->size + PAGE_SIZE;
>
> Wait, you just copied @secs from user memory in sgx_ioc_enclave_create()
> and now use ->size unverified? You're kidding, right?
The size of the enclave is checked in sgx_validate_secs() before it is used
to configure the shmem backing.
> > + struct sgx_epc_page *secs_epc;
> > + unsigned long ssaframesize;
> > + struct sgx_pageinfo pginfo;
> > + struct sgx_secinfo secinfo;
> > + struct file *backing;
> > + long ret;
> > +
> > + if (atomic_read(&encl->flags) & SGX_ENCL_CREATED)
> > + return -EINVAL;
> > +
> > + ssaframesize = sgx_calc_ssaframesize(secs->miscselect, secs->xfrm);
>
> So this is using more un-validated user input to do further calculations.
> What can possibly go wrong?
ssaframesize is also validated below, and the computations on miscselect and
xfm in sgx_calc_ssaframesize() are bounded such that bad input won't send
the kernel into the weeds.
That being said, I agree that it would be safer to move sgx_calc_ssaframesize()
inside sgx_validate_secs() and only compute encl_size after the secs is
validated.
> I sure hope *I* am wrong and am missing something here.
>
> If not, please, for the next version, audit all your user input and
> validate it before using it. Srsly.
>
> > + if (sgx_validate_secs(secs, ssaframesize)) {
> > + pr_debug("invalid SECS\n");
> > + return -EINVAL;
> > + }
> > +
> > + backing = shmem_file_setup("SGX backing", encl_size + (encl_size >> 5),
> > + VM_NORESERVE);
> > + if (IS_ERR(backing))
> > + return PTR_ERR(backing);
> > +
> > + encl->backing = backing;
> > +
> > + secs_epc = __sgx_alloc_epc_page();
> > + if (IS_ERR(secs_epc)) {
> > + ret = PTR_ERR(secs_epc);
> > + goto err_out_backing;
> > + }
> > +
> > + encl->secs.epc_page = secs_epc;
> > +
> > + pginfo.addr = 0;
> > + pginfo.contents = (unsigned long)secs;
> > + pginfo.metadata = (unsigned long)&secinfo;
> > + pginfo.secs = 0;
> > + memset(&secinfo, 0, sizeof(secinfo));
> > +
> > + ret = __ecreate((void *)&pginfo, sgx_get_epc_addr(secs_epc));
> > + if (ret) {
> > + pr_debug("ECREATE returned %ld\n", ret);
> > + goto err_out;
> > + }
> > +
> > + if (secs->attributes & SGX_ATTR_DEBUG)
> > + atomic_or(SGX_ENCL_DEBUG, &encl->flags);
> > +
> > + encl->secs.encl = encl;
> > + encl->secs_attributes = secs->attributes;
> > + encl->allowed_attributes |= SGX_ATTR_ALLOWED_MASK;
> > + encl->base = secs->base;
> > + encl->size = secs->size;
> > + encl->ssaframesize = secs->ssa_frame_size;
> > +
> > + /*
> > + * Set SGX_ENCL_CREATED only after the enclave is fully prepped. This
> > + * allows setting and checking enclave creation without having to take
> > + * encl->lock.
> > + */
> > + atomic_or(SGX_ENCL_CREATED, &encl->flags);
> > +
> > + return 0;
> > +
> > +err_out:
> > + sgx_free_epc_page(encl->secs.epc_page);
> > + encl->secs.epc_page = NULL;
> > +
> > +err_out_backing:
> > + fput(encl->backing);
> > + encl->backing = NULL;
> > +
> > + return ret;
> > +}
> > +
> > +/**
> > + * sgx_ioc_enclave_create - handler for %SGX_IOC_ENCLAVE_CREATE
> > + * @filep: open file to /dev/sgx
>
> That's
>
> @encl: enclave pointer
>
> or so.
>
> > + * @arg: userspace pointer to a struct sgx_enclave_create instance
> > + *
> > + * Allocate kernel data structures for a new enclave and execute ECREATE after
> > + * verifying the correctness of the provided SECS.
> > + *
> > + * Note, enforcement of restricted and disallowed attributes is deferred until
> > + * sgx_ioc_enclave_init(), only the architectural correctness of the SECS is
> > + * checked by sgx_ioc_enclave_create().
>
> Well, I don't see that checking. Where is it?
>
> > + *
> > + * Return:
> > + * 0 on success,
> > + * -errno otherwise
> > + */
> > +static long sgx_ioc_enclave_create(struct sgx_encl *encl, void __user *arg)
> > +{
> > + struct sgx_enclave_create ecreate;
> > + struct page *secs_page;
> > + struct sgx_secs *secs;
> > + int ret;
> > +
> > + if (copy_from_user(&ecreate, arg, sizeof(ecreate)))
> > + return -EFAULT;
> > +
> > + secs_page = alloc_page(GFP_KERNEL);
> > + if (!secs_page)
> > + return -ENOMEM;
> > +
> > + secs = kmap(secs_page);
> > + if (copy_from_user(secs, (void __user *)ecreate.src, sizeof(*secs))) {
> > + ret = -EFAULT;
> > + goto out;
> > + }
> > +
> > + ret = sgx_encl_create(encl, secs);
> > +
> > +out:
> > + kunmap(secs_page);
> > + __free_page(secs_page);
> > + return ret;
> > +}
>
> --
> Regards/Gruss,
> Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply
* Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver
From: Jarkko Sakkinen @ 2020-06-26 14:17 UTC (permalink / raw)
To: Borislav Petkov
Cc: x86, linux-sgx, linux-kernel, linux-security-module,
Jethro Beekman, Haitao Huang, Chunyang Hui, Jordan Hand,
Nathaniel McCallum, Seth Moore, Sean Christopherson,
Suresh Siddha, akpm, andriy.shevchenko, asapek, cedric.xing,
chenalexchen, conradparker, cyhanish, dave.hansen, haitao.huang,
josh, kai.huang, kai.svahn, kmoy, ludloff, luto, nhorman,
puiterwijk, rientjes, tglx, yaozhangx
In-Reply-To: <20200625185334.GN20319@zn.tnic>
On Thu, Jun 25, 2020 at 08:53:34PM +0200, Borislav Petkov wrote:
> On Thu, Jun 18, 2020 at 01:08:33AM +0300, Jarkko Sakkinen wrote:
>
> > Subject: Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver
> ^
> Add
I'll change it to "Add SGX enclave driver".
>
> > Intel Software Guard eXtensions (SGX) is a set of CPU instructions that
> > can be used by applications to set aside private regions of code and
> > data. The code outside the SGX hosted software entity is disallowed to
> > access the memory inside the enclave enforced by the CPU. We call these
> > entities as enclaves.
>
> s/as //
>
> > This commit implements a driver that provides an ioctl API to construct
>
> s/This commit implements/Implement/
>
> > and run enclaves. Enclaves are constructed from pages residing in
> > reserved physical memory areas. The contents of these pages can only be
> > accessed when they are mapped as part of an enclave, by a hardware
> > thread running inside the enclave.
> >
> > The starting state of an enclave consists of a fixed measured set of
> > pages that are copied to the EPC during the construction process by
> > using ENCLS leaf functions and Software Enclave Control Structure (SECS)
> > that defines the enclave properties.
> >
> > Enclave are constructed by using ENCLS leaf functions ECREATE, EADD and
>
> Enclaves
>
> > EINIT. ECREATE initializes SECS, EADD copies pages from system memory to
> > the EPC and EINIT check a given signed measurement and moves the enclave
>
> checks
>
> > into a state ready for execution.
> >
> > An initialized enclave can only be accessed through special Thread Control
> > Structure (TCS) pages by using ENCLU (ring-3 only) leaf EENTER. This leaf
> > function converts a thread into enclave mode and continues the execution in
> > the offset defined by the TCS provided to EENTER. An enclave is exited
> > through syscall, exception, interrupts or by explicitly calling another
> > ENCLU leaf EEXIT.
> >
> > The permissions, which enclave page is added will set the limit for maximum
> > permissions that can be set for mmap() and mprotect().
>
> I can't parse that sentence.
Neither can I.
> > This will
> > effectively allow to build different security schemes between producers and
> > consumers of enclaves. Later on we can increase granularity with LSM hooks
> > for page addition (i.e. for producers) and mapping of the enclave (i.e. for
> > consumers)
I rephrased the whole paragraph:
"
The mmap() permissions are capped by the contained enclave page
permissions. The mapped areas must also be opaque, i.e. each page address
must contain a page. This logic is implemented in sgx_encl_may_map().
"
> Other than that, nice explanation. I like that in a commit message.
>
> Thx.
Thank you.
>
> --
> Regards/Gruss,
> Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette
/Jarkko
^ permalink raw reply
* Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver
From: Jarkko Sakkinen @ 2020-06-26 14:19 UTC (permalink / raw)
To: Sean Christopherson
Cc: Borislav Petkov, x86, linux-sgx, linux-kernel,
linux-security-module, Jethro Beekman, Haitao Huang, Chunyang Hui,
Jordan Hand, Nathaniel McCallum, Seth Moore, Suresh Siddha, akpm,
andriy.shevchenko, asapek, cedric.xing, chenalexchen,
conradparker, cyhanish, dave.hansen, haitao.huang, josh,
kai.huang, kai.svahn, kmoy, ludloff, luto, nhorman, puiterwijk,
rientjes, tglx, yaozhangx
In-Reply-To: <20200625183448.GG3437@linux.intel.com>
On Thu, Jun 25, 2020 at 11:34:48AM -0700, Sean Christopherson wrote:
> On Thu, Jun 25, 2020 at 07:23:19PM +0200, Borislav Petkov wrote:
> > Also, you had all patches until now split nice and logically doing one
> > thing only.
> >
> > But this one is huge. Why?
> >
> > Why can't you split out the facilities which the driver uses: encl.[ch]
> > into a patch, then ioctl.c into a separate one and then the driver into
> > a third one? Or do they all belong together inseparably?
> >
> > I guess I'll find out eventually but it would've been nice if they were
> > split out...
>
> Hmm, I think the most reasonable way to break up this beast would be to
> incrementally introduce functionality. E.g. four or so patches, one for
> each ioctl() of ENCLAVE_CREATE, ENCLAVE_ADD_PAGES, ENCLAVE_INIT and
> ENCLAVE_SET_ATTRIBUTE, in that order.
>
> Splitting up by file probably wouldn't work very well. The split is
> pretty arbitrary, e.g. encl.[ch] isn't simply a pure representation of an
> enclave, there is a lot of the driver details/dependencies in there, i.e.
> the functionality between encl/ioctl/driver is all pretty intertwined.
>
> But I think serially introducing each ioctl() would be fairly clean, and
> would help readers/reviewers better understand SGX as the patches would
> naturally document the process of building an enclave, e.g. CREATE the
> enclave, then ADD_PAGES, then INIT the enclave. SET_ATTRIBUTE is a bit
> of an outlier in that it would be chronologically out of order with
> respect to building the enclave, but I think that's ok.
>
> Jarkko, thoughts?
I proposed the same before I go this email so I guess we have a
consensus here.
/Jarkko
^ permalink raw reply
* Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver
From: Borislav Petkov @ 2020-06-26 14:20 UTC (permalink / raw)
To: Sean Christopherson
Cc: Jarkko Sakkinen, x86, linux-sgx, linux-kernel,
linux-security-module, Jethro Beekman, Haitao Huang, Chunyang Hui,
Jordan Hand, Nathaniel McCallum, Seth Moore, Suresh Siddha, akpm,
andriy.shevchenko, asapek, cedric.xing, chenalexchen,
conradparker, cyhanish, dave.hansen, haitao.huang, josh,
kai.huang, kai.svahn, kmoy, ludloff, luto, nhorman, puiterwijk,
rientjes, tglx, yaozhangx
In-Reply-To: <20200626141627.GA6583@linux.intel.com>
On Fri, Jun 26, 2020 at 07:16:27AM -0700, Sean Christopherson wrote:
> That being said, I agree that it would be safer to move sgx_calc_ssaframesize()
> inside sgx_validate_secs() and only compute encl_size after the secs is
> validated.
Yap, that would be the right thing to do.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply
* Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver
From: Borislav Petkov @ 2020-06-26 15:34 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: x86, linux-sgx, linux-kernel, linux-security-module,
Jethro Beekman, Haitao Huang, Chunyang Hui, Jordan Hand,
Nathaniel McCallum, Seth Moore, Sean Christopherson,
Suresh Siddha, akpm, andriy.shevchenko, asapek, cedric.xing,
chenalexchen, conradparker, cyhanish, dave.hansen, haitao.huang,
josh, kai.huang, kai.svahn, kmoy, ludloff, luto, nhorman,
puiterwijk, rientjes, tglx, yaozhangx
In-Reply-To: <20200617220844.57423-12-jarkko.sakkinen@linux.intel.com>
On Thu, Jun 18, 2020 at 01:08:33AM +0300, Jarkko Sakkinen wrote:
...
This could use some commenting along the lines of:
"— If the enclave developer requires measurement of the page as a
proof for the content, use EEXTEND to add a measurement for 256 bytes of
the page. Repeat this operation until the entire page is measured."
At least this text from the SDM maps to the 256 bytes below. Otherwise
it is magic.
> +static int __sgx_encl_extend(struct sgx_encl *encl,
> + struct sgx_epc_page *epc_page)
> +{
> + int ret;
> + int i;
> +
> + for (i = 0; i < 16; i++) {
> + ret = __eextend(sgx_get_epc_addr(encl->secs.epc_page),
> + sgx_get_epc_addr(epc_page) + (i * 0x100));
> + if (ret) {
> + if (encls_failed(ret))
> + ENCLS_WARN(ret, "EEXTEND");
> + return -EIO;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int sgx_encl_add_page(struct sgx_encl *encl, unsigned long src,
> + unsigned long offset, unsigned long length,
> + struct sgx_secinfo *secinfo, unsigned long flags)
> +{
> + struct sgx_encl_page *encl_page;
> + struct sgx_epc_page *epc_page;
> + int ret;
> +
> + encl_page = sgx_encl_page_alloc(encl, offset, secinfo->flags);
> + if (IS_ERR(encl_page))
> + return PTR_ERR(encl_page);
> +
> + epc_page = __sgx_alloc_epc_page();
> + if (IS_ERR(epc_page)) {
> + kfree(encl_page);
> + return PTR_ERR(epc_page);
> + }
> +
> + if (atomic_read(&encl->flags) &
> + (SGX_ENCL_INITIALIZED | SGX_ENCL_DEAD)) {
> + ret = -EFAULT;
> + goto err_out_free;
> + }
You can do this first thing when you enter the function so that
you don't have to allocate needlessly in the error case, when
SGX_ENCL_INITIALIZED | SGX_ENCL_DEAD is set.
> +
> + mmap_read_lock(current->mm);
> + mutex_lock(&encl->lock);
> +
> + /*
> + * Insert prior to EADD in case of OOM. EADD modifies MRENCLAVE, i.e.
> + * can't be gracefully unwound, while failure on EADD/EXTEND is limited
> + * to userspace errors (or kernel/hardware bugs).
> + */
> + ret = radix_tree_insert(&encl->page_tree, PFN_DOWN(encl_page->desc),
> + encl_page);
> + if (ret)
> + goto err_out_unlock;
> +
> + ret = __sgx_encl_add_page(encl, encl_page, epc_page, secinfo,
> + src);
> + if (ret)
> + goto err_out;
> +
> + /*
> + * Complete the "add" before doing the "extend" so that the "add"
> + * isn't in a half-baked state in the extremely unlikely scenario the
> + * the enclave will be destroyed in response to EEXTEND failure.
> + */
> + encl_page->encl = encl;
> + encl_page->epc_page = epc_page;
> + encl->secs_child_cnt++;
> +
> + if (flags & SGX_PAGE_MEASURE) {
> + ret = __sgx_encl_extend(encl, epc_page);
> + if (ret)
> + goto err_out;
> + }
> +
> + mutex_unlock(&encl->lock);
> + mmap_read_unlock(current->mm);
> + return ret;
> +
> +err_out:
> + radix_tree_delete(&encl_page->encl->page_tree,
> + PFN_DOWN(encl_page->desc));
> +
> +err_out_unlock:
> + mutex_unlock(&encl->lock);
> + mmap_read_unlock(current->mm);
> +
> +err_out_free:
> + sgx_free_epc_page(epc_page);
> + kfree(encl_page);
> +
> + /*
> + * Destroy enclave on ENCLS failure as this means that EPC has been
> + * invalidated.
> + */
> + if (ret == -EIO) {
> + mutex_lock(&encl->lock);
> + sgx_encl_destroy(encl);
> + mutex_unlock(&encl->lock);
> + }
> +
> + return ret;
> +}
> +
> +/**
> + * sgx_ioc_enclave_add_pages() - The handler for %SGX_IOC_ENCLAVE_ADD_PAGES
> + * @encl: pointer to an enclave instance (via ioctl() file pointer)
> + * @arg: a user pointer to a struct sgx_enclave_add_pages instance
> + *
> + * Add one or more pages to an uninitialized enclave, and optionally extend the
"uninitialized"?
Where is the test for SGX_ENCL_INITIALIZED and erroring out otherwise?
I.e., what happens if you add pages to an initialized enclave?
> + * measurement with the contents of the page. The address range of pages must
> + * be contiguous.
Must? Who is enforcing this? I'm trying to find where...
> The SECINFO and measurement mask are applied to all pages.
> + *
> + * A SECINFO for a TCS is required to always contain zero permissions because
> + * CPU silently zeros them. Allowing anything else would cause a mismatch in
> + * the measurement.
> + *
> + * mmap()'s protection bits are capped by the page permissions. For each page
> + * address, the maximum protection bits are computed with the following
> + * heuristics:
> + *
> + * 1. A regular page: PROT_R, PROT_W and PROT_X match the SECINFO permissions.
> + * 2. A TCS page: PROT_R | PROT_W.
> + *
> + * mmap() is not allowed to surpass the minimum of the maximum protection bits
> + * within the given address range.
> + *
> + * If ENCLS opcode fails, that effectively means that EPC has been invalidated.
> + * When this happens the enclave is destroyed and -EIO is returned to the
> + * caller.
> + *
> + * Return:
> + * 0 on success,
> + * -EACCES if an executable source page is located in a noexec partition,
> + * -EIO if either ENCLS[EADD] or ENCLS[EEXTEND] fails
> + * -errno otherwise
> + */
> +static long sgx_ioc_enclave_add_pages(struct sgx_encl *encl, void __user *arg)
> +{
> + struct sgx_enclave_add_pages addp;
> + struct sgx_secinfo secinfo;
> + unsigned long c;
> + int ret;
> +
> + if (!(atomic_read(&encl->flags) & SGX_ENCL_CREATED))
> + return -EINVAL;
> +
> + if (copy_from_user(&addp, arg, sizeof(addp)))
> + return -EFAULT;
> +
> + if (!IS_ALIGNED(addp.offset, PAGE_SIZE) ||
> + !IS_ALIGNED(addp.src, PAGE_SIZE))
> + return -EINVAL;
> +
> + if (!(access_ok(addp.src, PAGE_SIZE)))
> + return -EFAULT;
> +
> + if (addp.length & (PAGE_SIZE - 1))
> + return -EINVAL;
How many pages are allowed? Unlimited? I'm hoping some limits are
checked somewhere...
> +
> + if (addp.offset + addp.length - PAGE_SIZE >= encl->size)
> + return -EINVAL;
> +
> + if (copy_from_user(&secinfo, (void __user *)addp.secinfo,
> + sizeof(secinfo)))
> + return -EFAULT;
> +
> + if (sgx_validate_secinfo(&secinfo))
> + return -EINVAL;
> +
> + for (c = 0 ; c < addp.length; c += PAGE_SIZE) {
> + if (signal_pending(current)) {
> + ret = -EINTR;
> + break;
> + }
> +
> + if (need_resched())
> + cond_resched();
> +
> + ret = sgx_encl_add_page(encl, addp.src + c, addp.offset + c,
> + addp.length - c, &secinfo, addp.flags);
> + if (ret)
> + break;
> + }
> +
> + addp.count = c;
> +
> + if (copy_to_user(arg, &addp, sizeof(addp)))
> + return -EFAULT;
> +
> + return ret;
> +}
> +
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply
* Re: [PATCH 06/14] umd: For clarity rename umh_info umd_info
From: Kees Cook @ 2020-06-26 15:37 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Linus Torvalds, David Miller, Greg Kroah-Hartman, Tetsuo Handa,
Alexei Starovoitov, Andrew Morton, Alexei Starovoitov, Al Viro,
bpf, linux-fsdevel, Daniel Borkmann, Jakub Kicinski,
Masahiro Yamada, Gary Lin, Bruno Meneguele, LSM List,
Casey Schaufler
In-Reply-To: <87o8p6f0kw.fsf_-_@x220.int.ebiederm.org>
On Fri, Jun 26, 2020 at 07:55:43AM -0500, Eric W. Biederman wrote:
> This structure is only used for user mode drivers so change
> the prefix from umh to umd to make that clear.
Should bpfilter_umh get renamed to bpfilter_umd at some point in this
series too?
--
Kees Cook
^ permalink raw reply
* [PATCH v8 1/2] acpi: Extend TPM2 ACPI table with missing log fields
From: Stefan Berger @ 2020-06-26 15:39 UTC (permalink / raw)
To: linux-integrity, linux-kernel, jarkko.sakkinen, linux-acpi,
linux-security-module
Cc: Stefan Berger, Rafael J . Wysocki
In-Reply-To: <20200626153948.2059251-1-stefanb@linux.vnet.ibm.com>
From: Stefan Berger <stefanb@linux.ibm.com>
Recent extensions of the TPM2 ACPI table added 3 more fields
including 12 bytes of start method specific parameters and Log Area
Minimum Length (u32) and Log Area Start Address (u64). So, we define
a new structure acpi_tpm2_phy that holds these optional new fields.
The new fields allow non-UEFI systems to access the TPM2's log.
The specification that has the new fields is the following:
TCG ACPI Specification
Family "1.2" and "2.0"
Version 1.2, Revision 8
https://trustedcomputinggroup.org/wp-content/uploads/TCG_ACPIGeneralSpecification_v1.20_r8.pdf
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
Cc: linux-acpi@vger.kernel.org
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
include/acpi/actbl3.h | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/include/acpi/actbl3.h b/include/acpi/actbl3.h
index b0b163b9efc6..bdcac69fa6bd 100644
--- a/include/acpi/actbl3.h
+++ b/include/acpi/actbl3.h
@@ -415,6 +415,13 @@ struct acpi_table_tpm2 {
/* Platform-specific data follows */
};
+/* Optional trailer for revision 4 holding platform-specific data */
+struct acpi_tpm2_phy {
+ u8 start_method_specific[12];
+ u32 log_area_minimum_length;
+ u64 log_area_start_address;
+};
+
/* Values for start_method above */
#define ACPI_TPM2_NOT_ALLOWED 0
--
2.26.2
^ permalink raw reply related
* [PATCH v8 0/2] tpm2: Make TPM2 logs accessible for non-UEFI firmware
From: Stefan Berger @ 2020-06-26 15:39 UTC (permalink / raw)
To: linux-integrity, linux-kernel, jarkko.sakkinen, linux-acpi,
linux-security-module
Cc: Stefan Berger
From: Stefan Berger <stefanb@linux.ibm.com>
This series of patches adds an optional extensions for the TPM2 ACPI table
with additional fields found in the TPM2 TCG ACPI specification (reference
is in the patch) that allow access to the log's address and its size. We
then modify the code that so far only enables access to a TPM 1.2's log for
a TPM2 as well. This then enables access to the TPM2's log on non-UEFI
system that for example run SeaBIOS.
Stefan
v7->v8:
- Added empty line.
v6->v7:
- Added empty lines and R-b.
v5->v6:
- Moved extensions of TPM2 table into acpi_tpm2_phy.
v4->v5:
- Added R-bs and A-bs.
v3->v4:
- Repost as one series
v2->v3:
- Split the series into two separate patches
- Added comments to ACPI table fields
- Added check for null pointer to log area and zero log size
v1->v2:
- Repost of the series
Stefan Berger (2):
acpi: Extend TPM2 ACPI table with missing log fields
tpm: Add support for event log pointer found in TPM2 ACPI table
drivers/char/tpm/eventlog/acpi.c | 63 +++++++++++++++++++++-----------
include/acpi/actbl3.h | 7 ++++
2 files changed, 49 insertions(+), 21 deletions(-)
--
2.26.2
^ permalink raw reply
* [PATCH v8 2/2] tpm: Add support for event log pointer found in TPM2 ACPI table
From: Stefan Berger @ 2020-06-26 15:39 UTC (permalink / raw)
To: linux-integrity, linux-kernel, jarkko.sakkinen, linux-acpi,
linux-security-module
Cc: Stefan Berger
In-Reply-To: <20200626153948.2059251-1-stefanb@linux.vnet.ibm.com>
From: Stefan Berger <stefanb@linux.ibm.com>
In case a TPM2 is attached, search for a TPM2 ACPI table when trying
to get the event log from ACPI. If one is found, use it to get the
start and length of the log area. This allows non-UEFI systems, such
as SeaBIOS, to pass an event log when using a TPM2.
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
drivers/char/tpm/eventlog/acpi.c | 63 +++++++++++++++++++++-----------
1 file changed, 42 insertions(+), 21 deletions(-)
diff --git a/drivers/char/tpm/eventlog/acpi.c b/drivers/char/tpm/eventlog/acpi.c
index 63ada5e53f13..51312c460335 100644
--- a/drivers/char/tpm/eventlog/acpi.c
+++ b/drivers/char/tpm/eventlog/acpi.c
@@ -49,9 +49,9 @@ int tpm_read_log_acpi(struct tpm_chip *chip)
void __iomem *virt;
u64 len, start;
struct tpm_bios_log *log;
-
- if (chip->flags & TPM_CHIP_FLAG_TPM2)
- return -ENODEV;
+ struct acpi_table_tpm2 *tbl;
+ struct acpi_tpm2_phy *t2phy;
+ int format;
log = &chip->log;
@@ -61,23 +61,44 @@ int tpm_read_log_acpi(struct tpm_chip *chip)
if (!chip->acpi_dev_handle)
return -ENODEV;
- /* Find TCPA entry in RSDT (ACPI_LOGICAL_ADDRESSING) */
- status = acpi_get_table(ACPI_SIG_TCPA, 1,
- (struct acpi_table_header **)&buff);
-
- if (ACPI_FAILURE(status))
- return -ENODEV;
-
- switch(buff->platform_class) {
- case BIOS_SERVER:
- len = buff->server.log_max_len;
- start = buff->server.log_start_addr;
- break;
- case BIOS_CLIENT:
- default:
- len = buff->client.log_max_len;
- start = buff->client.log_start_addr;
- break;
+ if (chip->flags & TPM_CHIP_FLAG_TPM2) {
+ status = acpi_get_table("TPM2", 1,
+ (struct acpi_table_header **)&tbl);
+ if (ACPI_FAILURE(status))
+ return -ENODEV;
+
+ if (tbl->header.length <
+ sizeof(*tbl) + sizeof(struct acpi_tpm2_phy))
+ return -ENODEV;
+
+ t2phy = (void *)tbl + sizeof(*tbl);
+ len = t2phy->log_area_minimum_length;
+
+ start = t2phy->log_area_start_address;
+ if (!start || !len)
+ return -ENODEV;
+
+ format = EFI_TCG2_EVENT_LOG_FORMAT_TCG_2;
+ } else {
+ /* Find TCPA entry in RSDT (ACPI_LOGICAL_ADDRESSING) */
+ status = acpi_get_table(ACPI_SIG_TCPA, 1,
+ (struct acpi_table_header **)&buff);
+ if (ACPI_FAILURE(status))
+ return -ENODEV;
+
+ switch (buff->platform_class) {
+ case BIOS_SERVER:
+ len = buff->server.log_max_len;
+ start = buff->server.log_start_addr;
+ break;
+ case BIOS_CLIENT:
+ default:
+ len = buff->client.log_max_len;
+ start = buff->client.log_start_addr;
+ break;
+ }
+
+ format = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
}
if (!len) {
dev_warn(&chip->dev, "%s: TCPA log area empty\n", __func__);
@@ -98,7 +119,7 @@ int tpm_read_log_acpi(struct tpm_chip *chip)
memcpy_fromio(log->bios_event_log, virt, len);
acpi_os_unmap_iomem(virt, len);
- return EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
+ return format;
err:
kfree(log->bios_event_log);
--
2.26.2
^ permalink raw reply related
* Re: [PATCH 05/14] umh: Separate the user mode driver and the user mode helper support
From: Tetsuo Handa @ 2020-06-26 16:22 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Linus Torvalds, David Miller, Greg Kroah-Hartman,
Alexei Starovoitov, Kees Cook, Andrew Morton, Alexei Starovoitov,
Al Viro, bpf, linux-fsdevel, Daniel Borkmann, Jakub Kicinski,
Masahiro Yamada, Gary Lin, Bruno Meneguele, LSM List,
Casey Schaufler
In-Reply-To: <87tuyyf0ln.fsf_-_@x220.int.ebiederm.org>
On 2020/06/26 21:55, Eric W. Biederman wrote:
> +static void umd_cleanup(struct subprocess_info *info)
> +{
> + struct umh_info *umh_info = info->data;
> +
> + /* cleanup if umh_pipe_setup() was successful but exec failed */
s/umh_pipe_setup/umd_setup/
> + if (info->retval) {
> + fput(umh_info->pipe_to_umh);
> + fput(umh_info->pipe_from_umh);
> + }
> +}
After this cleanup, I expect adding some protections/isolation which kernel threads
have (e.g. excluded from ptrace(), excluded from OOM victim selection, excluded from
SysRq-i, won't be terminated by SIGKILL from usermode processes, won't be stopped by
SIGSTOP from usermode processes, what else?). Doing it means giving up Alexei's
It's nice to be able to compile that blob with -g and be able to 'gdb -p' into it.
That works and very convenient when it comes to debugging. Compare that to debugging
a kernel module!
but I think doing it is essential for keeping usermode blob processes as secure/robust
as kernel threads.
^ permalink raw reply
* Re: [PATCH 06/14] umd: For clarity rename umh_info umd_info
From: Eric W. Biederman @ 2020-06-26 16:31 UTC (permalink / raw)
To: Kees Cook
Cc: Linus Torvalds, David Miller, Greg Kroah-Hartman, Tetsuo Handa,
Alexei Starovoitov, Andrew Morton, Alexei Starovoitov, Al Viro,
bpf, linux-fsdevel, Daniel Borkmann, Jakub Kicinski,
Masahiro Yamada, Gary Lin, Bruno Meneguele, LSM List,
Casey Schaufler
In-Reply-To: <202006260836.FB867484@keescook>
Kees Cook <keescook@chromium.org> writes:
> On Fri, Jun 26, 2020 at 07:55:43AM -0500, Eric W. Biederman wrote:
>> This structure is only used for user mode drivers so change
>> the prefix from umh to umd to make that clear.
>
> Should bpfilter_umh get renamed to bpfilter_umd at some point in this
> series too?
I think it would make a natural follow on, in a patches welcome sort of
way.
In this series I think it is important to draw a clear line between the
user mode driver infrastructure and the more general user mode helper
infrastructure. As it fundamentally makes a difference when you are
skimming through the code trying to find the details you care about.
But that line, which removes the maintenance burden from everyone else
is where this series stops.
I will be reposting shortly to fix the build issue I overlooked.
Eric
^ permalink raw reply
* Re: [PATCH 00/14] Make the user mode driver code a better citizen
From: Alexei Starovoitov @ 2020-06-26 16:40 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Linus Torvalds, David Miller, Greg Kroah-Hartman, Tetsuo Handa,
Kees Cook, Andrew Morton, Alexei Starovoitov, Al Viro, bpf,
linux-fsdevel, Daniel Borkmann, Jakub Kicinski, Masahiro Yamada,
Gary Lin, Bruno Meneguele, LSM List, Casey Schaufler
In-Reply-To: <87pn9mgfc2.fsf_-_@x220.int.ebiederm.org>
On Fri, Jun 26, 2020 at 07:51:41AM -0500, Eric W. Biederman wrote:
>
> Asking for people to fix their bugs in this user mode driver code has
> been remarkably unproductive. So here are my bug fixes.
>
> I have tested them by booting with the code compiled in and
> by killing "bpfilter_umh" and running iptables -vnL to restart
> the userspace driver.
>
> I have split the changes into small enough pieces so they should be
> easily readable and testable.
>
> The changes lean into the preexisting interfaces in the kernel and
> remove special cases for user mode driver code in favor of solutions
> that don't need special cases. This results in smaller code with
> fewer bugs.
>
> At a practical level this removes the maintenance burden of the
> user mode drivers from the user mode helper code and from exec as
> the special cases are removed.
>
> Similarly the LSM interaction bugs are fixed by not having unnecessary
> special cases for user mode drivers.
>
> Please let me know if you see any bugs. Once the code review is
> finished I plan to take this through my tree.
I did a quick look and I like the cleanup. Thanks! The end result looks good.
The only problem that you keep breaking the build between patches,
so series will not be bisectable.
blob_to_mnt is a great idea. Much better than embedding fs you advocated earlier.
I'm swamped with other stuff today and will test the set Sunday/Monday
with other patches that I'm working on.
I'm not sure why you want to rename the interface. Seems pointless. But fine.
As far as routing trees. Do you mind I'll take it via bpf-next ?
As I said countless times we're working on bpf_iter using fork_blob.
If you take this set via your tree we would need to wait the whole kernel release.
Which is 8+ weeks before we can use the interface (due to renaming and overall changes).
I'd really like to avoid this huge delay.
Unless you can land it into 5.8-rc2 or rc3.
^ permalink raw reply
* Re: [PATCH 05/14] umh: Separate the user mode driver and the user mode helper support
From: Eric W. Biederman @ 2020-06-26 16:45 UTC (permalink / raw)
To: Tetsuo Handa
Cc: Linus Torvalds, David Miller, Greg Kroah-Hartman,
Alexei Starovoitov, Kees Cook, Andrew Morton, Alexei Starovoitov,
Al Viro, bpf, linux-fsdevel, Daniel Borkmann, Jakub Kicinski,
Masahiro Yamada, Gary Lin, Bruno Meneguele, LSM List,
Casey Schaufler
In-Reply-To: <5ce4d340-096a-f468-6719-4c34a951511e@i-love.sakura.ne.jp>
Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> writes:
> On 2020/06/26 21:55, Eric W. Biederman wrote:
>> +static void umd_cleanup(struct subprocess_info *info)
>> +{
>> + struct umh_info *umh_info = info->data;
>> +
>> + /* cleanup if umh_pipe_setup() was successful but exec failed */
>
> s/umh_pipe_setup/umd_setup/
Good catch. I will fix that when I respin.
>> + if (info->retval) {
>> + fput(umh_info->pipe_to_umh);
>> + fput(umh_info->pipe_from_umh);
>> + }
>> +}
>
> After this cleanup, I expect adding some protections/isolation which kernel threads
> have (e.g. excluded from ptrace(), excluded from OOM victim selection, excluded from
> SysRq-i, won't be terminated by SIGKILL from usermode processes, won't be stopped by
> SIGSTOP from usermode processes, what else?). Doing it means giving up Alexei's
>
> It's nice to be able to compile that blob with -g and be able to 'gdb -p' into it.
> That works and very convenient when it comes to debugging. Compare that to debugging
> a kernel module!
>
> but I think doing it is essential for keeping usermode blob processes as secure/robust
> as kernel threads.
Do you have an application for a user mode driver?
I think concerns like that are best addressed in the context of a
specific driver/usecase. Just to make certain we are solving the right
problems.
My sense is that an advantage of user mode drivers can safely be buggier
than kernel drivers and the freedom to kill them when the drivers go
wrong (knowing the drivers will restart) is important.
Does this series by using the normal path through exec solve your
concerns with LSMs being able to identify these processes (both
individually and as class)?.
Eric
^ permalink raw reply
* Re: [PATCH 00/14] Make the user mode driver code a better citizen
From: Eric W. Biederman @ 2020-06-26 17:17 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Linus Torvalds, David Miller, Greg Kroah-Hartman, Tetsuo Handa,
Kees Cook, Andrew Morton, Alexei Starovoitov, Al Viro, bpf,
linux-fsdevel, Daniel Borkmann, Jakub Kicinski, Masahiro Yamada,
Gary Lin, Bruno Meneguele, LSM List, Casey Schaufler
In-Reply-To: <20200626164055.5iasnou57yrtt6wz@ast-mbp.dhcp.thefacebook.com>
Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:
> On Fri, Jun 26, 2020 at 07:51:41AM -0500, Eric W. Biederman wrote:
>>
>> Asking for people to fix their bugs in this user mode driver code has
>> been remarkably unproductive. So here are my bug fixes.
>>
>> I have tested them by booting with the code compiled in and
>> by killing "bpfilter_umh" and running iptables -vnL to restart
>> the userspace driver.
>>
>> I have split the changes into small enough pieces so they should be
>> easily readable and testable.
>>
>> The changes lean into the preexisting interfaces in the kernel and
>> remove special cases for user mode driver code in favor of solutions
>> that don't need special cases. This results in smaller code with
>> fewer bugs.
>>
>> At a practical level this removes the maintenance burden of the
>> user mode drivers from the user mode helper code and from exec as
>> the special cases are removed.
>>
>> Similarly the LSM interaction bugs are fixed by not having unnecessary
>> special cases for user mode drivers.
>>
>> Please let me know if you see any bugs. Once the code review is
>> finished I plan to take this through my tree.
>
> I did a quick look and I like the cleanup. Thanks!
Good then we have a path forward.
> The end result looks good.
> The only problem that you keep breaking the build between patches,
> so series will not be bisectable.
Keep breaking? There is an issue with patch 5/14 where the build breaks
when bpfilter is not enabled. Do you see any others? I know I tested
each patch individually. But I was only testing with CONFIG_BPFILTER
enabled so I missed one.
So there should not be things that break
but things slip through occassionally.
I will resend this shortly with the fix and any others that I can find.
> blob_to_mnt is a great idea. Much better than embedding fs you advocated earlier.
I was lazy and not overdesigning but I still suspect the blob will
benefit from becoming a cpio in the future.
> I'm swamped with other stuff today and will test the set Sunday/Monday
> with other patches that I'm working on.
> I'm not sure why you want to rename the interface. Seems
> pointless. But fine.
For maintainability I think the code very much benefits from a clear
separation between the user mode driver code from the user mode helper
code.
> As far as routing trees. Do you mind I'll take it via bpf-next ?
> As I said countless times we're working on bpf_iter using fork_blob.
> If you take this set via your tree we would need to wait the whole kernel release.
> Which is 8+ weeks before we can use the interface (due to renaming and overall changes).
> I'd really like to avoid this huge delay.
> Unless you can land it into 5.8-rc2 or rc3.
I also want to build upon this code.
How about when the review is done I post a frozen branch based on
v5.8-rc1 that you can merge into the bpf-next tree, and I can merge into
my branch. That way we both can build upon this code. That is the way
conflicts like this are usually handled.
Further I will leave any further enhancements to the user mode driver
infrastructure that people have suggested to you.
I will probably replace do_execve with a kernel_execve that doesn't need
set_fs() to copy the command line argument. I haven't seen Christoph
Hellwig address that yet, and it looks pretty straight foward at this
point.
Eric
^ permalink raw reply
* Re: [PATCH 00/14] Make the user mode driver code a better citizen
From: Alexei Starovoitov @ 2020-06-26 18:22 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Linus Torvalds, David Miller, Greg Kroah-Hartman, Tetsuo Handa,
Kees Cook, Andrew Morton, Alexei Starovoitov, Al Viro, bpf,
linux-fsdevel, Daniel Borkmann, Jakub Kicinski, Masahiro Yamada,
Gary Lin, Bruno Meneguele, LSM List, Casey Schaufler
In-Reply-To: <87sgeh926j.fsf@x220.int.ebiederm.org>
On Fri, Jun 26, 2020 at 12:17:40PM -0500, Eric W. Biederman wrote:
>
> > I'm swamped with other stuff today and will test the set Sunday/Monday
> > with other patches that I'm working on.
> > I'm not sure why you want to rename the interface. Seems
> > pointless. But fine.
>
> For maintainability I think the code very much benefits from a clear
> separation between the user mode driver code from the user mode helper
> code.
you mean different name gives that separation? makes sense.
> > As far as routing trees. Do you mind I'll take it via bpf-next ?
> > As I said countless times we're working on bpf_iter using fork_blob.
> > If you take this set via your tree we would need to wait the whole kernel release.
> > Which is 8+ weeks before we can use the interface (due to renaming and overall changes).
> > I'd really like to avoid this huge delay.
> > Unless you can land it into 5.8-rc2 or rc3.
>
> I also want to build upon this code.
>
> How about when the review is done I post a frozen branch based on
> v5.8-rc1 that you can merge into the bpf-next tree, and I can merge into
> my branch. That way we both can build upon this code. That is the way
> conflicts like this are usually handled.
sure. that works too.
> Further I will leave any further enhancements to the user mode driver
> infrastructure that people have suggested to you.
ok
^ permalink raw reply
* [PATCH v2 00/11] ima: Fix rule parsing bugs and extend KEXEC_CMDLINE rule support
From: Tyler Hicks @ 2020-06-26 22:38 UTC (permalink / raw)
To: Mimi Zohar, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
Prakhar Srivastava, linux-kernel, linux-integrity,
linux-security-module, Janne Karhunen, Eric Biederman, kexec,
Casey Schaufler
This series ultimately extends the supported IMA rule conditionals for
the KEXEC_CMDLINE hook function. As of today, there's an imbalance in
IMA language conditional support for KEXEC_CMDLINE rules in comparison
to KEXEC_KERNEL_CHECK and KEXEC_INITRAMFS_CHECK rules. The KEXEC_CMDLINE
rules do not support *any* conditionals so you cannot have a sequence of
rules like this:
dont_measure func=KEXEC_KERNEL_CHECK obj_type=foo_t
dont_measure func=KEXEC_INITRAMFS_CHECK obj_type=foo_t
dont_measure func=KEXEC_CMDLINE obj_type=foo_t
measure func=KEXEC_KERNEL_CHECK
measure func=KEXEC_INITRAMFS_CHECK
measure func=KEXEC_CMDLINE
Instead, KEXEC_CMDLINE rules can only be measured or not measured and
there's no additional flexibility in today's implementation of the
KEXEC_CMDLINE hook function.
With this series, the above sequence of rules becomes valid and any
calls to kexec_file_load() with a kernel and initramfs inode type of
foo_t will not be measured (that includes the kernel cmdline buffer)
while all other objects given to a kexec_file_load() syscall will be
measured. There's obviously not an inode directly associated with the
kernel cmdline buffer but this patch series ties the inode based
decision making for KEXEC_CMDLINE to the kernel's inode. I think this
will be intuitive to policy authors.
While reading IMA code and preparing to make this change, I realized
that the buffer based hook functions (KEXEC_CMDLINE and KEY_CHECK) are
quite special in comparison to longer standing hook functions. These
buffer based hook functions can only support measure actions and there
are some restrictions on the conditionals that they support. However,
the rule parser isn't enforcing any of those restrictions and IMA policy
authors wouldn't have any immediate way of knowing that the policy that
they wrote is invalid. For example, the sequence of rules above parses
successfully in today's kernel but the
"dont_measure func=KEXEC_CMDLINE ..." rule is incorrectly handled in
ima_match_rules(). The dont_measure rule is *always* considered to be a
match so, surprisingly, no KEXEC_CMDLINE measurements are made.
While making the rule parser more strict, I realized that the parser
does not correctly free all of the allocated memory associated with an
ima_rule_entry when going down some error paths. Invalid policy loaded
by the policy administrator could result in small memory leaks.
I envision patches 1-6 going to stable. The series is ordered in a way
that has all the fixes up front, followed by cleanups, followed by the
feature patch. The breakdown of patches looks like so:
Memory leak fixes: 1-3
Parser strictness fixes: 4-6
Code cleanups made possible by the fixes: 7-10
Extend KEXEC_CMDLINE rule support: 11
Perhaps the most logical ordering for code review is:
1, 2, 3, 7, 8, 4, 5, 6, 9, 10, 11
If you'd like me to re-order or split up the series, just let me know.
Thanks for considering these patches!
* Series-wide v2 changes
- Rebased onto next-integrity-testing
- Squashed patches 2 and 3 from v1
+ Updated this cover letter to account for changes to patch index
changes
+ See patch 2 for specific code changes
Tyler
Tyler Hicks (11):
ima: Have the LSM free its audit rule
ima: Free the entire rule when deleting a list of rules
ima: Free the entire rule if it fails to parse
ima: Fail rule parsing when buffer hook functions have an invalid
action
ima: Fail rule parsing when the KEXEC_CMDLINE hook is combined with an
invalid cond
ima: Fail rule parsing when the KEY_CHECK hook is combined with an
invalid cond
ima: Shallow copy the args_p member of ima_rule_entry.lsm elements
ima: Use correct type for the args_p member of ima_rule_entry.lsm
elements
ima: Move validation of the keyrings conditional into
ima_validate_rule()
ima: Use the common function to detect LSM conditionals in a rule
ima: Support additional conditionals in the KEXEC_CMDLINE hook
function
include/linux/ima.h | 4 +-
kernel/kexec_file.c | 2 +-
security/integrity/ima/ima.h | 7 +-
security/integrity/ima/ima_api.c | 2 +-
security/integrity/ima/ima_appraise.c | 2 +-
security/integrity/ima/ima_asymmetric_keys.c | 2 +-
security/integrity/ima/ima_main.c | 23 ++-
security/integrity/ima/ima_policy.c | 161 ++++++++++++++-----
security/integrity/ima/ima_queue_keys.c | 2 +-
9 files changed, 148 insertions(+), 57 deletions(-)
--
2.25.1
^ permalink raw reply
* [PATCH v2 01/11] ima: Have the LSM free its audit rule
From: Tyler Hicks @ 2020-06-26 22:38 UTC (permalink / raw)
To: Mimi Zohar, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
Prakhar Srivastava, linux-kernel, linux-integrity,
linux-security-module, Janne Karhunen, Casey Schaufler
In-Reply-To: <20200626223900.253615-1-tyhicks@linux.microsoft.com>
Ask the LSM to free its audit rule rather than directly calling kfree().
Both AppArmor and SELinux do additional work in their audit_rule_free()
hooks. Fix memory leaks by allowing the LSMs to perform necessary work.
Fixes: b16942455193 ("ima: use the lsm policy update notifier")
Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
Cc: Janne Karhunen <janne.karhunen@gmail.com>
Cc: Casey Schaufler <casey@schaufler-ca.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
---
* v2
- Fixed build warning by dropping the 'return -EINVAL' from
the stubbed out security_filter_rule_free() since it has a void
return type
- Added Mimi's Reviewed-by
- Developed a follow-on patch to rename security_filter_rule_*()
functions, to address Casey's request, but I'll submit it
independently of this patch series since it is somewhat unrelated
security/integrity/ima/ima.h | 5 +++++
security/integrity/ima/ima_policy.c | 2 +-
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 4515975cc540..59ec28f5c117 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -420,6 +420,7 @@ static inline void ima_free_modsig(struct modsig *modsig)
#ifdef CONFIG_IMA_LSM_RULES
#define security_filter_rule_init security_audit_rule_init
+#define security_filter_rule_free security_audit_rule_free
#define security_filter_rule_match security_audit_rule_match
#else
@@ -430,6 +431,10 @@ static inline int security_filter_rule_init(u32 field, u32 op, char *rulestr,
return -EINVAL;
}
+static inline void security_filter_rule_free(void *lsmrule)
+{
+}
+
static inline int security_filter_rule_match(u32 secid, u32 field, u32 op,
void *lsmrule)
{
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 66aa3e17a888..d7c268c2b0ce 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -258,7 +258,7 @@ static void ima_lsm_free_rule(struct ima_rule_entry *entry)
int i;
for (i = 0; i < MAX_LSM_RULES; i++) {
- kfree(entry->lsm[i].rule);
+ security_filter_rule_free(entry->lsm[i].rule);
kfree(entry->lsm[i].args_p);
}
kfree(entry);
--
2.25.1
^ permalink raw reply related
* [PATCH v2 02/11] ima: Free the entire rule when deleting a list of rules
From: Tyler Hicks @ 2020-06-26 22:38 UTC (permalink / raw)
To: Mimi Zohar, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
Prakhar Srivastava, linux-kernel, linux-integrity,
linux-security-module
In-Reply-To: <20200626223900.253615-1-tyhicks@linux.microsoft.com>
Create a function, ima_free_rule(), to free all memory associated with
an ima_rule_entry. Use the new function to fix memory leaks of allocated
ima_rule_entry members, such as .fsname and .keyrings, when deleting a
list of rules.
Make the existing ima_lsm_free_rule() function specific to the LSM
audit rule array of an ima_rule_entry and require that callers make an
additional call to kfree to free the ima_rule_entry itself.
This fixes a memory leak seen when loading by a valid rule that contains
an additional piece of allocated memory, such as an fsname, followed by
an invalid rule that triggers a policy load failure:
# echo -e "dont_measure fsname=securityfs\nbad syntax" > \
/sys/kernel/security/ima/policy
-bash: echo: write error: Invalid argument
# echo scan > /sys/kernel/debug/kmemleak
# cat /sys/kernel/debug/kmemleak
unreferenced object 0xffff9bab67ca12c0 (size 16):
comm "bash", pid 684, jiffies 4295212803 (age 252.344s)
hex dump (first 16 bytes):
73 65 63 75 72 69 74 79 66 73 00 6b 6b 6b 6b a5 securityfs.kkkk.
backtrace:
[<00000000adc80b1b>] kstrdup+0x2e/0x60
[<00000000d504cb0d>] ima_parse_add_rule+0x7d4/0x1020
[<00000000444825ac>] ima_write_policy+0xab/0x1d0
[<000000002b7f0d6c>] vfs_write+0xde/0x1d0
[<0000000096feedcf>] ksys_write+0x68/0xe0
[<0000000052b544a2>] do_syscall_64+0x56/0xa0
[<000000007ead1ba7>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
Fixes: f1b08bbcbdaf ("ima: define a new policy condition based on the filesystem name")
Fixes: 2b60c0ecedf8 ("IMA: Read keyrings= option from the IMA policy")
Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
---
* v2
- Collapsed patch #2 from v1 of this series, into this patch. This
patch now introduces ima_free_rule().
- Existing callers of ima_lsm_free_rule() are doing so to free rules
after a successful or failed ima_lsm_copy_rule() and those callers
continue to directly call ima_lsm_copy_rule() rather than doing
explicit reference ownership and calling ima_free_rule().
- The kfree(entry) of ima_lsm_free_rule() was removed from that
function to make it focused on freeing the LSM references. Direct
callers of ima_lsm_free_rule() must now call kfree(entry) after
ima_lsm_free_rule().
- A comment was added in ima_lsm_update_rule() to clarify why
ima_free_rule() isn't being used.
security/integrity/ima/ima_policy.c | 29 ++++++++++++++++++++++++-----
1 file changed, 24 insertions(+), 5 deletions(-)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index d7c268c2b0ce..bf00b966e87f 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -261,6 +261,21 @@ static void ima_lsm_free_rule(struct ima_rule_entry *entry)
security_filter_rule_free(entry->lsm[i].rule);
kfree(entry->lsm[i].args_p);
}
+}
+
+static void ima_free_rule(struct ima_rule_entry *entry)
+{
+ if (!entry)
+ return;
+
+ /*
+ * entry->template->fields may be allocated in ima_parse_rule() but that
+ * reference is owned by the corresponding ima_template_desc element in
+ * the defined_templates list and cannot be freed here
+ */
+ kfree(entry->fsname);
+ kfree(entry->keyrings);
+ ima_lsm_free_rule(entry);
kfree(entry);
}
@@ -302,6 +317,7 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
out_err:
ima_lsm_free_rule(nentry);
+ kfree(nentry);
return NULL;
}
@@ -315,7 +331,14 @@ static int ima_lsm_update_rule(struct ima_rule_entry *entry)
list_replace_rcu(&entry->list, &nentry->list);
synchronize_rcu();
+ /*
+ * ima_lsm_copy_rule() shallow copied all references, except for the
+ * LSM references, from entry to nentry so we only want to free the LSM
+ * references and the entry itself. All other memory refrences will now
+ * be owned by nentry.
+ */
ima_lsm_free_rule(entry);
+ kfree(entry);
return 0;
}
@@ -1402,15 +1425,11 @@ ssize_t ima_parse_add_rule(char *rule)
void ima_delete_rules(void)
{
struct ima_rule_entry *entry, *tmp;
- int i;
temp_ima_appraise = 0;
list_for_each_entry_safe(entry, tmp, &ima_temp_rules, list) {
- for (i = 0; i < MAX_LSM_RULES; i++)
- kfree(entry->lsm[i].args_p);
-
list_del(&entry->list);
- kfree(entry);
+ ima_free_rule(entry);
}
}
--
2.25.1
^ permalink raw reply related
* [PATCH v2 11/11] ima: Support additional conditionals in the KEXEC_CMDLINE hook function
From: Tyler Hicks @ 2020-06-26 22:39 UTC (permalink / raw)
To: Mimi Zohar, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
Prakhar Srivastava, linux-kernel, linux-integrity,
linux-security-module, Eric Biederman, kexec
In-Reply-To: <20200626223900.253615-1-tyhicks@linux.microsoft.com>
Take the properties of the kexec kernel's inode and the current task
ownership into consideration when matching a KEXEC_CMDLINE operation to
the rules in the IMA policy. This allows for some uniformity when
writing IMA policy rules for KEXEC_KERNEL_CHECK, KEXEC_INITRAMFS_CHECK,
and KEXEC_CMDLINE operations.
Prior to this patch, it was not possible to write a set of rules like
this:
dont_measure func=KEXEC_KERNEL_CHECK obj_type=foo_t
dont_measure func=KEXEC_INITRAMFS_CHECK obj_type=foo_t
dont_measure func=KEXEC_CMDLINE obj_type=foo_t
measure func=KEXEC_KERNEL_CHECK
measure func=KEXEC_INITRAMFS_CHECK
measure func=KEXEC_CMDLINE
The inode information associated with the kernel being loaded by a
kexec_kernel_load(2) syscall can now be included in the decision to
measure or not
Additonally, the uid, euid, and subj_* conditionals can also now be
used in KEXEC_CMDLINE rules. There was no technical reason as to why
those conditionals weren't being considered previously other than
ima_match_rules() didn't have a valid inode to use so it immediately
bailed out for KEXEC_CMDLINE operations rather than going through the
full list of conditional comparisons.
Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
Cc: Eric Biederman <ebiederm@xmission.com>
Cc: kexec@lists.infradead.org
---
* v2
- Moved the inode parameter of process_buffer_measurement() to be the
first parameter so that it more closely matches process_masurement()
include/linux/ima.h | 4 ++--
kernel/kexec_file.c | 2 +-
security/integrity/ima/ima.h | 2 +-
security/integrity/ima/ima_api.c | 2 +-
security/integrity/ima/ima_appraise.c | 2 +-
security/integrity/ima/ima_asymmetric_keys.c | 2 +-
security/integrity/ima/ima_main.c | 23 +++++++++++++++-----
security/integrity/ima/ima_policy.c | 17 +++++----------
security/integrity/ima/ima_queue_keys.c | 2 +-
9 files changed, 31 insertions(+), 25 deletions(-)
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 9164e1534ec9..d15100de6cdd 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -25,7 +25,7 @@ extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
enum kernel_read_file_id id);
extern void ima_post_path_mknod(struct dentry *dentry);
extern int ima_file_hash(struct file *file, char *buf, size_t buf_size);
-extern void ima_kexec_cmdline(const void *buf, int size);
+extern void ima_kexec_cmdline(int kernel_fd, const void *buf, int size);
#ifdef CONFIG_IMA_KEXEC
extern void ima_add_kexec_buffer(struct kimage *image);
@@ -103,7 +103,7 @@ static inline int ima_file_hash(struct file *file, char *buf, size_t buf_size)
return -EOPNOTSUPP;
}
-static inline void ima_kexec_cmdline(const void *buf, int size) {}
+static inline void ima_kexec_cmdline(int kernel_fd, const void *buf, int size) {}
#endif /* CONFIG_IMA */
#ifndef CONFIG_IMA_KEXEC
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index bb05fd52de85..07df431c1f21 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -287,7 +287,7 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
goto out;
}
- ima_kexec_cmdline(image->cmdline_buf,
+ ima_kexec_cmdline(kernel_fd, image->cmdline_buf,
image->cmdline_buf_len - 1);
}
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 59ec28f5c117..ff2bf57ff0c7 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -265,7 +265,7 @@ void ima_store_measurement(struct integrity_iint_cache *iint, struct file *file,
struct evm_ima_xattr_data *xattr_value,
int xattr_len, const struct modsig *modsig, int pcr,
struct ima_template_desc *template_desc);
-void process_buffer_measurement(const void *buf, int size,
+void process_buffer_measurement(struct inode *inode, const void *buf, int size,
const char *eventname, enum ima_hooks func,
int pcr, const char *keyring);
void ima_audit_measurement(struct integrity_iint_cache *iint,
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index bf22de8b7ce0..4f39fb93f278 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -162,7 +162,7 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
/**
* ima_get_action - appraise & measure decision based on policy.
- * @inode: pointer to inode to measure
+ * @inode: pointer to the inode associated with the object being validated
* @cred: pointer to credentials structure to validate
* @secid: secid of the task being validated
* @mask: contains the permission mask (MAY_READ, MAY_WRITE, MAY_EXEC,
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index a9649b04b9f1..6c52bf7ea7f0 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -328,7 +328,7 @@ int ima_check_blacklist(struct integrity_iint_cache *iint,
rc = is_binary_blacklisted(digest, digestsize);
if ((rc == -EPERM) && (iint->flags & IMA_MEASURE))
- process_buffer_measurement(digest, digestsize,
+ process_buffer_measurement(NULL, digest, digestsize,
"blacklisted-hash", NONE,
pcr, NULL);
}
diff --git a/security/integrity/ima/ima_asymmetric_keys.c b/security/integrity/ima/ima_asymmetric_keys.c
index aaae80c4e376..1c68c500c26f 100644
--- a/security/integrity/ima/ima_asymmetric_keys.c
+++ b/security/integrity/ima/ima_asymmetric_keys.c
@@ -58,7 +58,7 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
* if the IMA policy is configured to measure a key linked
* to the given keyring.
*/
- process_buffer_measurement(payload, payload_len,
+ process_buffer_measurement(NULL, payload, payload_len,
keyring->description, KEY_CHECK, 0,
keyring->description);
}
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 8351b2fd48e0..8a91711ca79b 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -726,6 +726,7 @@ int ima_load_data(enum kernel_load_data_id id)
/*
* process_buffer_measurement - Measure the buffer to ima log.
+ * @inode: inode associated with the object being measured (NULL for KEY_CHECK)
* @buf: pointer to the buffer that needs to be added to the log.
* @size: size of buffer(in bytes).
* @eventname: event name to be used for the buffer entry.
@@ -735,7 +736,7 @@ int ima_load_data(enum kernel_load_data_id id)
*
* Based on policy, the buffer is measured into the ima log.
*/
-void process_buffer_measurement(const void *buf, int size,
+void process_buffer_measurement(struct inode *inode, const void *buf, int size,
const char *eventname, enum ima_hooks func,
int pcr, const char *keyring)
{
@@ -768,7 +769,7 @@ void process_buffer_measurement(const void *buf, int size,
*/
if (func) {
security_task_getsecid(current, &secid);
- action = ima_get_action(NULL, current_cred(), secid, 0, func,
+ action = ima_get_action(inode, current_cred(), secid, 0, func,
&pcr, &template, keyring);
if (!(action & IMA_MEASURE))
return;
@@ -823,16 +824,26 @@ void process_buffer_measurement(const void *buf, int size,
/**
* ima_kexec_cmdline - measure kexec cmdline boot args
+ * @kernel_fd: file descriptor of the kexec kernel being loaded
* @buf: pointer to buffer
* @size: size of buffer
*
* Buffers can only be measured, not appraised.
*/
-void ima_kexec_cmdline(const void *buf, int size)
+void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
{
- if (buf && size != 0)
- process_buffer_measurement(buf, size, "kexec-cmdline",
- KEXEC_CMDLINE, 0, NULL);
+ struct fd f;
+
+ if (!buf || !size)
+ return;
+
+ f = fdget(kernel_fd);
+ if (!f.file)
+ return;
+
+ process_buffer_measurement(file_inode(f.file), buf, size,
+ "kexec-cmdline", KEXEC_CMDLINE, 0, NULL);
+ fdput(f);
}
static int __init init_ima(void)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 5eb14b567a31..294323b36d06 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -443,13 +443,9 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
{
int i;
- if ((func == KEXEC_CMDLINE) || (func == KEY_CHECK)) {
- if ((rule->flags & IMA_FUNC) && (rule->func == func)) {
- if (func == KEY_CHECK)
- return ima_match_keyring(rule, keyring, cred);
- return true;
- }
- return false;
+ if (func == KEY_CHECK) {
+ return (rule->flags & IMA_FUNC) && (rule->func == func) &&
+ ima_match_keyring(rule, keyring, cred);
}
if ((rule->flags & IMA_FUNC) &&
(rule->func != func && func != POST_SETATTR))
@@ -1007,10 +1003,9 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
if (entry->action & ~(MEASURE | DONT_MEASURE))
return false;
- if (entry->flags & ~(IMA_FUNC | IMA_PCR))
- return false;
-
- if (ima_rule_contains_lsm_cond(entry))
+ if (entry->flags & ~(IMA_FUNC | IMA_FSMAGIC | IMA_UID |
+ IMA_FOWNER | IMA_FSUUID |
+ IMA_EUID | IMA_PCR | IMA_FSNAME))
return false;
break;
diff --git a/security/integrity/ima/ima_queue_keys.c b/security/integrity/ima/ima_queue_keys.c
index 56ce24a18b66..69a8626a35c0 100644
--- a/security/integrity/ima/ima_queue_keys.c
+++ b/security/integrity/ima/ima_queue_keys.c
@@ -158,7 +158,7 @@ void ima_process_queued_keys(void)
list_for_each_entry_safe(entry, tmp, &ima_keys, list) {
if (!timer_expired)
- process_buffer_measurement(entry->payload,
+ process_buffer_measurement(NULL, entry->payload,
entry->payload_len,
entry->keyring_name,
KEY_CHECK, 0,
--
2.25.1
^ permalink raw reply related
* [PATCH v2 09/11] ima: Move validation of the keyrings conditional into ima_validate_rule()
From: Tyler Hicks @ 2020-06-26 22:38 UTC (permalink / raw)
To: Mimi Zohar, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
Prakhar Srivastava, linux-kernel, linux-integrity,
linux-security-module
In-Reply-To: <20200626223900.253615-1-tyhicks@linux.microsoft.com>
Use ima_validate_rule() to ensure that the combination of a hook
function and the keyrings conditional is valid and that the keyrings
conditional is not specified without an explicit KEY_CHECK func
conditional. This is a code cleanup and has no user-facing change.
Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
---
* v2
- Allowed IMA_DIGSIG_REQUIRED, IMA_PERMIT_DIRECTIO,
IMA_MODSIG_ALLOWED, and IMA_CHECK_BLACKLIST conditionals to be
present in the rule entry flags for non-buffer hook functions.
security/integrity/ima/ima_policy.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 8cdca2399d59..43d49ad958fb 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -1000,6 +1000,15 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
case KEXEC_KERNEL_CHECK:
case KEXEC_INITRAMFS_CHECK:
case POLICY_CHECK:
+ if (entry->flags & ~(IMA_FUNC | IMA_MASK | IMA_FSMAGIC |
+ IMA_UID | IMA_FOWNER | IMA_FSUUID |
+ IMA_INMASK | IMA_EUID | IMA_PCR |
+ IMA_FSNAME | IMA_DIGSIG_REQUIRED |
+ IMA_PERMIT_DIRECTIO |
+ IMA_MODSIG_ALLOWED |
+ IMA_CHECK_BLACKLIST))
+ return false;
+
break;
case KEXEC_CMDLINE:
if (entry->action & ~(MEASURE | DONT_MEASURE))
@@ -1027,7 +1036,8 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
default:
return false;
}
- }
+ } else if (entry->flags & IMA_KEYRINGS)
+ return false;
return true;
}
@@ -1209,7 +1219,6 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
keyrings_len = strlen(args[0].from) + 1;
if ((entry->keyrings) ||
- (entry->func != KEY_CHECK) ||
(keyrings_len < 2)) {
result = -EINVAL;
break;
--
2.25.1
^ permalink raw reply related
* [PATCH v2 08/11] ima: Use correct type for the args_p member of ima_rule_entry.lsm elements
From: Tyler Hicks @ 2020-06-26 22:38 UTC (permalink / raw)
To: Mimi Zohar, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
Prakhar Srivastava, linux-kernel, linux-integrity,
linux-security-module
In-Reply-To: <20200626223900.253615-1-tyhicks@linux.microsoft.com>
Make args_p be of the char pointer type rather than have it be a void
pointer that gets casted to char pointer when it is used. It is a simple
NUL-terminated string as returned by match_strdup().
Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
---
* v2
- No change
security/integrity/ima/ima_policy.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index ef69c54266c6..8cdca2399d59 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -74,7 +74,7 @@ struct ima_rule_entry {
int pcr;
struct {
void *rule; /* LSM file metadata specific */
- void *args_p; /* audit value */
+ char *args_p; /* audit value */
int type; /* audit type */
} lsm[MAX_LSM_RULES];
char *fsname;
@@ -314,7 +314,7 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
&nentry->lsm[i].rule);
if (!nentry->lsm[i].rule)
pr_warn("rule for LSM \'%s\' is undefined\n",
- (char *)entry->lsm[i].args_p);
+ entry->lsm[i].args_p);
}
return nentry;
}
@@ -918,7 +918,7 @@ static int ima_lsm_rule_init(struct ima_rule_entry *entry,
&entry->lsm[lsm_rule].rule);
if (!entry->lsm[lsm_rule].rule) {
pr_warn("rule for LSM \'%s\' is undefined\n",
- (char *)entry->lsm[lsm_rule].args_p);
+ entry->lsm[lsm_rule].args_p);
if (ima_rules == &ima_default_rules) {
kfree(entry->lsm[lsm_rule].args_p);
@@ -1667,27 +1667,27 @@ int ima_policy_show(struct seq_file *m, void *v)
switch (i) {
case LSM_OBJ_USER:
seq_printf(m, pt(Opt_obj_user),
- (char *)entry->lsm[i].args_p);
+ entry->lsm[i].args_p);
break;
case LSM_OBJ_ROLE:
seq_printf(m, pt(Opt_obj_role),
- (char *)entry->lsm[i].args_p);
+ entry->lsm[i].args_p);
break;
case LSM_OBJ_TYPE:
seq_printf(m, pt(Opt_obj_type),
- (char *)entry->lsm[i].args_p);
+ entry->lsm[i].args_p);
break;
case LSM_SUBJ_USER:
seq_printf(m, pt(Opt_subj_user),
- (char *)entry->lsm[i].args_p);
+ entry->lsm[i].args_p);
break;
case LSM_SUBJ_ROLE:
seq_printf(m, pt(Opt_subj_role),
- (char *)entry->lsm[i].args_p);
+ entry->lsm[i].args_p);
break;
case LSM_SUBJ_TYPE:
seq_printf(m, pt(Opt_subj_type),
- (char *)entry->lsm[i].args_p);
+ entry->lsm[i].args_p);
break;
}
seq_puts(m, " ");
--
2.25.1
^ permalink raw reply related
* [PATCH v2 07/11] ima: Shallow copy the args_p member of ima_rule_entry.lsm elements
From: Tyler Hicks @ 2020-06-26 22:38 UTC (permalink / raw)
To: Mimi Zohar, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
Prakhar Srivastava, linux-kernel, linux-integrity,
linux-security-module
In-Reply-To: <20200626223900.253615-1-tyhicks@linux.microsoft.com>
The args_p member is a simple string that is allocated by
ima_rule_init(). Shallow copy it like other non-LSM references in
ima_rule_entry structs.
There are no longer any necessary error path cleanups to do in
ima_lsm_copy_rule().
Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
---
* v2
- Adjusted context to account for ima_lsm_copy_rule() directly calling
ima_lsm_free_rule() and the lack of explicit reference ownership
transfers
- Added comment to ima_lsm_copy_rule() to document the args_p
reference ownership transfer
security/integrity/ima/ima_policy.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index f9b1bdb897da..ef69c54266c6 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -300,10 +300,13 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
continue;
nentry->lsm[i].type = entry->lsm[i].type;
- nentry->lsm[i].args_p = kstrdup(entry->lsm[i].args_p,
- GFP_KERNEL);
- if (!nentry->lsm[i].args_p)
- goto out_err;
+ nentry->lsm[i].args_p = entry->lsm[i].args_p;
+ /*
+ * Remove the reference from entry so that the associated
+ * memory will not be freed during a later call to
+ * ima_lsm_free_rule(entry).
+ */
+ entry->lsm[i].args_p = NULL;
security_filter_rule_init(nentry->lsm[i].type,
Audit_equal,
@@ -314,11 +317,6 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
(char *)entry->lsm[i].args_p);
}
return nentry;
-
-out_err:
- ima_lsm_free_rule(nentry);
- kfree(nentry);
- return NULL;
}
static int ima_lsm_update_rule(struct ima_rule_entry *entry)
--
2.25.1
^ permalink raw reply related
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