* Re: linux-next: umh: fix processed error when UMH_WAIT_PROC is used seems to break linux bridge on s390x (bisected)
From: Christian Borntraeger @ 2020-06-25 13:26 UTC (permalink / raw)
To: Luis Chamberlain
Cc: Christoph Hellwig, ast, axboe, bfields, bridge, chainsaw,
christian.brauner, chuck.lever, davem, dhowells, gregkh,
jarkko.sakkinen, jmorris, josh, keescook, keyrings, kuba,
lars.ellenberg, linux-fsdevel, linux-kernel, linux-nfs,
linux-security-module, nikolay, philipp.reisner, ravenexp, roopa,
serge, slyfox, viro, yangtiezhu, netdev, markward, linux-s390
In-Reply-To: <1263e370-7cee-24d8-b98c-117bf7c90a83@de.ibm.com>
On 24.06.20 20:37, Christian Borntraeger wrote:
>
>
> On 24.06.20 20:32, Christian Borntraeger wrote:
> [...]>
>> So the translations look correct. But your change is actually a sematic change
>> if(ret) will only trigger if there is an error
>> if (KWIFEXITED(ret)) will always trigger when the process ends. So we will always overwrite -ECHILD
>> and we did not do it before.
>>
>
> So the right fix is
>
> diff --git a/kernel/umh.c b/kernel/umh.c
> index f81e8698e36e..a3a3196e84d1 100644
> --- a/kernel/umh.c
> +++ b/kernel/umh.c
> @@ -154,7 +154,7 @@ static void call_usermodehelper_exec_sync(struct subprocess_info *sub_info)
> * the real error code is already in sub_info->retval or
> * sub_info->retval is 0 anyway, so don't mess with it then.
> */
> - if (KWIFEXITED(ret))
> + if (KWEXITSTATUS(ret))
> sub_info->retval = KWEXITSTATUS(ret);
> }
Ping. Shall I send this as a proper patch or are we merging this fixup in Andrews patch queue?
^ permalink raw reply
* Re: [RFC][PATCH] net/bpfilter: Remove this broken and apparently unmantained
From: Tetsuo Handa @ 2020-06-25 14:21 UTC (permalink / raw)
To: Greg KH
Cc: Alexei Starovoitov, Eric W. Biederman, Linus Torvalds, Kees Cook,
Andrew Morton, Alexei Starovoitov, David Miller, Al Viro, bpf,
linux-fsdevel, Daniel Borkmann, Jakub Kicinski, Masahiro Yamada,
Gary Lin, Bruno Meneguele, linux-security-module, Casey Schaufler
In-Reply-To: <20200625120725.GA3493334@kroah.com>
On 2020/06/25 21:07, Greg KH wrote:
>>>> call_usermodehelper() can teach LSM modules via pre-existing file's pathname and
>>>> inode's security label at security_bprm_creds_for_exec()/security_bprm_check() etc.
>>>> But since fork_usermode_blob() accepts only "byte array" and "length of byte array"
>>>> arguments, I'm not sure whether LSM modules can obtain information needed for
>>>> inspection. How does fork_usermode_blob() tell that information?
>>>
>>> It would seem that the "security context" for those would be the same as
>>> anything created before userspace launches today, right? You handle
>>> that ok, and this should be just the same.
>>
>> I don't think so. Today when call_usermodehelper() is called, LSMs switch their security
>> context (at least TOMOYO does it) for further syscalls from the usermode process started
>> by the kernel context. But when fork_usermode_blob() is called, how LSMs can switch their
>> security context for further syscalls from the usermode process started by the kernel
>> context?
>
> Ok, that makes a bit more sense. Why not just do the same thing that
> you do today with call_usermodehelper()? The logic in a way is the
> same, right?
>
call_usermodehelper() provides information like "the kernel is about to run
/sbin/modprobe in order to load foo module" but fork_usermode_blob() does not
provide information like "the kernel is about to run a blob that we think is
a userspace USB IR filter driver". That is unfriendly to LSM modules.
>
>>> Right now I, as a kernel module, can read/write to any file in the
>>> system, and do all sorts of other fun things. You can't mediate that
>>> today from a LSM, and this is just one other example of this.
>>
>> Some functions (e.g. kernel_sock_shutdown()) bypass permission checks by LSMs
>> comes from a sort of trustness that the byte array kept inside kernel address
>> space remains secure/intact.
>
> And what is going to change that "trustness" here? The byte array came
> from the kernel address space to start with. Are you thinking something
> outside of the kernel will then tamper with those bytes to do something
> else with them?
Right. e.g. ptrace() will allow reading/writing those bytes to do something
else with them. I guess 'gdb -p' is the same meaning.
> If so, shouldn't you be preventing that userspace
> program that does the tampering from doing that in the first place with
> the LSM running?
SELinux can handle process isolation very well. But the reality is that none of customers
I'm working for can afford using SELinux because SELinux is too complicated to support.
Instead, they use proprietary antivirus kernel modules (which tamper with syscall tables
and/or security_hook_heads). Therefore, I wish that isolation between processes running
fork_usermode_blob() and processes running normal usermode programs is implemented by
built-in mechanism (like DAC), and I said
We might need to invent built-in "protected userspace" because existing
"unprotected userspace" is not trustworthy enough to run kernel modules.
That's not just inventing fork_usermode_blob().
at https://lkml.kernel.org/r/62859212-df69-b913-c1e0-cd2e358d1adf@i-love.sakura.ne.jp .
I'm happy if we can implement such isolation without counting on in-tree LSMs.
^ permalink raw reply
* Re: [RFC][PATCH] net/bpfilter: Remove this broken and apparently unmantained
From: Stephen Smalley @ 2020-06-25 14:26 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Tetsuo Handa, Alexei Starovoitov, Eric W. Biederman,
Linus Torvalds, Kees Cook, Andrew Morton, Alexei Starovoitov,
David Miller, Al Viro, bpf, linux-fsdevel, Daniel Borkmann,
Jakub Kicinski, Masahiro Yamada, Gary Lin, Bruno Meneguele,
linux-security-module, Casey Schaufler
In-Reply-To: <20200625132551.GB3526980@kroah.com>
On Thu, Jun 25, 2020 at 9:25 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Jun 25, 2020 at 08:56:10AM -0400, Stephen Smalley wrote:
> > No, because we cannot label the inode based on the program's purpose
> > and therefore cannot configure an automatic transition to a suitable
> > security context for the process, unlike call_usermodehelper().
>
> Why, what prevents this? Can you not just do that based on the "blob
> address" or signature of it or something like that? Right now you all
> do this based on inode of a random file on a disk, what's the difference
> between a random blob in memory?
Given some kind of key to identify the blob and look up a suitable
context in policy, I think it would work. We just don't have that
with the current interface. With /bin/kmod and the like, we have a
security xattr assigned to the file when it was created that we can
use as the basis for determining the process security context.
> > On a different note, will the usermode blob be measured by IMA prior
> > to execution? What ensures that the blob was actually embedded in the
> > kernel image and wasn't just supplied as data through exploitation of
> > a kernel vulnerability or malicious kernel module?
>
> No reason it couldn't be passed to IMA for measuring, if people want to
> do that.
Actually, I think it probably happens already via IMA's existing hooks
but just wanted to confirm that IMA doesn't ignore S_PRIVATE inodes.
^ permalink raw reply
* Re: [RFC][PATCH] net/bpfilter: Remove this broken and apparently unmantained
From: Stephen Smalley @ 2020-06-25 14:36 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Tetsuo Handa, Alexei Starovoitov, Eric W. Biederman,
Linus Torvalds, Kees Cook, Andrew Morton, Alexei Starovoitov,
David Miller, Al Viro, bpf, linux-fsdevel, Daniel Borkmann,
Jakub Kicinski, Masahiro Yamada, Gary Lin, Bruno Meneguele,
linux-security-module, Casey Schaufler
In-Reply-To: <CAEjxPJ6MEb--R=zP_wCh-zgCochgcPhy7Fp7ENTYKB2NH9c6PA@mail.gmail.com>
On Thu, Jun 25, 2020 at 10:26 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Thu, Jun 25, 2020 at 9:25 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, Jun 25, 2020 at 08:56:10AM -0400, Stephen Smalley wrote:
> > > No, because we cannot label the inode based on the program's purpose
> > > and therefore cannot configure an automatic transition to a suitable
> > > security context for the process, unlike call_usermodehelper().
> >
> > Why, what prevents this? Can you not just do that based on the "blob
> > address" or signature of it or something like that? Right now you all
> > do this based on inode of a random file on a disk, what's the difference
> > between a random blob in memory?
>
> Given some kind of key to identify the blob and look up a suitable
> context in policy, I think it would work. We just don't have that
> with the current interface. With /bin/kmod and the like, we have a
> security xattr assigned to the file when it was created that we can
> use as the basis for determining the process security context.
Looks like info->cmdline could be used as that key if set; we would
just need a LSM hook to permit setting up the inode's security state
based on that key. If that were passed to shmem_kernel_file_setup()
as the name argument, then that might also help path-based LSMs
although it seems potentially ambiguous.
^ permalink raw reply
* Enabling interrupts in QEMU TPM TIS
From: Stefan Berger @ 2020-06-25 14:56 UTC (permalink / raw)
To: Jerry Snitselaar, linux-integrity, Jarkko Sakkinen; +Cc: LSM List, LKML
Hello!
I want to enable IRQs now in QEMU's TPM TIS device model and I need to
work with the following patch to Linux TIS. I am wondering whether the
changes there look reasonable to you? Windows works with the QEMU
modifications as-is, so maybe it's a bug in the TIS code (which I had
not run into before).
The point of the loop I need to introduce in the interrupt handler is
that while the interrupt handler is running another interrupt may
occur/be posted that then does NOT cause the interrupt handler to be
invoked again but causes a stall, unless the loop is there.
The 'o' in the dmesg log indicates the original IRQ for which the
handler was invoked. The interrupt values have the following meaning.
0x2: STS valid
0x4: locality changed
0x80: command ready
So the first 'looping entry' [in log below] indicates that a locality
change interrupt occurred while the interrupt handler was running due to
STS_valid + command ready. This sounds reasonable considering that we
are frequently acquiring and releasing the locality. The loop then deals
with the locality change interrupt and the interrupts then settle.
[ 210.365129] tpm_tis MSFT0101:00: 2.0 TPM (device-id 0x1, rev-id 1)
[ 210.367124] looping: 0x4 (o: 0x82)
[ 212.375045] looping: 0x80 (o: 0x2)
[ 212.389218] looping: 0x4 (o: 0x82)
[ 212.404161] looping: 0x80 (o: 0x2)
[ 212.526427] looping: 0x4 (o: 0x82)
[ 212.595488] looping: 0x4 (o: 0x82)
[ 212.614357] looping: 0x80 (o: 0x2)
diff --git a/drivers/char/tpm/tpm_tis_core.c
b/drivers/char/tpm/tpm_tis_core.c
index 65ab1b027949..f77544563fb1 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -704,7 +704,7 @@ static irqreturn_t tis_int_handler(int dummy, void
*dev_id)
{
struct tpm_chip *chip = dev_id;
struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
- u32 interrupt;
+ u32 interrupt, o;
int i, rc;
rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &interrupt);
@@ -715,6 +715,7 @@ static irqreturn_t tis_int_handler(int dummy, void
*dev_id)
return IRQ_NONE;
priv->irq_tested = true;
+again:
if (interrupt & TPM_INTF_DATA_AVAIL_INT)
wake_up_interruptible(&priv->read_queue);
if (interrupt & TPM_INTF_LOCALITY_CHANGE_INT)
@@ -731,7 +732,12 @@ static irqreturn_t tis_int_handler(int dummy, void
*dev_id)
if (rc < 0)
return IRQ_NONE;
+ o = interrupt;
tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &interrupt);
+ if (interrupt != 0) {
+ printk("looping: 0x%x (o: 0x%x)\n", interrupt, o);
+ goto again;
+ }
return IRQ_HANDLED;
}
@@ -1062,6 +1068,8 @@ int tpm_tis_core_init(struct device *dev, struct
tpm_tis_data *priv, int irq,
goto out_err;
}
+ tpm_chip_start(chip);
+ chip->flags |= TPM_CHIP_FLAG_IRQ;
if (irq) {
tpm_tis_probe_irq_single(chip, intmask, IRQF_SHARED,
irq);
@@ -1074,6 +1082,7 @@ int tpm_tis_core_init(struct device *dev, struct
tpm_tis_data *priv, int irq,
} else {
tpm_tis_probe_irq(chip, intmask);
}
+ tpm_chip_stop(chip);
}
rc = tpm_chip_register(chip);
--
2.26.2
Stefan
^ permalink raw reply related
* Re: [RFC][PATCH] net/bpfilter: Remove this broken and apparently unmantained
From: Tetsuo Handa @ 2020-06-25 15:21 UTC (permalink / raw)
To: Stephen Smalley, Greg Kroah-Hartman, Alexei Starovoitov
Cc: Eric W. Biederman, Linus Torvalds, Kees Cook, Andrew Morton,
Alexei Starovoitov, David Miller, Al Viro, bpf, linux-fsdevel,
Daniel Borkmann, Jakub Kicinski, Masahiro Yamada, Gary Lin,
Bruno Meneguele, linux-security-module, Casey Schaufler
In-Reply-To: <CAEjxPJ6MEb--R=zP_wCh-zgCochgcPhy7Fp7ENTYKB2NH9c6PA@mail.gmail.com>
On 2020/06/25 23:26, Stephen Smalley wrote:
> On Thu, Jun 25, 2020 at 9:25 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
>>
>> On Thu, Jun 25, 2020 at 08:56:10AM -0400, Stephen Smalley wrote:
>>> No, because we cannot label the inode based on the program's purpose
>>> and therefore cannot configure an automatic transition to a suitable
>>> security context for the process, unlike call_usermodehelper().
>>
>> Why, what prevents this? Can you not just do that based on the "blob
>> address" or signature of it or something like that? Right now you all
>> do this based on inode of a random file on a disk, what's the difference
>> between a random blob in memory?
>
> Given some kind of key to identify the blob and look up a suitable
> context in policy, I think it would work. We just don't have that
> with the current interface. With /bin/kmod and the like, we have a
> security xattr assigned to the file when it was created that we can
> use as the basis for determining the process security context.
My understanding is that fork_usermode_blob() is intended to be able to run
without filesystems so that usermode blobs can start even before global init
program (pid=1) starts.
But SELinux's policy is likely stored inside filesystems which would be
accessible only after global init program (pid=1) started.
Therefore, I wonder whether SELinux can look up a suitable context in policy
even if "some kind of key to identify the blob" is provided.
Also, since (at least some of) usermode blob processes started via
fork_usermode_blob() will remain after SELinux loads policy from filesystems,
I guess that we will need a method for moving already running usermode blob
processes to appropriate security contexts.
Is my understanding/concerns correct?
^ permalink raw reply
* Re: [RFC][PATCH] net/bpfilter: Remove this broken and apparently unmantained
From: Stephen Smalley @ 2020-06-25 16:03 UTC (permalink / raw)
To: Tetsuo Handa
Cc: Greg Kroah-Hartman, Alexei Starovoitov, Eric W. Biederman,
Linus Torvalds, Kees Cook, Andrew Morton, Alexei Starovoitov,
David Miller, Al Viro, bpf, linux-fsdevel, Daniel Borkmann,
Jakub Kicinski, Masahiro Yamada, Gary Lin, Bruno Meneguele,
linux-security-module, Casey Schaufler
In-Reply-To: <a34cf18a-f251-f4f1-ed7c-fb5e100df91d@i-love.sakura.ne.jp>
On Thu, Jun 25, 2020 at 11:21 AM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
>
> On 2020/06/25 23:26, Stephen Smalley wrote:
> > On Thu, Jun 25, 2020 at 9:25 AM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> >>
> >> On Thu, Jun 25, 2020 at 08:56:10AM -0400, Stephen Smalley wrote:
> >>> No, because we cannot label the inode based on the program's purpose
> >>> and therefore cannot configure an automatic transition to a suitable
> >>> security context for the process, unlike call_usermodehelper().
> >>
> >> Why, what prevents this? Can you not just do that based on the "blob
> >> address" or signature of it or something like that? Right now you all
> >> do this based on inode of a random file on a disk, what's the difference
> >> between a random blob in memory?
> >
> > Given some kind of key to identify the blob and look up a suitable
> > context in policy, I think it would work. We just don't have that
> > with the current interface. With /bin/kmod and the like, we have a
> > security xattr assigned to the file when it was created that we can
> > use as the basis for determining the process security context.
>
> My understanding is that fork_usermode_blob() is intended to be able to run
> without filesystems so that usermode blobs can start even before global init
> program (pid=1) starts.
>
> But SELinux's policy is likely stored inside filesystems which would be
> accessible only after global init program (pid=1) started.
>
> Therefore, I wonder whether SELinux can look up a suitable context in policy
> even if "some kind of key to identify the blob" is provided.
> Also, since (at least some of) usermode blob processes started via
> fork_usermode_blob() will remain after SELinux loads policy from filesystems,
> I guess that we will need a method for moving already running usermode blob
> processes to appropriate security contexts.
>
> Is my understanding/concerns correct?
It isn't fundamentally different than the issue of program execution
from a filesystem prior to initial policy load, e.g. executing
programs from the initrd or executing init from the "real" root.
Absent a policy, the process will just remain in the initial
SID/context (kernel SID), which will later be mapped to a context when
policy is loaded. Typical init programs address this by either
re-exec'ing themselves after policy load or by dynamically switching
contexts via write to /proc/self/attr/current. The kernel doesn't try
to retroactively transition previously started processes; they are
expected to either exit prior to policy load (ala transient processes
run from initrd) or re-exec or set their contexts after policy load.
^ permalink raw reply
* Re: [RFC][PATCH] net/bpfilter: Remove this broken and apparently unmantained
From: Casey Schaufler @ 2020-06-25 16:06 UTC (permalink / raw)
To: Tetsuo Handa, Stephen Smalley, Greg Kroah-Hartman,
Alexei Starovoitov
Cc: Eric W. Biederman, Linus Torvalds, Kees Cook, Andrew Morton,
Alexei Starovoitov, David Miller, Al Viro, bpf, linux-fsdevel,
Daniel Borkmann, Jakub Kicinski, Masahiro Yamada, Gary Lin,
Bruno Meneguele, linux-security-module, Casey Schaufler
In-Reply-To: <a34cf18a-f251-f4f1-ed7c-fb5e100df91d@i-love.sakura.ne.jp>
On 6/25/2020 8:21 AM, Tetsuo Handa wrote:
> On 2020/06/25 23:26, Stephen Smalley wrote:
>> On Thu, Jun 25, 2020 at 9:25 AM Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>>> On Thu, Jun 25, 2020 at 08:56:10AM -0400, Stephen Smalley wrote:
>>>> No, because we cannot label the inode based on the program's purpose
>>>> and therefore cannot configure an automatic transition to a suitable
>>>> security context for the process, unlike call_usermodehelper().
>>> Why, what prevents this? Can you not just do that based on the "blob
>>> address" or signature of it or something like that? Right now you all
>>> do this based on inode of a random file on a disk, what's the difference
>>> between a random blob in memory?
>> Given some kind of key to identify the blob and look up a suitable
>> context in policy, I think it would work. We just don't have that
>> with the current interface. With /bin/kmod and the like, we have a
>> security xattr assigned to the file when it was created that we can
>> use as the basis for determining the process security context.
It should also be noted that Smack uses multiple xattrs. It is also
possible for multiple current security modules to use xattrs, including
capabilities. It's not sufficient to provide "the security xattr", there
would have to be provision for general security xattrs.
> My understanding is that fork_usermode_blob() is intended to be able to run
> without filesystems so that usermode blobs can start even before global init
> program (pid=1) starts.
>
> But SELinux's policy is likely stored inside filesystems which would be
> accessible only after global init program (pid=1) started.
>
> Therefore, I wonder whether SELinux can look up a suitable context in policy
> even if "some kind of key to identify the blob" is provided.
A security module has to have some sort of policy for whatever happens prior
to "policy load" as it is. That's not unique to this situation.
^ permalink raw reply
* Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver
From: Borislav Petkov @ 2020-06-25 17:23 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:
> 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.
>
> This commit implements a driver that provides an ioctl API to construct
> 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
> EINIT. ECREATE initializes SECS, EADD copies pages from system memory to
> the EPC and EINIT check a given signed measurement and moves the enclave
> 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(). 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)
>
> Cc: linux-security-module@vger.kernel.org
> Acked-by: Jethro Beekman <jethro@fortanix.com>
> Tested-by: Jethro Beekman <jethro@fortanix.com>
> Tested-by: Haitao Huang <haitao.huang@linux.intel.com>
> Tested-by: Chunyang Hui <sanqian.hcy@antfin.com>
> Tested-by: Jordan Hand <jorhand@linux.microsoft.com>
> Tested-by: Nathaniel McCallum <npmccallum@redhat.com>
> Tested-by: Seth Moore <sethmo@google.com>
> Co-developed-by: Sean Christopherson <sean.j.christopherson@intel.com>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> Co-developed-by: Suresh Siddha <suresh.b.siddha@intel.com>
> Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
> .../userspace-api/ioctl/ioctl-number.rst | 1 +
> arch/x86/include/uapi/asm/sgx.h | 66 ++
> arch/x86/kernel/cpu/sgx/Makefile | 3 +
> arch/x86/kernel/cpu/sgx/driver.c | 194 +++++
> arch/x86/kernel/cpu/sgx/driver.h | 30 +
> arch/x86/kernel/cpu/sgx/encl.c | 335 +++++++++
> arch/x86/kernel/cpu/sgx/encl.h | 87 +++
> arch/x86/kernel/cpu/sgx/ioctl.c | 706 ++++++++++++++++++
> arch/x86/kernel/cpu/sgx/main.c | 11 +
> 9 files changed, 1433 insertions(+)
> create mode 100644 arch/x86/include/uapi/asm/sgx.h
> create mode 100644 arch/x86/kernel/cpu/sgx/driver.c
> create mode 100644 arch/x86/kernel/cpu/sgx/driver.h
> create mode 100644 arch/x86/kernel/cpu/sgx/encl.c
> create mode 100644 arch/x86/kernel/cpu/sgx/encl.h
> create mode 100644 arch/x86/kernel/cpu/sgx/ioctl.c
>
> diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
> index 59472cd6a11d..35f713e3a267 100644
> --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> @@ -323,6 +323,7 @@ Code Seq# Include File Comments
> <mailto:tlewis@mindspring.com>
> 0xA3 90-9F linux/dtlk.h
> 0xA4 00-1F uapi/linux/tee.h Generic TEE subsystem
> +0xA4 00-1F uapi/asm/sgx.h Intel SGX subsystem (a legit conflict as TEE and SGX do not co-exist)
> 0xAA 00-3F linux/uapi/linux/userfaultfd.h
> 0xAB 00-1F linux/nbd.h
> 0xAC 00-1F linux/raw.h
> diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
> new file mode 100644
> index 000000000000..5edb08ab8fd0
> --- /dev/null
> +++ b/arch/x86/include/uapi/asm/sgx.h
> @@ -0,0 +1,66 @@
> +/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) WITH Linux-syscall-note */
Checkpatch complains here:
WARNING: 'SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) WITH Linux-syscall-note */' is not supported in LICENSES/...
#114: FILE: arch/x86/include/uapi/asm/sgx.h:1:
+/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) WITH Linux-syscall-note */
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...
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply
* Re: Enabling interrupts in QEMU TPM TIS
From: Jason Gunthorpe @ 2020-06-25 17:28 UTC (permalink / raw)
To: Stefan Berger
Cc: Jerry Snitselaar, linux-integrity, Jarkko Sakkinen, LSM List,
LKML
In-Reply-To: <1ca3a53d-2b83-7522-5ce1-83d9cc2f207d@linux.ibm.com>
On Thu, Jun 25, 2020 at 10:56:43AM -0400, Stefan Berger wrote:
> Hello!
>
> I want to enable IRQs now in QEMU's TPM TIS device model and I need to work
> with the following patch to Linux TIS. I am wondering whether the changes
> there look reasonable to you? Windows works with the QEMU modifications
> as-is, so maybe it's a bug in the TIS code (which I had not run into
> before).
>
>
> The point of the loop I need to introduce in the interrupt handler is that
> while the interrupt handler is running another interrupt may occur/be posted
> that then does NOT cause the interrupt handler to be invoked again but
> causes a stall, unless the loop is there.
That seems like a qemu bug, TPM interrupts are supposed to be level
interrupts, not edge.
Jason
^ permalink raw reply
* Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver
From: Sean Christopherson @ 2020-06-25 18:34 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: <20200625172319.GJ20319@zn.tnic>
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?
^ permalink raw reply
* Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver
From: Borislav Petkov @ 2020-06-25 18:45 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: <20200625183448.GG3437@linux.intel.com>
On Thu, Jun 25, 2020 at 11:34:48AM -0700, Sean Christopherson wrote:
> 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.
Yeah, I guess I can try reviewing it this way too and address each ioctl
separately. You can try splitting later so that we don't waste time now.
It would be good to have it split eventually, though, so that it is more
palatable for other rewiewers too...
> 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.
... provided the functionality is not too intertwined to make a split
actually worse than a single big patch.
Thx.
--
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-25 18:53 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:
> Subject: Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver
^
Add
> 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.
> 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)
Other than that, nice explanation. I like that in a commit message.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply
* Re: [PATCH 02/12] ima: Create a function to free a rule entry
From: Mimi Zohar @ 2020-06-25 19:33 UTC (permalink / raw)
To: Tyler Hicks, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
Prakhar Srivastava, linux-kernel, linux-integrity,
linux-security-module
In-Reply-To: <20200623003236.830149-3-tyhicks@linux.microsoft.com>
On Mon, 2020-06-22 at 19:32 -0500, Tyler Hicks wrote:
> There are several possible pieces of allocated memory in a rule entry.
> Create a function that can free all allocated memory for a given rule
> entry.
>
> This patch introduces no functional changes but sets the groundwork for
> some memory leak fixes.
>
> Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
Having a function to release all memory associated with a policy rule
in general is a good idea. However, in the case of the shallow copy,
we're not removing any IMA rules, just updating the LSM info.
There is an opportunity to transition from the builtin policy rules to
a custom IMA policy. Afterwards IMA policy rules may only be
appended.
An IMA custom policy based on LSM info may be loaded prior to the LSM
policy. These LSM based rules are inactive until the corresponding
LSM rule is loaded. In some environments, LSM policies are loaded and
removed frequently. The IMA rules themselves are not removed, just
the LSM info is updated to reflect the current LSM info.
> ---
> security/integrity/ima/ima_policy.c | 33 +++++++++++++++++++++++++++--
> 1 file changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 236a731492d1..1320333201c6 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -261,6 +261,27 @@ 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
> + */
> +
> + /*
> + * When freeing newly added ima_rule_entry members, consider if you
> + * need to disown any references after the shallow copy in
> + * ima_lsm_copy_rule()
> + */
> + kfree(entry->fsname);
> + kfree(entry->keyrings);
> + ima_lsm_free_rule(entry);
> kfree(entry);
> }
>
> @@ -298,10 +319,18 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
> pr_warn("rule for LSM \'%s\' is undefined\n",
> (char *)entry->lsm[i].args_p);
> }
> +
> + /* Disown all references that were shallow copied */
> + entry->fsname = NULL;
> + entry->keyrings = NULL;
> + entry->template = NULL;
> return nentry;
>
> out_err:
> - ima_lsm_free_rule(nentry);
> + nentry->fsname = NULL;
> + nentry->keyrings = NULL;
> + nentry->template = NULL;
> + ima_free_rule(nentry);
> return NULL;
> }
>
> @@ -315,7 +344,7 @@ static int ima_lsm_update_rule(struct ima_rule_entry *entry)
>
> list_replace_rcu(&entry->list, &nentry->list);
> synchronize_rcu();
> - ima_lsm_free_rule(entry);
> + ima_free_rule(entry);
This should only update the LSM info, nothing else.
>
> return 0;
> }
^ permalink raw reply
* Re: [RFC][PATCH] net/bpfilter: Remove this broken and apparently unmantained
From: David Miller @ 2020-06-25 19:34 UTC (permalink / raw)
To: greg
Cc: penguin-kernel, alexei.starovoitov, ebiederm, torvalds, keescook,
akpm, ast, viro, bpf, linux-fsdevel, daniel, kuba,
yamada.masahiro, GLin, bmeneg, linux-security-module, casey
In-Reply-To: <20200625120725.GA3493334@kroah.com>
From: Greg KH <greg@kroah.com>
Date: Thu, 25 Jun 2020 14:07:25 +0200
> I really don't understand the objection here, why is this any different
> than any other random kernel driver for what it can do?
It's kernel code executing in userspace. If you don't trust the
signed code you don't trust the signed code.
Nothing is magic about a piece of code executing in userspace.
I seriously think this dicussion is trying to create an issue
that simply doesn't exist in reality.
If some kernel module executed "/bin/sh" it's the same problem.
There is no way to argue around this, so please stop doing so
it's silly.
^ permalink raw reply
* Re: [PATCH 01/12] ima: Have the LSM free its audit rule
From: Mimi Zohar @ 2020-06-25 19:41 UTC (permalink / raw)
To: Tyler Hicks, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
Prakhar Srivastava, linux-kernel, linux-integrity,
linux-security-module, Janne Karhunen
In-Reply-To: <20200623003236.830149-2-tyhicks@linux.microsoft.com>
On Mon, 2020-06-22 at 19:32 -0500, Tyler Hicks wrote:
> 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>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
^ permalink raw reply
* Re: [PATCH 10/12] ima: Move validation of the keyrings conditional into ima_validate_rule()
From: Tyler Hicks @ 2020-06-25 19:50 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: <20200623003236.830149-11-tyhicks@linux.microsoft.com>
On 2020-06-22 19:32:34, Tyler Hicks wrote:
> 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>
> ---
> security/integrity/ima/ima_policy.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 514baf24d6a5..ae2ec2a9cdb9 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -999,6 +999,12 @@ 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))
I accidentally left these out:
(IMA_DIGSIG_REQUIRED | IMA_PERMIT_DIRECTIO | IMA_MODSIG_ALLOWED | IMA_CHECK_BLACKLIST)
I'll add them in v2.
Tyler
> + return false;
> +
> break;
> case KEXEC_CMDLINE:
> if (entry->action & ~(MEASURE | DONT_MEASURE))
> @@ -1026,7 +1032,8 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
> default:
> return false;
> }
> - }
> + } else if (entry->flags & IMA_KEYRINGS)
> + return false;
>
> return true;
> }
> @@ -1208,7 +1215,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
* Re: [PATCH 02/12] ima: Create a function to free a rule entry
From: Tyler Hicks @ 2020-06-25 19:56 UTC (permalink / raw)
To: Mimi Zohar
Cc: Dmitry Kasatkin, James Morris, Serge E . Hallyn,
Lakshmi Ramasubramanian, Prakhar Srivastava, linux-kernel,
linux-integrity, linux-security-module
In-Reply-To: <1593113613.27152.345.camel@linux.ibm.com>
On 2020-06-25 15:33:33, Mimi Zohar wrote:
> On Mon, 2020-06-22 at 19:32 -0500, Tyler Hicks wrote:
> > There are several possible pieces of allocated memory in a rule entry.
> > Create a function that can free all allocated memory for a given rule
> > entry.
> >
> > This patch introduces no functional changes but sets the groundwork for
> > some memory leak fixes.
> >
> > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
>
> Having a function to release all memory associated with a policy rule
> in general is a good idea. However, in the case of the shallow copy,
> we're not removing any IMA rules, just updating the LSM info.
>
> There is an opportunity to transition from the builtin policy rules to
> a custom IMA policy. Afterwards IMA policy rules may only be
> appended.
>
> An IMA custom policy based on LSM info may be loaded prior to the LSM
> policy. These LSM based rules are inactive until the corresponding
> LSM rule is loaded. In some environments, LSM policies are loaded and
> removed frequently. The IMA rules themselves are not removed, just
> the LSM info is updated to reflect the current LSM info.
>
> > ---
> > security/integrity/ima/ima_policy.c | 33 +++++++++++++++++++++++++++--
> > 1 file changed, 31 insertions(+), 2 deletions(-)
> >
> > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> > index 236a731492d1..1320333201c6 100644
> > --- a/security/integrity/ima/ima_policy.c
> > +++ b/security/integrity/ima/ima_policy.c
> > @@ -261,6 +261,27 @@ 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
> > + */
> > +
> > + /*
> > + * When freeing newly added ima_rule_entry members, consider if you
> > + * need to disown any references after the shallow copy in
> > + * ima_lsm_copy_rule()
> > + */
> > + kfree(entry->fsname);
> > + kfree(entry->keyrings);
> > + ima_lsm_free_rule(entry);
> > kfree(entry);
> > }
> >
> > @@ -298,10 +319,18 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
> > pr_warn("rule for LSM \'%s\' is undefined\n",
> > (char *)entry->lsm[i].args_p);
> > }
> > +
> > + /* Disown all references that were shallow copied */
> > + entry->fsname = NULL;
> > + entry->keyrings = NULL;
> > + entry->template = NULL;
> > return nentry;
> >
> > out_err:
> > - ima_lsm_free_rule(nentry);
> > + nentry->fsname = NULL;
> > + nentry->keyrings = NULL;
> > + nentry->template = NULL;
> > + ima_free_rule(nentry);
>
> > return NULL;
> > }
> >
> > @@ -315,7 +344,7 @@ static int ima_lsm_update_rule(struct ima_rule_entry *entry)
> >
> > list_replace_rcu(&entry->list, &nentry->list);
> > synchronize_rcu();
> > - ima_lsm_free_rule(entry);
> > + ima_free_rule(entry);
>
> This should only update the LSM info, nothing else.
That's effectively what's happening since the fsname, keyrings, and
template pointers are being set to NULL, before exiting
ima_lsm_copy_rule(), in the ima_rule_entry that's going to be freed.
This patch is only introducing the function which can free all memory
associated with a rule and is starting to use it in place that a rule
entry is freed.
Would you rather me introduce ima_free_rule() for the upcoming memory
leak fixes in the series but not make use of it in
ima_lsm_update_rule()?
Tyler
>
> >
> > return 0;
> > }
^ permalink raw reply
* Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver
From: Jarkko Sakkinen @ 2020-06-25 20:21 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: <20200625172319.GJ20319@zn.tnic>
On Thu, Jun 25, 2020 at 07:23:19PM +0200, Borislav Petkov wrote:
> On Thu, Jun 18, 2020 at 01:08:33AM +0300, Jarkko Sakkinen wrote:
> > 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.
> >
> > This commit implements a driver that provides an ioctl API to construct
> > 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
> > EINIT. ECREATE initializes SECS, EADD copies pages from system memory to
> > the EPC and EINIT check a given signed measurement and moves the enclave
> > 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(). 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)
> >
> > Cc: linux-security-module@vger.kernel.org
> > Acked-by: Jethro Beekman <jethro@fortanix.com>
> > Tested-by: Jethro Beekman <jethro@fortanix.com>
> > Tested-by: Haitao Huang <haitao.huang@linux.intel.com>
> > Tested-by: Chunyang Hui <sanqian.hcy@antfin.com>
> > Tested-by: Jordan Hand <jorhand@linux.microsoft.com>
> > Tested-by: Nathaniel McCallum <npmccallum@redhat.com>
> > Tested-by: Seth Moore <sethmo@google.com>
> > Co-developed-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > Co-developed-by: Suresh Siddha <suresh.b.siddha@intel.com>
> > Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > ---
> > .../userspace-api/ioctl/ioctl-number.rst | 1 +
> > arch/x86/include/uapi/asm/sgx.h | 66 ++
> > arch/x86/kernel/cpu/sgx/Makefile | 3 +
> > arch/x86/kernel/cpu/sgx/driver.c | 194 +++++
> > arch/x86/kernel/cpu/sgx/driver.h | 30 +
> > arch/x86/kernel/cpu/sgx/encl.c | 335 +++++++++
> > arch/x86/kernel/cpu/sgx/encl.h | 87 +++
> > arch/x86/kernel/cpu/sgx/ioctl.c | 706 ++++++++++++++++++
> > arch/x86/kernel/cpu/sgx/main.c | 11 +
> > 9 files changed, 1433 insertions(+)
> > create mode 100644 arch/x86/include/uapi/asm/sgx.h
> > create mode 100644 arch/x86/kernel/cpu/sgx/driver.c
> > create mode 100644 arch/x86/kernel/cpu/sgx/driver.h
> > create mode 100644 arch/x86/kernel/cpu/sgx/encl.c
> > create mode 100644 arch/x86/kernel/cpu/sgx/encl.h
> > create mode 100644 arch/x86/kernel/cpu/sgx/ioctl.c
> >
> > diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
> > index 59472cd6a11d..35f713e3a267 100644
> > --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> > +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> > @@ -323,6 +323,7 @@ Code Seq# Include File Comments
> > <mailto:tlewis@mindspring.com>
> > 0xA3 90-9F linux/dtlk.h
> > 0xA4 00-1F uapi/linux/tee.h Generic TEE subsystem
> > +0xA4 00-1F uapi/asm/sgx.h Intel SGX subsystem (a legit conflict as TEE and SGX do not co-exist)
> > 0xAA 00-3F linux/uapi/linux/userfaultfd.h
> > 0xAB 00-1F linux/nbd.h
> > 0xAC 00-1F linux/raw.h
> > diff --git a/arch/x86/include/uapi/asm/sgx.h b/arch/x86/include/uapi/asm/sgx.h
> > new file mode 100644
> > index 000000000000..5edb08ab8fd0
> > --- /dev/null
> > +++ b/arch/x86/include/uapi/asm/sgx.h
> > @@ -0,0 +1,66 @@
> > +/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) WITH Linux-syscall-note */
>
> Checkpatch complains here:
>
> WARNING: 'SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) WITH Linux-syscall-note */' is not supported in LICENSES/...
> #114: FILE: arch/x86/include/uapi/asm/sgx.h:1:
> +/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) WITH Linux-syscall-note */
>
> 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...
It's still kind a strongly connected set of functionalities, but I get
your point.
I'd consider splitting for a slighly different angle:
1. Commit for the base driver.
2. Commit for each ioctl, adding the necessary "framework" to get that
piece of functionality completed. The order would be:
A. Create
B. Add
C. Initialize
Would be probably easier to review also this way because the commit kind
of rationalizes why things exist.
What do you think?
/Jarkko
/Jarkko
/Jarkko
^ permalink raw reply
* Re: [PATCH v33 11/21] x86/sgx: Linux Enclave Driver
From: Borislav Petkov @ 2020-06-25 20:25 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: <20200625202148.GB15394@linux.intel.com>
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.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply
* Re: [PATCH 02/12] ima: Create a function to free a rule entry
From: Mimi Zohar @ 2020-06-25 20:32 UTC (permalink / raw)
To: Tyler Hicks
Cc: Dmitry Kasatkin, James Morris, Serge E . Hallyn,
Lakshmi Ramasubramanian, Prakhar Srivastava, linux-kernel,
linux-integrity, linux-security-module
In-Reply-To: <20200625195647.GB4694@sequoia>
On Thu, 2020-06-25 at 14:56 -0500, Tyler Hicks wrote:
> On 2020-06-25 15:33:33, Mimi Zohar wrote:
> > On Mon, 2020-06-22 at 19:32 -0500, Tyler Hicks wrote:
> > > There are several possible pieces of allocated memory in a rule entry.
> > > Create a function that can free all allocated memory for a given rule
> > > entry.
> > >
> > > This patch introduces no functional changes but sets the groundwork for
> > > some memory leak fixes.
> > >
> > > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> >
> > Having a function to release all memory associated with a policy rule
> > in general is a good idea. However, in the case of the shallow copy,
> > we're not removing any IMA rules, just updating the LSM info.
> >
> > There is an opportunity to transition from the builtin policy rules to
> > a custom IMA policy. Afterwards IMA policy rules may only be
> > appended.
> >
> > An IMA custom policy based on LSM info may be loaded prior to the LSM
> > policy. These LSM based rules are inactive until the corresponding
> > LSM rule is loaded. In some environments, LSM policies are loaded and
> > removed frequently. The IMA rules themselves are not removed, just
> > the LSM info is updated to reflect the current LSM info.
> >
> > > ---
> > > security/integrity/ima/ima_policy.c | 33 +++++++++++++++++++++++++++--
> > > 1 file changed, 31 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> > > index 236a731492d1..1320333201c6 100644
> > > --- a/security/integrity/ima/ima_policy.c
> > > +++ b/security/integrity/ima/ima_policy.c
> > > @@ -261,6 +261,27 @@ 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
> > > + */
> > > +
> > > + /*
> > > + * When freeing newly added ima_rule_entry members, consider if you
> > > + * need to disown any references after the shallow copy in
> > > + * ima_lsm_copy_rule()
> > > + */
> > > + kfree(entry->fsname);
> > > + kfree(entry->keyrings);
> > > + ima_lsm_free_rule(entry);
> > > kfree(entry);
> > > }
> > >
> > > @@ -298,10 +319,18 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
> > > pr_warn("rule for LSM \'%s\' is undefined\n",
> > > (char *)entry->lsm[i].args_p);
> > > }
> > > +
> > > + /* Disown all references that were shallow copied */
> > > + entry->fsname = NULL;
> > > + entry->keyrings = NULL;
> > > + entry->template = NULL;
> > > return nentry;
> > >
> > > out_err:
> > > - ima_lsm_free_rule(nentry);
> > > + nentry->fsname = NULL;
> > > + nentry->keyrings = NULL;
> > > + nentry->template = NULL;
> > > + ima_free_rule(nentry);
> >
> > > return NULL;
> > > }
> > >
> > > @@ -315,7 +344,7 @@ static int ima_lsm_update_rule(struct ima_rule_entry *entry)
> > >
> > > list_replace_rcu(&entry->list, &nentry->list);
> > > synchronize_rcu();
> > > - ima_lsm_free_rule(entry);
> > > + ima_free_rule(entry);
> >
> > This should only update the LSM info, nothing else.
>
> That's effectively what's happening since the fsname, keyrings, and
> template pointers are being set to NULL, before exiting
> ima_lsm_copy_rule(), in the ima_rule_entry that's going to be freed.
Ah, that clarified the reason for setting fsname, keyrings, ... to
null before calling ima_free_rule.
>
> This patch is only introducing the function which can free all memory
> associated with a rule and is starting to use it in place that a rule
> entry is freed.
>
> Would you rather me introduce ima_free_rule() for the upcoming memory
> leak fixes in the series but not make use of it in
> ima_lsm_update_rule()?
You could add a comment explaining the NULLs, but it might be clearer
to keep the direct call to ima_lsm_free_rule().
Mimi
^ permalink raw reply
* Re: [PATCH 10/12] ima: Move validation of the keyrings conditional into ima_validate_rule()
From: Mimi Zohar @ 2020-06-25 20:46 UTC (permalink / raw)
To: Tyler Hicks, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
Prakhar Srivastava, linux-kernel, linux-integrity,
linux-security-module
In-Reply-To: <20200625195017.GA4694@sequoia>
> > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> > index 514baf24d6a5..ae2ec2a9cdb9 100644
> > --- a/security/integrity/ima/ima_policy.c
> > +++ b/security/integrity/ima/ima_policy.c
> > @@ -999,6 +999,12 @@ 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))
>
> I accidentally left these out:
>
> (IMA_DIGSIG_REQUIRED | IMA_PERMIT_DIRECTIO | IMA_MODSIG_ALLOWED | IMA_CHECK_BLACKLIST)
>
> I'll add them in v2.
Thanks, I noticed when skimming the patches the first time around.
Mimi
^ permalink raw reply
* Re: [PATCH 03/12] ima: Free the entire rule when deleting a list of rules
From: Mimi Zohar @ 2020-06-25 21:05 UTC (permalink / raw)
To: Tyler Hicks, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
Prakhar Srivastava, linux-kernel, linux-integrity,
linux-security-module
In-Reply-To: <20200623003236.830149-4-tyhicks@linux.microsoft.com>
On Mon, 2020-06-22 at 19:32 -0500, Tyler Hicks wrote:
> Use ima_free_rule() to fix memory leaks of allocated ima_rule_entry
> members, such as .fsname and .keyrings, when deleting a list of rules.
>
> This fixes a memory leak seen when loading by a valid rule that contains
> an additional piece of allocated memory, such as an fsname, followed by
> an invalid rule that triggers a policy load failure:
>
> # echo -e "dont_measure fsname=securityfs\nbad syntax" > \
> /sys/kernel/security/ima/policy
> -bash: echo: write error: Invalid argument
> # echo scan > /sys/kernel/debug/kmemleak
> # cat /sys/kernel/debug/kmemleak
> unreferenced object 0xffff9bab67ca12c0 (size 16):
> comm "tee", pid 684, jiffies 4295212803 (age 252.344s)
> hex dump (first 16 bytes):
> 73 65 63 75 72 69 74 79 66 73 00 6b 6b 6b 6b a5 securityfs.kkkk.
> backtrace:
> [<00000000adc80b1b>] kstrdup+0x2e/0x60
> [<00000000d504cb0d>] ima_parse_add_rule+0x7d4/0x1020
> [<00000000444825ac>] ima_write_policy+0xab/0x1d0
> [<000000002b7f0d6c>] vfs_write+0xde/0x1d0
> [<0000000096feedcf>] ksys_write+0x68/0xe0
> [<0000000052b544a2>] do_syscall_64+0x56/0xa0
> [<000000007ead1ba7>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> Fixes: f1b08bbcbdaf ("ima: define a new policy condition based on the filesystem name")
> Fixes: 2b60c0ecedf8 ("IMA: Read keyrings= option from the IMA policy")
> Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
Thanks! Thinking about it some more. It makes more sense to define
ima_free_rule() here in this patch.
Mimi
> ---
> security/integrity/ima/ima_policy.c | 6 +-----
> 1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 1320333201c6..94ca3b8abb69 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -1431,15 +1431,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);
> }
> }
>
^ permalink raw reply
* Re: [PATCH 03/12] ima: Free the entire rule when deleting a list of rules
From: Mimi Zohar @ 2020-06-25 21:07 UTC (permalink / raw)
To: Tyler Hicks, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
Prakhar Srivastava, linux-kernel, linux-integrity,
linux-security-module
In-Reply-To: <20200623003236.830149-4-tyhicks@linux.microsoft.com>
On Mon, 2020-06-22 at 19:32 -0500, Tyler Hicks wrote:
> Use ima_free_rule() to fix memory leaks of allocated ima_rule_entry
> members, such as .fsname and .keyrings, when deleting a list of rules.
>
> This fixes a memory leak seen when loading by a valid rule that contains
> an additional piece of allocated memory, such as an fsname, followed by
> an invalid rule that triggers a policy load failure:
>
> # echo -e "dont_measure fsname=securityfs\nbad syntax" > \
> /sys/kernel/security/ima/policy
> -bash: echo: write error: Invalid argument
> # echo scan > /sys/kernel/debug/kmemleak
> # cat /sys/kernel/debug/kmemleak
> unreferenced object 0xffff9bab67ca12c0 (size 16):
> comm "tee", pid 684, jiffies 4295212803 (age 252.344s)
> hex dump (first 16 bytes):
> 73 65 63 75 72 69 74 79 66 73 00 6b 6b 6b 6b a5 securityfs.kkkk.
> backtrace:
> [<00000000adc80b1b>] kstrdup+0x2e/0x60
> [<00000000d504cb0d>] ima_parse_add_rule+0x7d4/0x1020
> [<00000000444825ac>] ima_write_policy+0xab/0x1d0
> [<000000002b7f0d6c>] vfs_write+0xde/0x1d0
> [<0000000096feedcf>] ksys_write+0x68/0xe0
> [<0000000052b544a2>] do_syscall_64+0x56/0xa0
> [<000000007ead1ba7>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> Fixes: f1b08bbcbdaf ("ima: define a new policy condition based on the filesystem name")
> Fixes: 2b60c0ecedf8 ("IMA: Read keyrings= option from the IMA policy")
> Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
Your decision, but you might consider squashing this patch with 3/12.
Everything all together in one patch.
Mimi
> ---
> security/integrity/ima/ima_policy.c | 6 +-----
> 1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 1320333201c6..94ca3b8abb69 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -1431,15 +1431,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);
> }
> }
>
^ permalink raw reply
* Re: [PATCH 03/12] ima: Free the entire rule when deleting a list of rules
From: Mimi Zohar @ 2020-06-25 21:08 UTC (permalink / raw)
To: Tyler Hicks, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
Prakhar Srivastava, linux-kernel, linux-integrity,
linux-security-module
In-Reply-To: <1593119238.27152.395.camel@linux.ibm.com>
On Thu, 2020-06-25 at 17:07 -0400, Mimi Zohar wrote:
> On Mon, 2020-06-22 at 19:32 -0500, Tyler Hicks wrote:
> > Use ima_free_rule() to fix memory leaks of allocated ima_rule_entry
> > members, such as .fsname and .keyrings, when deleting a list of rules.
> >
> > This fixes a memory leak seen when loading by a valid rule that contains
> > an additional piece of allocated memory, such as an fsname, followed by
> > an invalid rule that triggers a policy load failure:
> >
> > # echo -e "dont_measure fsname=securityfs\nbad syntax" > \
> > /sys/kernel/security/ima/policy
> > -bash: echo: write error: Invalid argument
> > # echo scan > /sys/kernel/debug/kmemleak
> > # cat /sys/kernel/debug/kmemleak
> > unreferenced object 0xffff9bab67ca12c0 (size 16):
> > comm "tee", pid 684, jiffies 4295212803 (age 252.344s)
> > hex dump (first 16 bytes):
> > 73 65 63 75 72 69 74 79 66 73 00 6b 6b 6b 6b a5 securityfs.kkkk.
> > backtrace:
> > [<00000000adc80b1b>] kstrdup+0x2e/0x60
> > [<00000000d504cb0d>] ima_parse_add_rule+0x7d4/0x1020
> > [<00000000444825ac>] ima_write_policy+0xab/0x1d0
> > [<000000002b7f0d6c>] vfs_write+0xde/0x1d0
> > [<0000000096feedcf>] ksys_write+0x68/0xe0
> > [<0000000052b544a2>] do_syscall_64+0x56/0xa0
> > [<000000007ead1ba7>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> >
> > Fixes: f1b08bbcbdaf ("ima: define a new policy condition based on the filesystem name")
> > Fixes: 2b60c0ecedf8 ("IMA: Read keyrings= option from the IMA policy")
> > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
>
> Your decision, but you might consider squashing this patch with 3/12.
> Everything all together in one patch.
Oops, that was the comment for 4/12.
Mimi
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox