* Re: [PATCH v2 1/5] fs: Add support for an O_MAYEXEC flag on sys_open()
From: Aleksa Sarai @ 2019-09-06 22:18 UTC (permalink / raw)
To: Jeff Layton
Cc: Mickaël Salaün, Florian Weimer,
Mickaël Salaün, linux-kernel, Alexei Starovoitov,
Al Viro, Andy Lutomirski, Christian Heimes, Daniel Borkmann,
Eric Chiang, James Morris, Jan Kara, Jann Horn, Jonathan Corbet,
Kees Cook, Matthew Garrett, Matthew Wilcox, Michael Kerrisk,
Mimi Zohar, Philippe Trébuchet, Scott Shell,
Sean Christopherson, Shuah Khan, Song Liu, Steve Dower,
Steve Grubb, Thibaut Sautereau, Vincent Strubel,
Yves-Alexis Perez, kernel-hardening, linux-api,
linux-security-module, linux-fsdevel
In-Reply-To: <20190906220546.gkqxzm4y3ynevvtz@yavin.dot.cyphar.com>
[-- Attachment #1: Type: text/plain, Size: 3465 bytes --]
On 2019-09-07, Aleksa Sarai <cyphar@cyphar.com> wrote:
> On 2019-09-06, Jeff Layton <jlayton@kernel.org> wrote:
> > On Sat, 2019-09-07 at 03:13 +1000, Aleksa Sarai wrote:
> > > On 2019-09-06, Jeff Layton <jlayton@kernel.org> wrote:
> > > > On Fri, 2019-09-06 at 18:06 +0200, Mickaël Salaün wrote:
> > > > > On 06/09/2019 17:56, Florian Weimer wrote:
> > > > > > Let's assume I want to add support for this to the glibc dynamic loader,
> > > > > > while still being able to run on older kernels.
> > > > > >
> > > > > > Is it safe to try the open call first, with O_MAYEXEC, and if that fails
> > > > > > with EINVAL, try again without O_MAYEXEC?
> > > > >
> > > > > The kernel ignore unknown open(2) flags, so yes, it is safe even for
> > > > > older kernel to use O_MAYEXEC.
> > > > >
> > > >
> > > > Well...maybe. What about existing programs that are sending down bogus
> > > > open flags? Once you turn this on, they may break...or provide a way to
> > > > circumvent the protections this gives.
> > >
> > > It should be noted that this has been a valid concern for every new O_*
> > > flag introduced (and yet we still introduced new flags, despite the
> > > concern) -- though to be fair, O_TMPFILE actually does have a
> > > work-around with the O_DIRECTORY mask setup.
> > >
> > > The openat2() set adds O_EMPTYPATH -- though in fairness it's also
> > > backwards compatible because empty path strings have always given ENOENT
> > > (or EINVAL?) while O_EMPTYPATH is a no-op non-empty strings.
> > >
> > > > Maybe this should be a new flag that is only usable in the new openat2()
> > > > syscall that's still under discussion? That syscall will enforce that
> > > > all flags are recognized. You presumably wouldn't need the sysctl if you
> > > > went that route too.
> > >
> > > I'm also interested in whether we could add an UPGRADE_NOEXEC flag to
> > > how->upgrade_mask for the openat2(2) patchset (I reserved a flag bit for
> > > it, since I'd heard about this work through the grape-vine).
> > >
> >
> > I rather like the idea of having openat2 fds be non-executable by
> > default, and having userland request it specifically via O_MAYEXEC (or
> > some similar openat2 flag) if it's needed. Then you could add an
> > UPGRADE_EXEC flag instead?
> >
> > That seems like something reasonable to do with a brand new API, and
> > might be very helpful for preventing certain classes of attacks.
>
> In that case, maybe openat2(2) should default to not allowing any
> upgrades by default? The reason I pitched UPGRADE_NOEXEC is because
> UPGRADE_NO{READ,WRITE} are the existing @how->upgrade_mask flags.
Sorry, another issue is that there isn't a current way to really
restrict fexecve() permissions (from my [limited] understanding,
__FMODE_EXEC isn't the right thing to use) -- so we can't blanket block
exec through openat2() O_PATH descriptors and add UPGRADE_EXEC later.
We would have to implement FMODE_EXEC (and FMODE_MAP_EXEC as you
suggested) in order to implement FMODE_UPGRADE_EXEC before we could even
get a first version of openat2(2) in. Though, I do (a little
begrudgingly) agree that we should have a safe default if possible
(magical O_PATH reopening trickery is something that most people don't
know about and probably wouldn't want to happen if they did).
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v2 0/5] Add support for O_MAYEXEC
From: Aleksa Sarai @ 2019-09-06 22:44 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Steve Grubb, Florian Weimer, Mickaël Salaün,
linux-kernel, Alexei Starovoitov, Al Viro, Andy Lutomirski,
Christian Heimes, Daniel Borkmann, Eric Chiang, James Morris,
Jan Kara, Jann Horn, Jonathan Corbet, Kees Cook, Matthew Garrett,
Matthew Wilcox, Michael Kerrisk, Mickaël Salaün,
Mimi Zohar, Philippe Trébuchet, Scott Shell,
Sean Christopherson, Shuah Khan, Song Liu, Steve Dower,
Thibaut S autereau, Vincent Strubel, Yves-Alexis Perez,
kernel-hardening, linux-api, linux-security-module, linux-fsdevel
In-Reply-To: <C95B704C-F84F-4341-BDE7-CD70C5DDBEEF@amacapital.net>
[-- Attachment #1: Type: text/plain, Size: 2905 bytes --]
On 2019-09-06, Andy Lutomirski <luto@amacapital.net> wrote:
> > On Sep 6, 2019, at 12:07 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> >
> >> On Friday, September 6, 2019 2:57:00 PM EDT Florian Weimer wrote:
> >> * Steve Grubb:
> >>> Now with LD_AUDIT
> >>> $ LD_AUDIT=/home/sgrubb/test/openflags/strip-flags.so.0 strace ./test
> >>> 2>&1 | grep passwd openat(3, "passwd", O_RDONLY) = 4
> >>>
> >>> No O_CLOEXEC flag.
> >>
> >> I think you need to explain in detail why you consider this a problem.
> >
> > Because you can strip the O_MAYEXEC flag from being passed into the kernel.
> > Once you do that, you defeat the security mechanism because it never gets
> > invoked. The issue is that the only thing that knows _why_ something is being
> > opened is user space. With this mechanism, you can attempt to pass this
> > reason to the kernel so that it may see if policy permits this. But you can
> > just remove the flag.
>
> I’m with Florian here. Once you are executing code in a process, you
> could just emulate some other unapproved code. This series is not
> intended to provide the kind of absolute protection you’re imagining.
I also agree, though I think that there is a separate argument to be
made that there are two possible problems with O_MAYEXEC (which might
not be really big concerns):
* It's very footgun-prone if you didn't call O_MAYEXEC yourself and
you pass the descriptor elsewhere. You need to check f_flags to see
if it contains O_MAYEXEC. Maybe there is an argument to be made that
passing O_MAYEXECs around isn't a valid use-case, but in that case
there should be some warnings about that.
* There's effectively a TOCTOU flaw (even if you are sure O_MAYEXEC is
in f_flags) -- if the filesystem becomes re-mounted noexec (or the
file has a-x permissions) after you've done the check you won't get
hit with an error when you go to use the file descriptor later.
To fix both you'd need to do what you mention later:
> What the kernel *could* do is prevent mmapping a non-FMODE_EXEC file
> with PROT_EXEC, which would indeed have a real effect (in an iOS-like
> world, for example) but would break many, many things.
And I think this would be useful (with the two possible ways of
executing .text split into FMODE_EXEC and FMODE_MAP_EXEC, as mentioned
in a sister subthread), but would have to be opt-in for the obvious
reason you outlined. However, we could make it the default for
openat2(2) -- assuming we can agree on what the semantics of a
theoretical FMODE_EXEC should be.
And of course we'd need to do FMODE_UPGRADE_EXEC (which would need to
also permit fexecve(2) though probably not PROT_EXEC -- I don't think
you can mmap() an O_PATH descriptor).
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v2] KEYS: trusted: correctly initialize digests and fix locking issue
From: Jarkko Sakkinen @ 2019-09-07 19:02 UTC (permalink / raw)
To: Roberto Sassu, zohar
Cc: linux-integrity, linux-security-module, keyrings, linux-kernel,
silviu.vlasceanu
In-Reply-To: <20190904185057.8400-1-roberto.sassu@huawei.com>
On Wed, 2019-09-04 at 20:50 +0200, Roberto Sassu wrote:
> This patch fixes two issues introduced with commit 0b6cf6b97b7e ("tpm: pass
> an array of tpm_extend_digest structures to tpm_pcr_extend()").
>
> It initializes the algorithm in init_digests() for trusted keys, and moves
> the algorithm check in tpm_pcr_extend() before locks are taken in
> tpm_find_get_ops().
>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> Fixes: 0b6cf6b97b7e ("tpm: pass an array of tpm_extend_digest structures to tpm_pcr_extend()")
> ---
The changelog is missing. You should place it right after these three
dashes before diffstat. So, why did you do v2?
I don't see any description of the two issues. The commit messages
goes on explaining right away what this patch does. Would be nice
to have one paragraph describing both of the issues at first before
striving into solutions.
Also, the granularity should be one patch per one issue so this will
require two patches in total.
/Jarkko
^ permalink raw reply
* Re: [PATCH v2] KEYS: trusted: correctly initialize digests and fix locking issue
From: Jarkko Sakkinen @ 2019-09-07 19:03 UTC (permalink / raw)
To: Roberto Sassu, zohar
Cc: linux-integrity, linux-security-module, keyrings, linux-kernel,
silviu.vlasceanu
In-Reply-To: <10abf6bea8b2612a40eae338e94704d152f53825.camel@linux.intel.com>
On Sat, 2019-09-07 at 22:02 +0300, Jarkko Sakkinen wrote:
> On Wed, 2019-09-04 at 20:50 +0200, Roberto Sassu wrote:
> > This patch fixes two issues introduced with commit 0b6cf6b97b7e ("tpm: pass
> > an array of tpm_extend_digest structures to tpm_pcr_extend()").
> >
> > It initializes the algorithm in init_digests() for trusted keys, and moves
> > the algorithm check in tpm_pcr_extend() before locks are taken in
> > tpm_find_get_ops().
> >
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > Fixes: 0b6cf6b97b7e ("tpm: pass an array of tpm_extend_digest structures to tpm_pcr_extend()")
> > ---
>
> The changelog is missing. You should place it right after these three
> dashes before diffstat. So, why did you do v2?
>
> I don't see any description of the two issues. The commit messages
> goes on explaining right away what this patch does. Would be nice
> to have one paragraph describing both of the issues at first before
> striving into solutions.
>
> Also, the granularity should be one patch per one issue so this will
> require two patches in total.
Actually taking my words back as far as the last paragraph goes. Since
the fixes tag is the same I'm cool with one patch as long as the commit
message describes better what you're doing and why.
/Jarkko
^ permalink raw reply
* Re: [PATCH] tpm_tis_core: Set TPM_CHIP_FLAG_IRQ before probing for interrupts
From: Jarkko Sakkinen @ 2019-09-07 21:21 UTC (permalink / raw)
To: Jan Lübbe, Stefan Berger
Cc: linux-security-module, linux-kernel, linux-integrity,
Stefan Berger, linux-stable
In-Reply-To: <b19179168421eb511856f0ec5fd328d97f06a68c.camel@pengutronix.de>
On Fri, 2019-09-06 at 14:37 +0200, Jan Lübbe wrote:
> This is due to the SPI accesses performed by tis_int_handler (which
> will sleep). Switching to devm_request_threaded_irq fixes this and
> leads to a successful IRQ probe.
Aah, right through tpm_tis_read32/write32(). This is definitely a new
regression. Thanks for reporting this! This was completely missed when
the support for other than TCG MMIO was implemented for the TIS driver.
This should have a patch of it own with your reported-by unless you
care to send bug fix for it.
> But: It seems that the IRQ is not acked correctly, as the interrupt
> line stays low. I suspect this is because the tpm_chip_stop from
> http://git.infradead.org/users/jjs/linux-tpmdd.git/commitdiff/9b558deab2c5d7dc23d5f7a4064892ede482ad32
> happens before the threaded handler runs. I'm currently unable to
> verify that though, as my build machine's disk just died. :/
I cannot recall all the nyances related to interrupt probing but with a
quick look I wonder why it does not utilize int_queue. Then
tpm_chip_stop() could be done synchronously.
> Regards,
> Jan
/Jarkko
^ permalink raw reply
* Re: [PATCH 2/3] ima: update the file measurement on truncate
From: Mimi Zohar @ 2019-09-08 15:38 UTC (permalink / raw)
To: Janne Karhunen, linux-integrity, linux-security-module, linux-mm,
viro
Cc: Konsta Karsisto
In-Reply-To: <20190902094540.12786-2-janne.karhunen@gmail.com>
On Mon, 2019-09-02 at 12:45 +0300, Janne Karhunen wrote:
> Let IMA know when a file is being opened with truncate
> or truncated directly.
>
> Depends on commit 72649b7862a7 ("ima: keep the integrity state of open files up to date")'
>
> Signed-off-by: Janne Karhunen <janne.karhunen@gmail.com>
> Signed-off-by: Konsta Karsisto <konsta.karsisto@gmail.com>
> ---
> fs/namei.c | 5 ++++-
> fs/open.c | 3 +++
> 2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/fs/namei.c b/fs/namei.c
> index 209c51a5226c..0994fe26bef1 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3418,8 +3418,11 @@ static int do_last(struct nameidata *nd,
> goto out;
> opened:
> error = ima_file_check(file, op->acc_mode);
> - if (!error && will_truncate)
> + if (!error && will_truncate) {
> error = handle_truncate(file);
> + if (!error)
> + ima_file_update(file);
Security and IMA hooks are normally named after the function. For
example, there's a security hook named security_path_truncate() in
handle_truncate(). The new hook after the truncate would either be
named security_post_path_truncate() or ima_post_path_truncate().
> + }
> out:
> if (unlikely(error > 0)) {
> WARN_ON(1);
> diff --git a/fs/open.c b/fs/open.c
> index a59abe3c669a..98c2d4629371 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -63,6 +63,9 @@ int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,
> /* Note any delegations or leases have already been broken: */
> ret = notify_change(dentry, &newattrs, NULL);
> inode_unlock(dentry->d_inode);
> +
> + if (filp)
> + ima_file_update(filp);
> return ret;
> }
>
do_truncate() is called from a number of places. Are you sure that
the call to IMA should be in all of them? security_path_truncate()
isn't in all of the callers.
Mimi
^ permalink raw reply
* Re: [PATCH 1/3] ima: keep the integrity state of open files up to date
From: Mimi Zohar @ 2019-09-08 16:35 UTC (permalink / raw)
To: Janne Karhunen, linux-integrity, linux-security-module, linux-mm,
viro
Cc: Konsta Karsisto
In-Reply-To: <20190902094540.12786-1-janne.karhunen@gmail.com>
On Mon, 2019-09-02 at 12:45 +0300, Janne Karhunen wrote:
> When a file is open for writing, kernel crash or power outage
> is guaranteed to corrupt the inode integrity state leading to
> file appraisal failure on the subsequent boot. Add some basic
> infrastructure to keep the integrity measurements up to date
> as the files are written to.
The term "measurement" refers to the file hash stored in the IMA
measurement list and used to extend the TPM. IMA-appraisal verifies
the file hash against a known good value stored as an extended
attribute(xattr). For immutable files, the known good value should be
a file signature. For mutable files, the known good value is a file
hash. The purpose of this patch set is to increase the frequency in
which the file hash, stored as an xattr, is updated.
Throughout this patch set the term "measurement" or "measure" is
inappropriately used.
>
> Core file operations (open, close, sync, msync, truncate) are
> now allowed to update the measurement immediately. In order
> to maintain sufficient write performance for writes, add a
> latency tunable delayed work workqueue for computing the
> measurements.
>
> Changelog v2:
> - Make write measurements optional
> - Add support for MMIO measurements
> - Handle all writes via page flush
> - Add work cancellation support, files are properly closed
> after last_writer checks out. This fixes a userspace break
> where the file was still open for writing after closing it.
> - Fix/unify IMA_UPDATE_XATTR usage
>
> Signed-off-by: Janne Karhunen <janne.karhunen@gmail.com>
> Signed-off-by: Konsta Karsisto <konsta.karsisto@gmail.com>
> ---
> include/linux/ima.h | 14 +++
> security/integrity/ima/Kconfig | 30 ++++++
> security/integrity/ima/ima.h | 13 +++
> security/integrity/ima/ima_appraise.c | 13 ++-
> security/integrity/ima/ima_main.c | 128 +++++++++++++++++++++++++-
> security/integrity/integrity.h | 18 ++++
> 6 files changed, 213 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index a20ad398d260..6736844e90d3 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -93,6 +93,20 @@ static inline void ima_post_path_mknod(struct dentry *dentry)
> static inline void ima_kexec_cmdline(const void *buf, int size) {}
> #endif /* CONFIG_IMA */
>
> +#if ((defined CONFIG_IMA) && defined(CONFIG_IMA_MEASURE_WRITES))
> +void ima_file_update(struct file *file);
> +void ima_file_delayed_update(struct file *file);
> +#else
> +static inline void ima_file_update(struct file *file)
> +{
> + return;
> +}
> +static inline void ima_file_delayed_update(struct file *file)
> +{
> + return;
> +}
> +#endif
> +
> #ifndef CONFIG_IMA_KEXEC
> struct kimage;
>
> diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> index 897bafc59a33..df1cf684a442 100644
> --- a/security/integrity/ima/Kconfig
> +++ b/security/integrity/ima/Kconfig
> @@ -310,3 +310,33 @@ config IMA_APPRAISE_SIGNED_INIT
> default n
> help
> This option requires user-space init to be signed.
> +
> +config IMA_MEASURE_WRITES
> + bool "Measure file writes (EXPERIMENTAL)"
"MEASURE" is the wrong term.
> + depends on IMA
Only IMA_APPRAISE updates the security.ima. This should be "depends
on IMA_APPRAISE".
> + depends on EVM
Anyone relying on file hashes to verify a file's integrity should
enable EVM, but as much as possible IMA and EVM are defined
independently of each other. Please remove this dependency.
> + default n
> + help
> + By default IMA measures files only when they close or sync.
By default IMA-appraisal updates the file hash, stored as an xattr,
...
> + Choose this option if you want the integrity measurements on
> + the disk to update when the files are still open for writing.
> +
> +config IMA_MEASUREMENT_LATENCY
> + int
> + depends on IMA
More specific dependency is required.
I'd like to see smaller patches that build upon each other. For
example, the initial design should be in the first patch. Performance
improvements, like the latency and latency_ceiling, could be
subsequent patches.
> + range 0 60000
> + default 50
> + help
> + This value defines the measurement interval when files are
> + being written. Value is in milliseconds.
> +
> +config IMA_MEASUREMENT_LATENCY_CEILING
> + int
> + depends on IMA
More specific dependency required.
> + range 100 60000
The "ceiling" needs to be greater than the "latency". If it isn't
possible to implement in the Kconfig, then at least check in the code.
> + default 5000
> + help
> + In order to maintain high write performance for large files,
> + IMA increases the measurement interval as the file size grows.
> + This value defines the ceiling for the measurement delay in
> + milliseconds.
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 19769bf5f6ab..195e67631f70 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -160,6 +160,19 @@ void ima_init_template_list(void);
> int __init ima_init_digests(void);
> int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
> void *lsm_data);
> +#if ((defined CONFIG_IMA) && defined(CONFIG_IMA_MEASURE_WRITES))
Both Kconfig options shouldn't be required. Use the more specific
one.
> +void ima_cancel_measurement(struct integrity_iint_cache *iint);
(The function needs to be renamed to something without the word
"measurement".)
Currently ima_cancel_measurement() is defined and called from
ima_main.c.
> +#else
> +static inline void ima_cancel_measurement(struct integrity_iint_cache *iint)
> +{
> + return;
> +}
> +static inline void ima_init_measurement(struct integrity_iint_cache *iint,
> + struct dentry *dentry)
> +{
> + return;
> +}
> +#endif
If the function definition and usage are in the same file, the
function should be defined as static. There shouldn't be a need for
these the function declarations or stub functions.
>
> /*
> * used to protect h_table and sha_table
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index 136ae4e0ee92..6c137fb5289b 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -78,6 +78,15 @@ static int ima_fix_xattr(struct dentry *dentry,
> return rc;
> }
>
> +#ifdef CONFIG_IMA_MEASURE_WRITES
ifdef's don't belong in C code. Refer to section 21 "Conditional
Compilation" in Documentation/process/coding-style.rst.
> +inline void ima_init_measurement(struct integrity_iint_cache *iint,
> + struct dentry *dentry)
(
(The function needs to be renamed to something without the word
"measurement".)
Writing xattrs requires the i_rwsem. Please add a comment indicating
that callers must take the i_rwsem.
> +{
> + if (test_bit(IMA_UPDATE_XATTR, &iint->atomic_flags))
> + ima_fix_xattr(dentry, iint);
> +}
> +#endif
> +
> /* Return specific func appraised cached result */
> enum integrity_status ima_get_cache_status(struct integrity_iint_cache *iint,
> enum ima_hooks func)
> @@ -341,8 +350,10 @@ int ima_appraise_measurement(enum ima_hooks func,
> iint->flags |= IMA_NEW_FILE;
> if ((iint->flags & IMA_NEW_FILE) &&
> (!(iint->flags & IMA_DIGSIG_REQUIRED) ||
> - (inode->i_size == 0)))
> + (inode->i_size == 0))) {
> + ima_init_measurement(iint, dentry);
Do we really need to write the file hashes for 0 length files?
> status = INTEGRITY_PASS;
> + }
> goto out;
> }
>
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 79c01516211b..46d28cdb6466 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -12,7 +12,7 @@
> *
> * File: ima_main.c
> * implements the IMA hooks: ima_bprm_check, ima_file_mmap,
> - * and ima_file_check.
> + * ima_file_delayed_update, ima_file_update and ima_file_check.
> */
>
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> @@ -26,6 +26,8 @@
> #include <linux/xattr.h>
> #include <linux/ima.h>
> #include <linux/iversion.h>
> +#include <linux/workqueue.h>
> +#include <linux/sizes.h>
> #include <linux/fs.h>
>
> #include "ima.h"
> @@ -42,6 +44,7 @@ static int hash_setup_done;
> static struct notifier_block ima_lsm_policy_notifier = {
> .notifier_call = ima_lsm_policy_change,
> };
> +static struct workqueue_struct *ima_update_wq;
All of the workqueue support should probably be defined in a separate
file, not here in ima_main.c.
>
> static int __init hash_setup(char *str)
> {
> @@ -151,6 +154,7 @@ static void ima_check_last_writer(struct integrity_iint_cache *iint,
>
> if (!(mode & FMODE_WRITE))
> return;
> + ima_cancel_measurement(iint);
>
> mutex_lock(&iint->mutex);
> if (atomic_read(&inode->i_writecount) == 1) {
> @@ -420,6 +424,117 @@ int ima_bprm_check(struct linux_binprm *bprm)
> MAY_EXEC, CREDS_CHECK);
> }
>
> +#ifdef CONFIG_IMA_MEASURE_WRITES
ifdef's don't belong in C code.
> +static unsigned long ima_inode_update_delay(struct inode *inode)
> +{
> + unsigned long blocks, msecs;
> +
> + blocks = i_size_read(inode) / SZ_1M + 1;
> + msecs = blocks * IMA_LATENCY_INCREMENT;
> + if (msecs > CONFIG_IMA_MEASUREMENT_LATENCY_CEILING)
> + msecs = CONFIG_IMA_MEASUREMENT_LATENCY_CEILING;
> +
> + return msecs;
> +}
> +
> +static void ima_delayed_update_handler(struct work_struct *work)
> +{
> + struct ima_work_entry *entry;
> +
> + entry = container_of(work, typeof(*entry), work.work);
> +
> + ima_file_update(entry->file);
> + entry->file = NULL;
> + entry->state = IMA_WORK_INACTIVE;
> +}
> +
> +void ima_cancel_measurement(struct integrity_iint_cache *iint)
> +{
> + if (iint->ima_work.state != IMA_WORK_ACTIVE)
> + return;
> +
> + cancel_delayed_work_sync(&iint->ima_work.work);
> + iint->ima_work.state = IMA_WORK_CANCELLED;
> +}
> +
> +/**
> + * ima_file_delayed_update
> + * @file: pointer to file structure being updated
> + */
> +void ima_file_delayed_update(struct file *file)
> +{
> + struct inode *inode = file_inode(file);
> + struct integrity_iint_cache *iint;
> + unsigned long msecs;
> + bool creq;
> +
> + if (!ima_policy_flag || !S_ISREG(inode->i_mode))
> + return;
> +
> + iint = integrity_iint_find(inode);
> + if (!iint)
> + return;
> +
> + if (!test_bit(IMA_UPDATE_XATTR, &iint->atomic_flags))
> + return;
> +
> + mutex_lock(&iint->mutex);
> + if (iint->ima_work.state == IMA_WORK_ACTIVE)
> + goto out;
> +
> + msecs = ima_inode_update_delay(inode);
> + iint->ima_work.file = file;
> + iint->ima_work.state = IMA_WORK_ACTIVE;
> + INIT_DELAYED_WORK(&iint->ima_work.work, ima_delayed_update_handler);
> +
> + creq = queue_delayed_work(ima_update_wq,
> + &iint->ima_work.work,
> + msecs_to_jiffies(msecs));
> + if (creq == false) {
> + iint->ima_work.file = NULL;
> + iint->ima_work.state = IMA_WORK_INACTIVE;
> + }
> +out:
> + mutex_unlock(&iint->mutex);
> +}
> +EXPORT_SYMBOL_GPL(ima_file_delayed_update);
> +
> +/**
> + * ima_file_update - update the file measurement
> + * @file: pointer to file structure being updated
> + */
> +void ima_file_update(struct file *file)
> +{
> + struct inode *inode = file_inode(file);
> + struct integrity_iint_cache *iint;
> + bool should_measure = true;
> + u64 i_version;
> +
> + if (!ima_policy_flag || !S_ISREG(inode->i_mode))
> + return;
> +
> + iint = integrity_iint_find(inode);
> + if (!iint)
> + return;
> +
> + if (!test_bit(IMA_UPDATE_XATTR, &iint->atomic_flags))
> + return;
> +
> + mutex_lock(&iint->mutex);
> + if (IS_I_VERSION(inode)) {
> + i_version = inode_query_iversion(inode);
> + if (i_version == iint->version)
> + should_measure = false;
> + }
> + if (should_measure) {
> + iint->flags &= ~IMA_COLLECTED;
> + ima_update_xattr(iint, file);
> + }
> + mutex_unlock(&iint->mutex);
> +}
> +EXPORT_SYMBOL_GPL(ima_file_update);
> +#endif /* CONFIG_IMA_MEASURE_WRITES */
> +
> /**
> * ima_path_check - based on policy, collect/store measurement.
> * @file: pointer to the file to be measured
> @@ -716,9 +831,18 @@ static int __init init_ima(void)
> if (error)
> pr_warn("Couldn't register LSM notifier, error %d\n", error);
>
> - if (!error)
> + if (!error) {
> ima_update_policy_flag();
>
> + ima_update_wq = alloc_workqueue("ima-update-wq",
> + WQ_MEM_RECLAIM |
> + WQ_CPU_INTENSIVE,
> + 0);
> + if (!ima_update_wq) {
> + pr_err("Failed to allocate write measurement workqueue\n");
> + error = -ENOMEM;
> + }
> + }
> return error;
> }
>
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index d9323d31a3a8..0f80c3d2e079 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -64,6 +64,11 @@
> #define IMA_DIGSIG 3
> #define IMA_MUST_MEASURE 4
>
> +/* delayed measurement job state */
> +#define IMA_WORK_INACTIVE 0
> +#define IMA_WORK_ACTIVE 1
> +#define IMA_WORK_CANCELLED 2
> +
> enum evm_ima_xattr_type {
> IMA_XATTR_DIGEST = 0x01,
> EVM_XATTR_HMAC,
> @@ -115,6 +120,18 @@ struct signature_v2_hdr {
> uint8_t sig[0]; /* signature payload */
> } __packed;
>
> +#if CONFIG_IMA_MEASUREMENT_LATENCY == 0
> +#define IMA_LATENCY_INCREMENT 100
> +#else
> +#define IMA_LATENCY_INCREMENT CONFIG_IMA_MEASUREMENT_LATENCY
> +#endif
> +
> +struct ima_work_entry {
> + struct delayed_work work;
> + struct file *file;
> + uint8_t state;
> +};
> +
Please add a comment indicating the type of work or maybe update the
struct name.
Mimi
> /* integrity data associated with an inode */
> struct integrity_iint_cache {
> struct rb_node rb_node; /* rooted in integrity_iint_tree */
> @@ -131,6 +148,7 @@ struct integrity_iint_cache {
> enum integrity_status ima_creds_status:4;
> enum integrity_status evm_status:4;
> struct ima_digest_data *ima_hash;
> + struct ima_work_entry ima_work;
> };
>
> /* rbtree tree calls to lookup, insert, delete
^ permalink raw reply
* Re: [PATCH 3/3] ima: update the file measurement on writes
From: Mimi Zohar @ 2019-09-08 17:07 UTC (permalink / raw)
To: Janne Karhunen, linux-integrity, linux-security-module, linux-mm,
viro
Cc: Konsta Karsisto
In-Reply-To: <20190902094540.12786-3-janne.karhunen@gmail.com>
On Mon, 2019-09-02 at 12:45 +0300, Janne Karhunen wrote:
> From: Konsta Karsisto <konsta.karsisto@gmail.com>
>
> Hook do_writepages() in order to do IMA measurement of an inode
> that is written out.
With this explanation I would expect to see a security/ima hook in
do_writepages(), nothing else. There's a lot more going on here than
that. The memory management maintainer(s) doesn't really care about
IMA. Please break this patch up so that it is easier to review and
upstream.
My comments on the first patch in this patch set (e.g. function names,
ifdefs in C code, workqueue belonging in a separate patch) are also
applicable to this patch.
>
> Depends on commit 72649b7862a7 ("ima: keep the integrity state of open files up to date")'
>
> Signed-off-by: Konsta Karsisto <konsta.karsisto@gmail.com>
> Signed-off-by: Janne Karhunen <janne.karhunen@gmail.com>
> ---
[snip]
> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> index 1804f64ff43c..d5041c625529 100644
> --- a/mm/page-writeback.c
> +++ b/mm/page-writeback.c
> @@ -38,6 +38,7 @@
> #include <linux/sched/rt.h>
> #include <linux/sched/signal.h>
> #include <linux/mm_inline.h>
> +#include <linux/ima.h>
> #include <trace/events/writeback.h>
>
> #include "internal.h"
> @@ -2347,6 +2348,12 @@ int do_writepages(struct address_space *mapping, struct writeback_control *wbc)
> cond_resched();
> congestion_wait(BLK_RW_ASYNC, HZ/50);
> }
> + if (ret == 0) {
> + if (wbc->sync_mode == WB_SYNC_ALL)
> + ima_inode_update(mapping->host);
> + else
> + ima_inode_delayed_update(mapping->host);
It's hard enough upstreaming a single security or IMA hook. There's
no need for two. security/IMA hooks are normally based on function
names (eg. ima_do_writepages). The sync_mode should be an argument.
> + }
> return ret;
> }
>
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 46d28cdb6466..affc74a07125 100644
> @@ -425,6 +430,42 @@ int ima_bprm_check(struct linux_binprm *bprm)
> }
>
> #ifdef CONFIG_IMA_MEASURE_WRITES
> +void ima_get_file(struct integrity_iint_cache *iint,
> + struct file *file)
> +{
> + struct ima_fl_entry *e;
> +
> + if (!iint || !file)
> + return;
> + if (!(file->f_mode & FMODE_WRITE) ||
> + !test_bit(IMA_UPDATE_XATTR, &iint->atomic_flags))
> + return;
> +
> + list_for_each_entry(e, &iint->file_list, list) {
> + if (e->file == file)
> + return;
> + }
> + e = kmalloc(sizeof(*e), GFP_KERNEL);
> + if (!e)
> + return;
> + e->file = file;
> + list_add(&e->list, &iint->file_list);
> +}
> +
> +void ima_put_file(struct integrity_iint_cache *iint,
> + struct file *file)
> +{
> + struct ima_fl_entry *e;
> +
> + list_for_each_entry(e, &iint->file_list, list) {
> + if (e->file == file) {
> + list_del(&e->list);
> + kfree(e);
> + break;
> + }
> + }
> +}
> +
Functions with "get" or "put" in their name increment/decrement a
reference count. No reference count is being updated here.
Mimi
^ permalink raw reply
* [PATCH v3] KEYS: trusted: correctly initialize digests and fix locking issue
From: Roberto Sassu @ 2019-09-08 17:45 UTC (permalink / raw)
To: jarkko.sakkinen, zohar
Cc: linux-integrity, linux-security-module, keyrings, linux-kernel,
silviu.vlasceanu, Roberto Sassu
Commit 0b6cf6b97b7e ("tpm: pass an array of tpm_extend_digest structures to
tpm_pcr_extend()") modifies tpm_pcr_extend() to accept a digest for each
PCR bank. After modification, tpm_pcr_extend() expects that digests are
passed in the same order as the algorithms set in chip->allocated_banks.
This patch fixes two issues introduced in the last iterations of the patch
set: missing initialization of the TPM algorithm ID in the tpm_digest
structures passed to tpm_pcr_extend() by the trusted key module, and
unreleased locks in the TPM driver due to returning from tpm_pcr_extend()
without calling tpm_put_ops(). To avoid the second issue, input check is
done before locks are taken.
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Fixes: 0b6cf6b97b7e ("tpm: pass an array of tpm_extend_digest structures to tpm_pcr_extend()")
---
Changelog
v2:
- provide explanation of the problem
v1:
- correct referenced commit
drivers/char/tpm/tpm-interface.c | 8 ++++----
security/keys/trusted.c | 5 +++++
2 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 1b4f95c13e00..1fffa91fc148 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -316,14 +316,14 @@ int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
int rc;
int i;
- chip = tpm_find_get_ops(chip);
- if (!chip)
- return -ENODEV;
-
for (i = 0; i < chip->nr_allocated_banks; i++)
if (digests[i].alg_id != chip->allocated_banks[i].alg_id)
return -EINVAL;
+ chip = tpm_find_get_ops(chip);
+ if (!chip)
+ return -ENODEV;
+
if (chip->flags & TPM_CHIP_FLAG_TPM2) {
rc = tpm2_pcr_extend(chip, pcr_idx, digests);
tpm_put_ops(chip);
diff --git a/security/keys/trusted.c b/security/keys/trusted.c
index ade699131065..1fbd77816610 100644
--- a/security/keys/trusted.c
+++ b/security/keys/trusted.c
@@ -1228,11 +1228,16 @@ static int __init trusted_shash_alloc(void)
static int __init init_digests(void)
{
+ int i;
+
digests = kcalloc(chip->nr_allocated_banks, sizeof(*digests),
GFP_KERNEL);
if (!digests)
return -ENOMEM;
+ for (i = 0; i < chip->nr_allocated_banks; i++)
+ digests[i].alg_id = chip->allocated_banks[i].alg_id;
+
return 0;
}
--
2.17.1
^ permalink raw reply related
* Re: [PATCH bpf-next v10 06/10] bpf,landlock: Add a new map type: inode
From: Mickaël Salaün @ 2019-09-08 22:09 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Mickaël Salaün, linux-kernel, Alexander Viro,
Alexei Starovoitov, Andrew Morton, Andy Lutomirski,
Arnaldo Carvalho de Melo, Casey Schaufler, Daniel Borkmann,
David Drysdale, David S . Miller, Eric W . Biederman,
James Morris, Jann Horn, John Johansen, Jonathan Corbet,
Kees Cook, Michael Kerrisk, Paul Moore, Sargun Dhillon,
Serge E . Hallyn, Shuah Khan, Stephen Smalley, Tejun Heo,
Tetsuo Handa, Thomas Graf, Tycho Andersen, Will Drewry,
kernel-hardening, linux-api, linux-fsdevel, linux-security-module,
netdev
In-Reply-To: <a870c2c9-d2f7-e0fa-c8cc-35dbf8b5b87d@ssi.gouv.fr>
On 31/07/2019 20:46, Mickaël Salaün wrote:
>
> On 27/07/2019 03:40, Alexei Starovoitov wrote:
>> On Sun, Jul 21, 2019 at 11:31:12PM +0200, Mickaël Salaün wrote:
>>> FIXME: 64-bits in the doc
>
> FYI, this FIXME was fixed, just not removed from this message. :)
>
>>>
>>> This new map store arbitrary values referenced by inode keys. The map
>>> can be updated from user space with file descriptor pointing to inodes
>>> tied to a file system. From an eBPF (Landlock) program point of view,
>>> such a map is read-only and can only be used to retrieved a value tied
>>> to a given inode. This is useful to recognize an inode tagged by user
>>> space, without access right to this inode (i.e. no need to have a write
>>> access to this inode).
>>>
>>> Add dedicated BPF functions to handle this type of map:
>>> * bpf_inode_htab_map_update_elem()
>>> * bpf_inode_htab_map_lookup_elem()
>>> * bpf_inode_htab_map_delete_elem()
>>>
>>> This new map require a dedicated helper inode_map_lookup_elem() because
>>> of the key which is a pointer to an opaque data (only provided by the
>>> kernel). This act like a (physical or cryptographic) key, which is why
>>> it is also not allowed to get the next key.
>>>
>>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>>
>> there are too many things to comment on.
>> Let's do this patch.
>>
>> imo inode_map concept is interesting, but see below...
>>
>>> +
>>> + /*
>>> + * Limit number of entries in an inode map to the maximum number of
>>> + * open files for the current process. The maximum number of file
>>> + * references (including all inode maps) for a process is then
>>> + * (RLIMIT_NOFILE - 1) * RLIMIT_NOFILE. If the process' RLIMIT_NOFILE
>>> + * is 0, then any entry update is forbidden.
>>> + *
>>> + * An eBPF program can inherit all the inode map FD. The worse case is
>>> + * to fill a bunch of arraymaps, create an eBPF program, close the
>>> + * inode map FDs, and start again. The maximum number of inode map
>>> + * entries can then be close to RLIMIT_NOFILE^3.
>>> + */
>>> + if (attr->max_entries > rlimit(RLIMIT_NOFILE))
>>> + return -EMFILE;
>>
>> rlimit is checked, but no fd are consumed.
>> Once created such inode map_fd can be passed to a different process.
>> map_fd can be pinned into bpffs.
>> etc.
>> what the value of the check?
>
> I was looking for the most meaningful limit for a process, and rlimit is
> the best I found. As the limit of open FD per processes, rlimit is not
> perfect, but I think the semantic is close here (e.g. a process can also
> pass FD through unix socket).
>
>>
>>> +
>>> + /* decorelate UAPI from kernel API */
>>> + attr->key_size = sizeof(struct inode *);
>>> +
>>> + return htab_map_alloc_check(attr);
>>> +}
>>> +
>>> +static void inode_htab_put_key(void *key)
>>> +{
>>> + struct inode **inode = key;
>>> +
>>> + if ((*inode)->i_state & I_FREEING)
>>> + return;
>>
>> checking the state without take a lock? isn't it racy?
>
> This should only trigger when called from security_inode_free(). I'll
> add a comment.
>
>>
>>> + iput(*inode);
>>> +}
>>> +
>>> +/* called from syscall or (never) from eBPF program */
>>> +static int map_get_next_no_key(struct bpf_map *map, void *key, void *next_key)
>>> +{
>>> + /* do not leak a file descriptor */
>>
>> what this comment suppose to mean?
>
> Because a key is a reference to an inode, a possible return value for
> this function could be a file descriptor pointing to this inode (the
> same way a file descriptor is use to add an element). For now, I don't
> want to implement a way for a process with such a map to extract such
> inode, which I compare to a possible leak (of information, not kernel
> memory nor object). This could be implemented in the future if there is
> value in it (and probably some additional safeguards), though.
>
>>
>>> + return -ENOTSUPP;
>>> +}
>>> +
>>> +/* must call iput(inode) after this call */
>>> +static struct inode *inode_from_fd(int ufd, bool check_access)
>>> +{
>>> + struct inode *ret;
>>> + struct fd f;
>>> + int deny;
>>> +
>>> + f = fdget(ufd);
>>> + if (unlikely(!f.file))
>>> + return ERR_PTR(-EBADF);
>>> + /* TODO?: add this check when called from an eBPF program too (already
>>> + * checked by the LSM parent hooks anyway) */
>>> + if (unlikely(IS_PRIVATE(file_inode(f.file)))) {
>>> + ret = ERR_PTR(-EINVAL);
>>> + goto put_fd;
>>> + }
>>> + /* check if the FD is tied to a mount point */
>>> + /* TODO?: add this check when called from an eBPF program too */
>>> + if (unlikely(f.file->f_path.mnt->mnt_flags & MNT_INTERNAL)) {
>>> + ret = ERR_PTR(-EINVAL);
>>> + goto put_fd;
>>> + }
>>
>> a bunch of TODOs do not inspire confidence.
>
> I think the current implement is good, but these TODOs are here to draw
> attention on particular points for which I would like external review
> and opinion (hence the "?").
>
>>
>>> + if (check_access) {
>>> + /*
>>> + * must be allowed to access attributes from this file to then
>>> + * be able to compare an inode to its map entry
>>> + */
>>> + deny = security_inode_getattr(&f.file->f_path);
>>> + if (deny) {
>>> + ret = ERR_PTR(deny);
>>> + goto put_fd;
>>> + }
>>> + }
>>> + ret = file_inode(f.file);
>>> + ihold(ret);
>>> +
>>> +put_fd:
>>> + fdput(f);
>>> + return ret;
>>> +}
>>> +
>>> +/*
>>> + * The key is a FD when called from a syscall, but an inode address when called
>>> + * from an eBPF program.
>>> + */
>>> +
>>> +/* called from syscall */
>>> +int bpf_inode_fd_htab_map_lookup_elem(struct bpf_map *map, int *key, void *value)
>>> +{
>>> + void *ptr;
>>> + struct inode *inode;
>>> + int ret;
>>> +
>>> + /* check inode access */
>>> + inode = inode_from_fd(*key, true);
>>> + if (IS_ERR(inode))
>>> + return PTR_ERR(inode);
>>> +
>>> + rcu_read_lock();
>>> + ptr = htab_map_lookup_elem(map, &inode);
>>> + iput(inode);
>>> + if (IS_ERR(ptr)) {
>>> + ret = PTR_ERR(ptr);
>>> + } else if (!ptr) {
>>> + ret = -ENOENT;
>>> + } else {
>>> + ret = 0;
>>> + copy_map_value(map, value, ptr);
>>> + }
>>> + rcu_read_unlock();
>>> + return ret;
>>> +}
>>> +
>>> +/* called from kernel */
>>
>> wrong comment?
>> kernel side cannot call it, right?
>
> This is called from bpf_inode_fd_htab_map_delete_elem() (code just
> beneath), and from
> kernel/bpf/syscall.c:bpf_inode_ptr_unlocked_htab_map_delet_elem() which
> can be called by security_inode_free() (hook_inode_free_security).
>
>>
>>> +int bpf_inode_ptr_locked_htab_map_delete_elem(struct bpf_map *map,
>>> + struct inode **key, bool remove_in_inode)
>>> +{
>>> + if (remove_in_inode)
>>> + landlock_inode_remove_map(*key, map);
>>> + return htab_map_delete_elem(map, key);
>>> +}
>>> +
>>> +/* called from syscall */
>>> +int bpf_inode_fd_htab_map_delete_elem(struct bpf_map *map, int *key)
>>> +{
>>> + struct inode *inode;
>>> + int ret;
>>> +
>>> + /* do not check inode access (similar to directory check) */
>>> + inode = inode_from_fd(*key, false);
>>> + if (IS_ERR(inode))
>>> + return PTR_ERR(inode);
>>> + ret = bpf_inode_ptr_locked_htab_map_delete_elem(map, &inode, true);
>>> + iput(inode);
>>> + return ret;
>>> +}
>>> +
>>> +/* called from syscall */
>>> +int bpf_inode_fd_htab_map_update_elem(struct bpf_map *map, int *key, void *value,
>>> + u64 map_flags)
>>> +{
>>> + struct inode *inode;
>>> + int ret;
>>> +
>>> + WARN_ON_ONCE(!rcu_read_lock_held());
>>> +
>>> + /* check inode access */
>>> + inode = inode_from_fd(*key, true);
>>> + if (IS_ERR(inode))
>>> + return PTR_ERR(inode);
>>> + ret = htab_map_update_elem(map, &inode, value, map_flags);
>>> + if (!ret)
>>> + ret = landlock_inode_add_map(inode, map);
>>> + iput(inode);
>>> + return ret;
>>> +}
>>> +
>>> +static void inode_htab_map_free(struct bpf_map *map)
>>> +{
>>> + struct bpf_htab *htab = container_of(map, struct bpf_htab, map);
>>> + struct hlist_nulls_node *n;
>>> + struct hlist_nulls_head *head;
>>> + struct htab_elem *l;
>>> + int i;
>>> +
>>> + for (i = 0; i < htab->n_buckets; i++) {
>>> + head = select_bucket(htab, i);
>>> + hlist_nulls_for_each_entry_safe(l, n, head, hash_node) {
>>> + landlock_inode_remove_map(*((struct inode **)l->key), map);
>>> + }
>>> + }
>>> + htab_map_free(map);
>>> +}
>>
>> user space can delete the map.
>> that will trigger inode_htab_map_free() which will call
>> landlock_inode_remove_map().
>> which will simply itereate the list and delete from the list.
>
> landlock_inode_remove_map() removes the reference to the map (being
> freed) from the inode (with an RCU lock).
>
>>
>> While in parallel inode can be destoyed and hook_inode_free_security()
>> will be called.
>> I think nothing that protects from this race.
>
> According to security_inode_free(), the inode is effectively freed after
> the RCU grace period. However, I forgot to call bpf_map_inc() in
> landlock_inode_add_map(), which would prevent the map to be freed
> outside of the security_inode_free(). I'll fix that.
>
>>
>>> +
>>> +/*
>>> + * We need a dedicated helper to deal with inode maps because the key is a
>>> + * pointer to an opaque data, only provided by the kernel. This really act
>>> + * like a (physical or cryptographic) key, which is why it is also not allowed
>>> + * to get the next key with map_get_next_key().
>>
>> inode pointer is like cryptographic key? :)
>
> I wanted to highlight the fact that, contrary to other map key types,
> the value of this one should not be readable, only usable. A "secret
> value" is more appropriate but still confusing. I'll rephrase that.
>
>>
>>> + */
>>> +BPF_CALL_2(bpf_inode_map_lookup_elem, struct bpf_map *, map, void *, key)
>>> +{
>>> + WARN_ON_ONCE(!rcu_read_lock_held());
>>> + return (unsigned long)htab_map_lookup_elem(map, &key);
>>> +}
>>> +
>>> +const struct bpf_func_proto bpf_inode_map_lookup_elem_proto = {
>>> + .func = bpf_inode_map_lookup_elem,
>>> + .gpl_only = false,
>>> + .pkt_access = true,
>>
>> pkt_access ? :)
>
> This slipped in with this rebase, I'll remove it. :)
>
>>
>>> + .ret_type = RET_PTR_TO_MAP_VALUE_OR_NULL,
>>> + .arg1_type = ARG_CONST_MAP_PTR,
>>> + .arg2_type = ARG_PTR_TO_INODE,
>>> +};
>>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>>> index b2a8cb14f28e..e46441c42b68 100644
>>> --- a/kernel/bpf/syscall.c
>>> +++ b/kernel/bpf/syscall.c
>>> @@ -801,6 +801,8 @@ static int map_lookup_elem(union bpf_attr *attr)
>>> } else if (map->map_type == BPF_MAP_TYPE_QUEUE ||
>>> map->map_type == BPF_MAP_TYPE_STACK) {
>>> err = map->ops->map_peek_elem(map, value);
>>> + } else if (map->map_type == BPF_MAP_TYPE_INODE) {
>>> + err = bpf_inode_fd_htab_map_lookup_elem(map, key, value);
>>> } else {
>>> rcu_read_lock();
>>> if (map->ops->map_lookup_elem_sys_only)
>>> @@ -951,6 +953,10 @@ static int map_update_elem(union bpf_attr *attr)
>>> } else if (map->map_type == BPF_MAP_TYPE_QUEUE ||
>>> map->map_type == BPF_MAP_TYPE_STACK) {
>>> err = map->ops->map_push_elem(map, value, attr->flags);
>>> + } else if (map->map_type == BPF_MAP_TYPE_INODE) {
>>> + rcu_read_lock();
>>> + err = bpf_inode_fd_htab_map_update_elem(map, key, value, attr->flags);
>>> + rcu_read_unlock();
>>> } else {
>>> rcu_read_lock();
>>> err = map->ops->map_update_elem(map, key, value, attr->flags);
>>> @@ -1006,7 +1012,10 @@ static int map_delete_elem(union bpf_attr *attr)
>>> preempt_disable();
>>> __this_cpu_inc(bpf_prog_active);
>>> rcu_read_lock();
>>> - err = map->ops->map_delete_elem(map, key);
>>> + if (map->map_type == BPF_MAP_TYPE_INODE)
>>> + err = bpf_inode_fd_htab_map_delete_elem(map, key);
>>> + else
>>> + err = map->ops->map_delete_elem(map, key);
>>> rcu_read_unlock();
>>> __this_cpu_dec(bpf_prog_active);
>>> preempt_enable();
>>> @@ -1018,6 +1027,22 @@ static int map_delete_elem(union bpf_attr *attr)
>>> return err;
>>> }
>>>
>>> +int bpf_inode_ptr_unlocked_htab_map_delete_elem(struct bpf_map *map,
>>> + struct inode **key, bool remove_in_inode)
>>> +{
>>> + int err;
>>> +
>>> + preempt_disable();
>>> + __this_cpu_inc(bpf_prog_active);
>>> + rcu_read_lock();
>>> + err = bpf_inode_ptr_locked_htab_map_delete_elem(map, key, remove_in_inode);
>>> + rcu_read_unlock();
>>> + __this_cpu_dec(bpf_prog_active);
>>> + preempt_enable();
>>> + maybe_wait_bpf_programs(map);
>>
>> if that function was actually doing synchronize_rcu() the consequences
>> would have been unpleasant. Fortunately it's a nop in this case.
>> Please read the code carefully before copy-paste.
>> Also what do you think the reason of bpf_prog_active above?
>> What is the reason of rcu_read_lock above?
>
> The RCU is used as for every map modifications (usually from userspace).
> I wasn't sure about the other protections so I kept the same (generic)
> checks as in map_delete_elem() (just above) because this function follow
> the same semantic. What can I safely remove?
>
>>
>> I think the patch set needs to shrink at least in half to be reviewable.
>> The way you tie seccomp and lsm is probably the biggest obstacle
>> than any of the bugs above.
>> Can you drop seccomp ? and do it as normal lsm ?
>
> The seccomp/enforcement part is needed to have a minimum viable product,
> i.e. a process able to sandbox itself. Are you suggesting to first merge
> a version when it is only possible to create inode maps but not use them
> in an useful way (i.e. for sandboxing)? I can do it if it's OK with you,
> and I hope it will not be a problem for the security folks if it can
> help to move forward.
I talked with Kees Cook and James Morris at LSS NA, and I think the
better strategy to shrink this patch series is to tackle a much less
complex problem at first. Instead on focusing right now on file system,
the next version of this patch series will focus on memory protection,
which is also something desired. I'll then iterate with file system
support (i.e. inode maps) and other use cases once the basics of
Landlock are upstream. For this next series, the majority of the code
will be on the LSM side, while the eBPF part will mainly consist to add
a new program type. Because bpf-next is moving rapidly, I think it still
make sense to base this work on this tree (instead of linux-security).
Regards,
Mickaël
^ permalink raw reply
* Re: [PATCH bpf-next v10 06/10] bpf,landlock: Add a new map type: inode
From: Al Viro @ 2019-09-08 22:19 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Alexei Starovoitov, Mickaël Salaün, linux-kernel,
Alexei Starovoitov, Andrew Morton, Andy Lutomirski,
Arnaldo Carvalho de Melo, Casey Schaufler, Daniel Borkmann,
David Drysdale, David S . Miller, Eric W . Biederman,
James Morris, Jann Horn, John Johansen, Jonathan Corbet,
Kees Cook, Michael Kerrisk, Paul Moore, Sargun Dhillon,
Serge E . Hallyn, Shuah Khan, Stephen Smalley, Tejun Heo,
Tetsuo Handa, Thomas Graf, Tycho Andersen, Will Drewry,
kernel-hardening, linux-api, linux-fsdevel, linux-security-module,
netdev
In-Reply-To: <894922a2-1150-366c-3f08-c8b759da0742@digikod.net>
On Mon, Sep 09, 2019 at 12:09:57AM +0200, Mickaël Salaün wrote:
> >>> + rcu_read_lock();
> >>> + ptr = htab_map_lookup_elem(map, &inode);
> >>> + iput(inode);
Wait a sec. You are doing _what_ under rcu_read_lock()?
> >>> + if (IS_ERR(ptr)) {
> >>> + ret = PTR_ERR(ptr);
> >>> + } else if (!ptr) {
> >>> + ret = -ENOENT;
> >>> + } else {
> >>> + ret = 0;
> >>> + copy_map_value(map, value, ptr);
> >>> + }
> >>> + rcu_read_unlock();
^ permalink raw reply
* Re: [PATCH v2 0/5] Add support for O_MAYEXEC
From: James Morris @ 2019-09-09 0:16 UTC (permalink / raw)
To: Mickaël Salaün
Cc: linux-kernel, Aleksa Sarai, Alexei Starovoitov, Al Viro,
Andy Lutomirski, Christian Heimes, Daniel Borkmann, Eric Chiang,
Florian Weimer, Jan Kara, Jann Horn, Jonathan Corbet, Kees Cook,
Matthew Garrett, Matthew Wilcox, Michael Kerrisk,
Mickaël Salaün, Mimi Zohar, Philippe Trébuchet,
Scott Shell, Sean Christopherson, Shuah Khan, Song Liu,
Steve Dower, Steve Grubb, Thibaut Sautereau, Vincent Strubel,
Yves-Alexis Perez, kernel-hardening, linux-api,
linux-security-module, linux-fsdevel, Mimi Zohar
In-Reply-To: <20190906152455.22757-1-mic@digikod.net>
[-- Attachment #1: Type: text/plain, Size: 1105 bytes --]
On Fri, 6 Sep 2019, Mickaël Salaün wrote:
> Furthermore, the security policy can also be delegated to an LSM, either
> a MAC system or an integrity system. For instance, the new kernel
> MAY_OPENEXEC flag closes a major IMA measurement/appraisal interpreter
> integrity gap by bringing the ability to check the use of scripts [2].
To clarify, scripts are already covered by IMA if they're executed
directly, and the gap is when invoking a script as a parameter to the
interpreter (and for any sourced files). In that case only the interpreter
is measured/appraised, unless there's a rule also covering the script
file(s).
See:
https://en.opensuse.org/SDB:Ima_evm#script-behaviour
In theory you could probably also close the gap by modifying the
interpreters to check for the execute bit on any file opened for
interpretation (as earlier suggested by Steve Grubb), and then you could
have IMA measure/appraise all files with +x. I suspect this could get
messy in terms of unwanted files being included, and the MAY_OPENEXEC flag
has cleaner semantics.
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH v2 0/5] Add support for O_MAYEXEC
From: Mickaël Salaün @ 2019-09-09 9:09 UTC (permalink / raw)
To: Aleksa Sarai, Andy Lutomirski
Cc: Steve Grubb, Florian Weimer, Mickaël Salaün,
linux-kernel, Alexei Starovoitov, Al Viro, Andy Lutomirski,
Christian Heimes, Daniel Borkmann, Eric Chiang, James Morris,
Jan Kara, Jann Horn, Jonathan Corbet, Kees Cook, Matthew Garrett,
Matthew Wilcox, Michael Kerrisk, Mimi Zohar,
Philippe Trébuchet, Scott Shell, Sean Christopherson,
Shuah Khan, Song Liu, Steve Dower, Thibaut S autereau,
Vincent Strubel, Yves-Alexis Perez, kernel-hardening, linux-api,
linux-security-module, linux-fsdevel
In-Reply-To: <20190906224410.lffd6l5lnm4z3hht@yavin.dot.cyphar.com>
On 07/09/2019 00:44, Aleksa Sarai wrote:
> On 2019-09-06, Andy Lutomirski <luto@amacapital.net> wrote:
>>> On Sep 6, 2019, at 12:07 PM, Steve Grubb <sgrubb@redhat.com> wrote:
>>>
>>>> On Friday, September 6, 2019 2:57:00 PM EDT Florian Weimer wrote:
>>>> * Steve Grubb:
>>>>> Now with LD_AUDIT
>>>>> $ LD_AUDIT=/home/sgrubb/test/openflags/strip-flags.so.0 strace ./test
>>>>> 2>&1 | grep passwd openat(3, "passwd", O_RDONLY) = 4
>>>>>
>>>>> No O_CLOEXEC flag.
>>>>
>>>> I think you need to explain in detail why you consider this a problem.
Right, LD_PRELOAD and such things are definitely not part of the threat
model for O_MAYEXEC, on purpose, because this must be addressed with
other security mechanism (e.g. correct file system access-control, IMA
policy, SELinux or other LSM security policies). This is a requirement
for O_MAYEXEC to be useful.
An interpreter is just a flexible program which is generic and doesn't
have other purpose other than behaving accordingly to external rules
(i.e. scripts). If you don't trust your interpreter, it should not be
executable in the first place. O_MAYEXEC enables to restrict the use of
(some) interpreters accordingly to a *global* system security policy.
>>>
>>> Because you can strip the O_MAYEXEC flag from being passed into the kernel.
>>> Once you do that, you defeat the security mechanism because it never gets
>>> invoked. The issue is that the only thing that knows _why_ something is being
>>> opened is user space. With this mechanism, you can attempt to pass this
>>> reason to the kernel so that it may see if policy permits this. But you can
>>> just remove the flag.
>>
>> I’m with Florian here. Once you are executing code in a process, you
>> could just emulate some other unapproved code. This series is not
>> intended to provide the kind of absolute protection you’re imagining.
>
> I also agree, though I think that there is a separate argument to be
> made that there are two possible problems with O_MAYEXEC (which might
> not be really big concerns):
>
> * It's very footgun-prone if you didn't call O_MAYEXEC yourself and
> you pass the descriptor elsewhere. You need to check f_flags to see
> if it contains O_MAYEXEC. Maybe there is an argument to be made that
> passing O_MAYEXECs around isn't a valid use-case, but in that case
> there should be some warnings about that.
That could be an issue if you don't trust your system, especially if the
mount points (and the "noexec" option) can be changed by untrusted
users. As I said above, there is a requirement for basic security
properties as a meaningful file system access control, and obviously not
letting any user change mount points (which can lead to much sever
security issues anyway).
If a process A pass a FD to an interpreter B, then the interpreter B
must trust the process A. Moreover, being able to tell if the FD was
open with O_MAYEXEC and relying on it may create a wrong feeling of
security. As I said in a previous email, being able to probe for
O_MAYEXEC does not make sense because it would not be enough to
know the system policy (either this flag is enforced or not, for mount
points, based on xattr, time…). The main goal of O_MAYEXEC is to ask the
kernel, on a trusted link (hence without LD_PRELOAD-like interfering),
for a file which is allowed to be interpreted/executed by this interpreter.
To be able to correctly handle the case you pointed out (FD passing),
either an existing or a new LSM should handle this behavior according to
the origin of the FD and the chain of processes getting it.
Some advanced LSM rules could tie interpreters with scripts dedicated to
them, and have different behavior for the same scripts but with
different interpreters.
>
> * There's effectively a TOCTOU flaw (even if you are sure O_MAYEXEC is
> in f_flags) -- if the filesystem becomes re-mounted noexec (or the
> file has a-x permissions) after you've done the check you won't get
> hit with an error when you go to use the file descriptor later.
Again, the threat model needs to be appropriate to make O_MAYEXEC
useful. The security policies of the system need to be seen as a whole,
and updated as such.
As for most file system access control on Linux, it may be possible to
have TOCTOU, but the whole system should be designed to protect against
that. For example, changing file access control (e.g. mount point
options) without a reboot may lead to inconsistent security properties,
which is why such thing are discouraged by some access control systems
(e.g. SELinux).
>
> To fix both you'd need to do what you mention later:
>
>> What the kernel *could* do is prevent mmapping a non-FMODE_EXEC file
>> with PROT_EXEC, which would indeed have a real effect (in an iOS-like
>> world, for example) but would break many, many things.
>
> And I think this would be useful (with the two possible ways of
> executing .text split into FMODE_EXEC and FMODE_MAP_EXEC, as mentioned
> in a sister subthread), but would have to be opt-in for the obvious
> reason you outlined. However, we could make it the default for
> openat2(2) -- assuming we can agree on what the semantics of a
> theoretical FMODE_EXEC should be.
>
> And of course we'd need to do FMODE_UPGRADE_EXEC (which would need to
> also permit fexecve(2) though probably not PROT_EXEC -- I don't think
> you can mmap() an O_PATH descriptor).
The mmapping restriction may be interesting but it is a different use
case. This series address the interpreter/script problem. Either the
script may be mapped executable is the choice of the interpreter. In
most cases, no script are mapped as such, exactly because they are
interpreted by a process but not by the CPU.
--
Mickaël Salaün
Les données à caractère personnel recueillies et traitées dans le cadre de cet échange, le sont à seule fin d’exécution d’une relation professionnelle et s’opèrent dans cette seule finalité et pour la durée nécessaire à cette relation. Si vous souhaitez faire usage de vos droits de consultation, de rectification et de suppression de vos données, veuillez contacter contact.rgpd@sgdsn.gouv.fr. Si vous avez reçu ce message par erreur, nous vous remercions d’en informer l’expéditeur et de détruire le message. The personal data collected and processed during this exchange aims solely at completing a business relationship and is limited to the necessary duration of that relationship. If you wish to use your rights of consultation, rectification and deletion of your data, please contact: contact.rgpd@sgdsn.gouv.fr. If you have received this message in error, we thank you for informing the sender and destroying the message.
^ permalink raw reply
* Re: [PATCH v2 1/5] fs: Add support for an O_MAYEXEC flag on sys_open()
From: Mickaël Salaün @ 2019-09-09 9:18 UTC (permalink / raw)
To: Andy Lutomirski, Jeff Layton
Cc: Florian Weimer, Mickaël Salaün, linux-kernel,
Aleksa Sarai, Alexei Starovoitov, Al Viro, Andy Lutomirski,
Christian Heimes, Daniel Borkmann, Eric Chiang, James Morris,
Jan Kara, Jann Horn, Jonathan Corbet, Kees Cook, Matthew Garrett,
Matthew Wilcox, Michael Kerrisk, Mimi Zohar,
Philippe Trébuchet, Scott Shell, Sean Christopherson,
Shuah Khan, Song Liu, Steve Dower, Steve Grubb, Thibaut Sautereau,
Vincent Strubel, Yves-Alexis Perez, kernel-hardening, linux-api,
linux-security-module, linux-fsdevel
In-Reply-To: <D1212E06-773B-42B9-B7C3-C4C1C2A6111D@amacapital.net>
On 06/09/2019 20:41, Andy Lutomirski wrote:
>
>
>> On Sep 6, 2019, at 11:38 AM, Jeff Layton <jlayton@kernel.org> wrote:
>>
>>> On Fri, 2019-09-06 at 19:14 +0200, Mickaël Salaün wrote:
>>>> On 06/09/2019 18:48, Jeff Layton wrote:
>>>>> On Fri, 2019-09-06 at 18:06 +0200, Mickaël Salaün wrote:
>>>>>> On 06/09/2019 17:56, Florian Weimer wrote:
>>>>>> Let's assume I want to add support for this to the glibc dynamic loader,
>>>>>> while still being able to run on older kernels.
>>>>>>
>>>>>> Is it safe to try the open call first, with O_MAYEXEC, and if that fails
>>>>>> with EINVAL, try again without O_MAYEXEC?
>>>>>
>>>>> The kernel ignore unknown open(2) flags, so yes, it is safe even for
>>>>> older kernel to use O_MAYEXEC.
>>>>>
>>>>
>>>> Well...maybe. What about existing programs that are sending down bogus
>>>> open flags? Once you turn this on, they may break...or provide a way to
>>>> circumvent the protections this gives.
>>>
>>> Well, I don't think we should nor could care about bogus programs that
>>> do not conform to the Linux ABI.
>>>
>>
>> But they do conform. The ABI is just undefined here. Unknown flags are
>> ignored so we never really know if $random_program may be setting them.
>>
>>>> Maybe this should be a new flag that is only usable in the new openat2()
>>>> syscall that's still under discussion? That syscall will enforce that
>>>> all flags are recognized. You presumably wouldn't need the sysctl if you
>>>> went that route too.
>>>
>>> Here is a thread about a new syscall:
>>> https://lore.kernel.org/lkml/1544699060.6703.11.camel@linux.ibm.com/
>>>
>>> I don't think it fit well with auditing nor integrity. Moreover using
>>> the current open(2) behavior of ignoring unknown flags fit well with the
>>> usage of O_MAYEXEC (because it is only a hint to the kernel about the
>>> use of the *opened* file).
>>>
>>
>> The fact that open and openat didn't vet unknown flags is really a bug.
>>
>> Too late to fix it now, of course, and as Aleksa points out, we've
>> worked around that in the past. Now though, we have a new openat2
>> syscall on the horizon. There's little need to continue these sorts of
>> hacks.
>>
>> New open flags really have no place in the old syscalls, IMO.
>>
>>>> Anyone that wants to use this will have to recompile anyway. If the
>>>> kernel doesn't support openat2 or if the flag is rejected then you know
>>>> that you have no O_MAYEXEC support and can decide what to do.
>>>
>>> If we want to enforce a security policy, we need to either be the system
>>> administrator or the distro developer. If a distro ship interpreters
>>> using this flag, we don't need to recompile anything, but we need to be
>>> able to control the enforcement according to the mount point
>>> configuration (or an advanced MAC, or an IMA config). I don't see why an
>>> userspace process should check if this flag is supported or not, it
>>> should simply use it, and the sysadmin will enable an enforcement if it
>>> makes sense for the whole system.
>>>
>>
>> A userland program may need to do other risk mitigation if it sets
>> O_MAYEXEC and the kernel doesn't recognize it.
>>
>> Personally, here's what I'd suggest:
>>
>> - Base this on top of the openat2 set
>> - Change it that so that openat2() files are non-executable by default. Anyone wanting to do that needs to set O_MAYEXEC or upgrade the fd somehow.
>> - Only have the openat2 syscall pay attention to O_MAYEXEC. Let open and openat continue ignoring the new flag.
>>
>> That works around a whole pile of potential ABI headaches. Note that
>> we'd need to make that decision before the openat2 patches are merged.
>>
>> Even better would be to declare the new flag in some openat2-only flag
>> space, so there's no confusion about it being supported by legacy open
>> calls.
>>
>> If glibc wants to implement an open -> openat2 wrapper in userland
>> later, it can set that flag in the wrapper implicitly to emulate the old
>> behavior.
>>
>> Given that you're going to have to recompile software to take advantage
>> of this anyway, what's the benefit to changing legacy syscalls?
>>
>>>>>> Or do I risk disabling this security feature if I do that?
>>>>>
>>>>> It is only a security feature if the kernel support it, otherwise it is
>>>>> a no-op.
>>>>>
>>>>
>>>> With a security feature, I think we really want userland to aware of
>>>> whether it works.
>>>
>>> If userland would like to enforce something, it can already do it
>>> without any kernel modification. The goal of the O_MAYEXEC flag is to
>>> enable the kernel, hence sysadmins or system designers, to enforce a
>>> global security policy that makes sense.
>>>
>>
>> I don't see how this helps anything if you can't tell whether the kernel
>> recognizes the damned thing. Also, our track record with global sysctl
>> switches like this is pretty poor. They're an administrative headache as
>> well as a potential attack vector.
>
> I tend to agree. The sysctl seems like it’s asking for trouble. I can see an ld.so.conf option to turn this thing off making sense.
The sysctl is required to enable the adoption of this flag without
breaking existing systems. Current systems may have "noexec" on mount
points containing scripts. Without giving the ability to the sysadmin to
control that behavior, updating to a newer version of an interpreter
using O_MAYEXEC may break such systems.
How would you do this with ld.so.conf ?
--
Mickaël Salaün
Les données à caractère personnel recueillies et traitées dans le cadre de cet échange, le sont à seule fin d’exécution d’une relation professionnelle et s’opèrent dans cette seule finalité et pour la durée nécessaire à cette relation. Si vous souhaitez faire usage de vos droits de consultation, de rectification et de suppression de vos données, veuillez contacter contact.rgpd@sgdsn.gouv.fr. Si vous avez reçu ce message par erreur, nous vous remercions d’en informer l’expéditeur et de détruire le message. The personal data collected and processed during this exchange aims solely at completing a business relationship and is limited to the necessary duration of that relationship. If you wish to use your rights of consultation, rectification and deletion of your data, please contact: contact.rgpd@sgdsn.gouv.fr. If you have received this message in error, we thank you for informing the sender and destroying the message.
^ permalink raw reply
* Re: [PATCH v2 1/5] fs: Add support for an O_MAYEXEC flag on sys_open()
From: Mickaël Salaün @ 2019-09-09 9:25 UTC (permalink / raw)
To: James Morris, Jeff Layton
Cc: Florian Weimer, Mickaël Salaün, linux-kernel,
Aleksa Sarai, Alexei Starovoitov, Al Viro, Andy Lutomirski,
Christian Heimes, Daniel Borkmann, Eric Chiang, Jan Kara,
Jann Horn, Jonathan Corbet, Kees Cook, Matthew Garrett,
Matthew Wilcox, Michael Kerrisk, Mimi Zohar,
Philippe Trébuchet, Scott Shell, Sean Christopherson,
Shuah Khan, Song Liu, Steve Dower, Steve Grubb, Thibaut Sautereau,
Vincent Strubel, Yves-Alexis Perez, kernel-hardening, linux-api,
linux-security-module, linux-fsdevel
In-Reply-To: <alpine.LRH.2.21.1909061202070.18660@namei.org>
On 06/09/2019 21:03, James Morris wrote:
> On Fri, 6 Sep 2019, Jeff Layton wrote:
>
>> The fact that open and openat didn't vet unknown flags is really a bug.
>>
>> Too late to fix it now, of course, and as Aleksa points out, we've
>> worked around that in the past. Now though, we have a new openat2
>> syscall on the horizon. There's little need to continue these sorts of
>> hacks.
>>
>> New open flags really have no place in the old syscalls, IMO.
>
> Agree here. It's unfortunate but a reality and Linus will reject any such
> changes which break existing userspace.
Do you mean that adding new flags to open(2) is not possible?
Does it means that unspecified behaviors are definitely part of the
Linux specification and can't be fixed?
As I said, O_MAYEXEC should be ignored if it is not supported by the
kernel, which perfectly fit with the current open(2) flags behavior, and
should also behave the same with openat2(2).
--
Mickaël Salaün
Les données à caractère personnel recueillies et traitées dans le cadre de cet échange, le sont à seule fin d’exécution d’une relation professionnelle et s’opèrent dans cette seule finalité et pour la durée nécessaire à cette relation. Si vous souhaitez faire usage de vos droits de consultation, de rectification et de suppression de vos données, veuillez contacter contact.rgpd@sgdsn.gouv.fr. Si vous avez reçu ce message par erreur, nous vous remercions d’en informer l’expéditeur et de détruire le message. The personal data collected and processed during this exchange aims solely at completing a business relationship and is limited to the necessary duration of that relationship. If you wish to use your rights of consultation, rectification and deletion of your data, please contact: contact.rgpd@sgdsn.gouv.fr. If you have received this message in error, we thank you for informing the sender and destroying the message.
^ permalink raw reply
* Re: [PATCH v2 1/5] fs: Add support for an O_MAYEXEC flag on sys_open()
From: Mickaël Salaün @ 2019-09-09 9:33 UTC (permalink / raw)
To: Andy Lutomirski, Jeff Layton
Cc: Aleksa Sarai, Florian Weimer, Mickaël Salaün,
linux-kernel, Alexei Starovoitov, Al Viro, Andy Lutomirski,
Christian Heimes, Daniel Borkmann, Eric Chiang, James Morris,
Jan Kara, Jann Horn, Jonathan Corbet, Kees Cook, Matthew Garrett,
Matthew Wilcox, Michael Kerrisk, Mimi Zohar,
Philippe Trébuchet, Scott Shell, Sean Christopherson,
Shuah Khan, Song Liu, Steve Dower, Steve Grubb, Thibaut Sautereau,
Vincent Strubel, Yves-Alexis Perez, kernel-hardening, linux-api,
linux-security-module, linux-fsdevel
In-Reply-To: <D2A57C7B-B0FD-424E-9F81-B858FFF21FF0@amacapital.net>
On 06/09/2019 22:06, Andy Lutomirski wrote:
>
>
>> On Sep 6, 2019, at 12:43 PM, Jeff Layton <jlayton@kernel.org> wrote:
>>
>>> On Sat, 2019-09-07 at 03:13 +1000, Aleksa Sarai wrote:
>>>> On 2019-09-06, Jeff Layton <jlayton@kernel.org> wrote:
>>>>> On Fri, 2019-09-06 at 18:06 +0200, Mickaël Salaün wrote:
>>>>>> On 06/09/2019 17:56, Florian Weimer wrote:
>>>>>> Let's assume I want to add support for this to the glibc dynamic loader,
>>>>>> while still being able to run on older kernels.
>>>>>>
>>>>>> Is it safe to try the open call first, with O_MAYEXEC, and if that fails
>>>>>> with EINVAL, try again without O_MAYEXEC?
>>>>>
>>>>> The kernel ignore unknown open(2) flags, so yes, it is safe even for
>>>>> older kernel to use O_MAYEXEC.
>>>>>
>>>>
>>>> Well...maybe. What about existing programs that are sending down bogus
>>>> open flags? Once you turn this on, they may break...or provide a way to
>>>> circumvent the protections this gives.
>>>
>>> It should be noted that this has been a valid concern for every new O_*
>>> flag introduced (and yet we still introduced new flags, despite the
>>> concern) -- though to be fair, O_TMPFILE actually does have a
>>> work-around with the O_DIRECTORY mask setup.
>>>
>>> The openat2() set adds O_EMPTYPATH -- though in fairness it's also
>>> backwards compatible because empty path strings have always given ENOENT
>>> (or EINVAL?) while O_EMPTYPATH is a no-op non-empty strings.
>>>
>>>> Maybe this should be a new flag that is only usable in the new openat2()
>>>> syscall that's still under discussion? That syscall will enforce that
>>>> all flags are recognized. You presumably wouldn't need the sysctl if you
>>>> went that route too.
>>>
>>> I'm also interested in whether we could add an UPGRADE_NOEXEC flag to
>>> how->upgrade_mask for the openat2(2) patchset (I reserved a flag bit for
>>> it, since I'd heard about this work through the grape-vine).
>>>
>>
>> I rather like the idea of having openat2 fds be non-executable by
>> default, and having userland request it specifically via O_MAYEXEC (or
>> some similar openat2 flag) if it's needed. Then you could add an
>> UPGRADE_EXEC flag instead?
>>
>> That seems like something reasonable to do with a brand new API, and
>> might be very helpful for preventing certain classes of attacks.
>>
>>
>
> There are at least four concepts of executability here:
>
> - Just check the file mode and any other relevant permissions. Return a normal fd. Makes sense for script interpreters, perhaps.
This is the purpose of this patch series. It doesn't make sense to add
memory restrictions nor constrain fexecve and such.
>
> - Make the fd fexecve-able.
>
> - Make the resulting fd mappable PROT_EXEC.
>
> - Make the resulting fd upgradable.
>
> I’m not at all convinced that the kernel needs to distinguish all these, but at least upgradability should be its own thing IMO.
>
--
Mickaël Salaün
Les données à caractère personnel recueillies et traitées dans le cadre de cet échange, le sont à seule fin d’exécution d’une relation professionnelle et s’opèrent dans cette seule finalité et pour la durée nécessaire à cette relation. Si vous souhaitez faire usage de vos droits de consultation, de rectification et de suppression de vos données, veuillez contacter contact.rgpd@sgdsn.gouv.fr. Si vous avez reçu ce message par erreur, nous vous remercions d’en informer l’expéditeur et de détruire le message. The personal data collected and processed during this exchange aims solely at completing a business relationship and is limited to the necessary duration of that relationship. If you wish to use your rights of consultation, rectification and deletion of your data, please contact: contact.rgpd@sgdsn.gouv.fr. If you have received this message in error, we thank you for informing the sender and destroying the message.
^ permalink raw reply
* Re: [PATCH v2 1/5] fs: Add support for an O_MAYEXEC flag on sys_open()
From: James Morris @ 2019-09-09 10:12 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Jeff Layton, Florian Weimer, Mickaël Salaün,
linux-kernel, Aleksa Sarai, Alexei Starovoitov, Al Viro,
Andy Lutomirski, Christian Heimes, Daniel Borkmann, Eric Chiang,
Jan Kara, Jann Horn, Jonathan Corbet, Kees Cook, Matthew Garrett,
Matthew Wilcox, Michael Kerrisk, Mimi Zohar,
Philippe Trébuchet, Scott Shell, Sean Christopherson,
Shuah Khan, Song Liu, Steve Dower, Steve Grubb, Thibaut Sautereau,
Vincent Strubel, Yves-Alexis Perez, kernel-hardening, linux-api,
linux-security-module, linux-fsdevel
In-Reply-To: <49e98ece-e85f-3006-159b-2e04ba67019e@ssi.gouv.fr>
[-- Attachment #1: Type: text/plain, Size: 1211 bytes --]
On Mon, 9 Sep 2019, Mickaël Salaün wrote:
>
> On 06/09/2019 21:03, James Morris wrote:
> > On Fri, 6 Sep 2019, Jeff Layton wrote:
> >
> >> The fact that open and openat didn't vet unknown flags is really a bug.
> >>
> >> Too late to fix it now, of course, and as Aleksa points out, we've
> >> worked around that in the past. Now though, we have a new openat2
> >> syscall on the horizon. There's little need to continue these sorts of
> >> hacks.
> >>
> >> New open flags really have no place in the old syscalls, IMO.
> >
> > Agree here. It's unfortunate but a reality and Linus will reject any such
> > changes which break existing userspace.
>
> Do you mean that adding new flags to open(2) is not possible?
>
> Does it means that unspecified behaviors are definitely part of the
> Linux specification and can't be fixed?
This is my understanding.
>
> As I said, O_MAYEXEC should be ignored if it is not supported by the
> kernel, which perfectly fit with the current open(2) flags behavior, and
> should also behave the same with openat2(2).
The problem here is programs which are already using the value of
O_MAYEXEC, which will break. Hence, openat2(2).
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH v2 1/5] fs: Add support for an O_MAYEXEC flag on sys_open()
From: Mickaël Salaün @ 2019-09-09 10:54 UTC (permalink / raw)
To: James Morris
Cc: Jeff Layton, Florian Weimer, Mickaël Salaün,
linux-kernel, Aleksa Sarai, Alexei Starovoitov, Al Viro,
Andy Lutomirski, Christian Heimes, Daniel Borkmann, Eric Chiang,
Jan Kara, Jann Horn, Jonathan Corbet, Kees Cook, Matthew Garrett,
Matthew Wilcox, Michael Kerrisk, Mimi Zohar,
Philippe Trébuchet, Scott Shell, Sean Christopherson,
Shuah Khan, Song Liu, Steve Dower, Steve Grubb, Thibaut Sautereau,
Vincent Strubel, Yves-Alexis Perez, kernel-hardening, linux-api,
linux-security-module, linux-fsdevel
In-Reply-To: <alpine.LRH.2.21.1909090309260.27895@namei.org>
On 09/09/2019 12:12, James Morris wrote:
> On Mon, 9 Sep 2019, Mickaël Salaün wrote:
>
>>
>> On 06/09/2019 21:03, James Morris wrote:
>>> On Fri, 6 Sep 2019, Jeff Layton wrote:
>>>
>>>> The fact that open and openat didn't vet unknown flags is really a bug.
>>>>
>>>> Too late to fix it now, of course, and as Aleksa points out, we've
>>>> worked around that in the past. Now though, we have a new openat2
>>>> syscall on the horizon. There's little need to continue these sorts of
>>>> hacks.
>>>>
>>>> New open flags really have no place in the old syscalls, IMO.
>>>
>>> Agree here. It's unfortunate but a reality and Linus will reject any such
>>> changes which break existing userspace.
>>
>> Do you mean that adding new flags to open(2) is not possible?
>>
>> Does it means that unspecified behaviors are definitely part of the
>> Linux specification and can't be fixed?
>
> This is my understanding.
>
>>
>> As I said, O_MAYEXEC should be ignored if it is not supported by the
>> kernel, which perfectly fit with the current open(2) flags behavior, and
>> should also behave the same with openat2(2).
>
> The problem here is programs which are already using the value of
> O_MAYEXEC, which will break. Hence, openat2(2).
Well, it still depends on the sysctl, which doesn't enforce anything by
default, hence doesn't break existing behavior, and this unused flags
could be fixed/removed or reported by sysadmins or distro developers.
--
Mickaël Salaün
Les données à caractère personnel recueillies et traitées dans le cadre de cet échange, le sont à seule fin d’exécution d’une relation professionnelle et s’opèrent dans cette seule finalité et pour la durée nécessaire à cette relation. Si vous souhaitez faire usage de vos droits de consultation, de rectification et de suppression de vos données, veuillez contacter contact.rgpd@sgdsn.gouv.fr. Si vous avez reçu ce message par erreur, nous vous remercions d’en informer l’expéditeur et de détruire le message. The personal data collected and processed during this exchange aims solely at completing a business relationship and is limited to the necessary duration of that relationship. If you wish to use your rights of consultation, rectification and deletion of your data, please contact: contact.rgpd@sgdsn.gouv.fr. If you have received this message in error, we thank you for informing the sender and destroying the message.
^ permalink raw reply
* Re: [PATCH 2/5] integrity: remove pointless subdir-$(CONFIG_...)
From: Masahiro Yamada @ 2019-09-09 11:44 UTC (permalink / raw)
To: Mimi Zohar, linux-integrity
Cc: Dave Howells, James Morris, Josh Boyer, Martin Schwidefsky,
Nayna Jain, Serge E. Hallyn, Linux Kernel Mailing List,
linux-security-module
In-Reply-To: <20190726021058.4212-3-yamada.masahiro@socionext.com>
On Fri, Jul 26, 2019 at 11:12 AM Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
>
> The ima/ and evm/ sub-directories contain built-in objects, so
> obj-$(CONFIG_...) is the correct way to descend into them.
>
> subdir-$(CONFIG_...) is redundant.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
Ping.
>
> security/integrity/Makefile | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/security/integrity/Makefile b/security/integrity/Makefile
> index b6d6273a4176..35e6ca773734 100644
> --- a/security/integrity/Makefile
> +++ b/security/integrity/Makefile
> @@ -14,7 +14,5 @@ integrity-$(CONFIG_LOAD_UEFI_KEYS) += platform_certs/efi_parser.o \
> platform_certs/load_uefi.o
> integrity-$(CONFIG_LOAD_IPL_KEYS) += platform_certs/load_ipl_s390.o
>
> -subdir-$(CONFIG_IMA) += ima
> obj-$(CONFIG_IMA) += ima/
> -subdir-$(CONFIG_EVM) += evm
> obj-$(CONFIG_EVM) += evm/
> --
> 2.17.1
>
--
Best Regards
Masahiro Yamada
^ permalink raw reply
* Re: [PATCH v2 1/5] fs: Add support for an O_MAYEXEC flag on sys_open()
From: Aleksa Sarai @ 2019-09-09 11:54 UTC (permalink / raw)
To: Mickaël Salaün
Cc: James Morris, Jeff Layton, Florian Weimer,
Mickaël Salaün, linux-kernel, Alexei Starovoitov,
Al Viro, Andy Lutomirski, Christian Heimes, Daniel Borkmann,
Eric Chiang, Jan Kara, Jann Horn, Jonathan Corbet, Kees Cook,
Matthew Garrett, Matthew Wilcox, Michael Kerrisk, Mimi Zohar,
Philippe Trébuchet, Scott Shell, Sean Christopherson,
Shuah Khan, Song Liu, Steve Dower, Steve Grubb, Thibaut Sautereau,
Vincent Strubel, Yves-Alexis Perez, kernel-hardening, linux-api,
linux-security-module, linux-fsdevel
In-Reply-To: <49e98ece-e85f-3006-159b-2e04ba67019e@ssi.gouv.fr>
[-- Attachment #1: Type: text/plain, Size: 2717 bytes --]
On 2019-09-09, Mickaël Salaün <mickael.salaun@ssi.gouv.fr> wrote:
> On 06/09/2019 21:03, James Morris wrote:
> > On Fri, 6 Sep 2019, Jeff Layton wrote:
> >
> >> The fact that open and openat didn't vet unknown flags is really a bug.
> >>
> >> Too late to fix it now, of course, and as Aleksa points out, we've
> >> worked around that in the past. Now though, we have a new openat2
> >> syscall on the horizon. There's little need to continue these sorts of
> >> hacks.
> >>
> >> New open flags really have no place in the old syscalls, IMO.
> >
> > Agree here. It's unfortunate but a reality and Linus will reject any such
> > changes which break existing userspace.
>
> Do you mean that adding new flags to open(2) is not possible?
It is possible, as long as there is no case where a program that works
today (and passes garbage to the unused bits in flags) works with the
change.
O_TMPFILE was okay because it's actually two flags (one is O_DIRECTORY)
and no working program does file IO to a directory (there are also some
other tricky things done there, I'll admit I don't fully understand it).
O_EMPTYPATH works because it's a no-op with non-empty path strings, and
empty path strings have always given an error (so no working program
does it today).
However, O_MAYEXEC will result in programs that pass garbage bits to
potentially get -EACCES that worked previously.
> As I said, O_MAYEXEC should be ignored if it is not supported by the
> kernel, which perfectly fit with the current open(2) flags behavior, and
> should also behave the same with openat2(2).
NACK on having that behaviour with openat2(2). -EINVAL on unknown flags
is how all other syscalls work (any new syscall proposed today that
didn't do that would be rightly rejected), and is a quirk of open(2)
which unfortunately cannot be fixed. The fact that *every new O_ flag
needs to work around this problem* should be an indication that this
interface mis-design should not be allowed to infect any more syscalls.
Note that this point is regardless of the fact that O_MAYEXEC is a
*security* flag -- if userspace wants to have a secure fallback on
old kernels (which is "the right thing" to do) they would have to do
more work than necessary. And programs that don't care don't have to do
anything special.
However with -EINVAL, the programs doing "the right thing" get an easy
-EINVAL check. And programs that don't care can just un-set O_MAYEXEC
and retry. You should be forced to deal with the case where a flag is
not supported -- and this is doubly true of security flags!
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH 1/5] integrity: remove unneeded, broken attempt to add -fshort-wchar
From: Masahiro Yamada @ 2019-09-09 11:43 UTC (permalink / raw)
To: Mimi Zohar, linux-integrity
Cc: Dave Howells, James Morris, Josh Boyer, Martin Schwidefsky,
Nayna Jain, Serge E. Hallyn, Linux Kernel Mailing List,
linux-security-module
In-Reply-To: <20190726021058.4212-2-yamada.masahiro@socionext.com>
On Fri, Jul 26, 2019 at 11:11 AM Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
>
> I guess commit 15ea0e1e3e18 ("efi: Import certificates from UEFI Secure
> Boot") attempted to add -fshort-wchar for building load_uefi.o, but it
> has never worked as intended.
>
> load_uefi.o is created in the platform_certs/ sub-directory. If you
> really want to add -fshort-wchar, the correct code is:
>
> $(obj)/platform_certs/load_uefi.o: KBUILD_CFLAGS += -fshort-wchar
>
> or, in a more Kbuild-ish way:
>
> CFLAGS_load_uefi.o := -fshort-wchar
>
> But, you do not need to fix it.
>
> Commit 8c97023cf051 ("Kbuild: use -fshort-wchar globally") had already
> added -fshort-wchar globally. This code was unneeded in the first place.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
Ping.
>
> security/integrity/Makefile | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/security/integrity/Makefile b/security/integrity/Makefile
> index 19faace69644..b6d6273a4176 100644
> --- a/security/integrity/Makefile
> +++ b/security/integrity/Makefile
> @@ -13,7 +13,6 @@ integrity-$(CONFIG_INTEGRITY_PLATFORM_KEYRING) += platform_certs/platform_keyrin
> integrity-$(CONFIG_LOAD_UEFI_KEYS) += platform_certs/efi_parser.o \
> platform_certs/load_uefi.o
> integrity-$(CONFIG_LOAD_IPL_KEYS) += platform_certs/load_ipl_s390.o
> -$(obj)/load_uefi.o: KBUILD_CFLAGS += -fshort-wchar
>
> subdir-$(CONFIG_IMA) += ima
> obj-$(CONFIG_IMA) += ima/
> --
> 2.17.1
>
--
Best Regards
Masahiro Yamada
^ permalink raw reply
* [PATCH v6 00/12] add integrity and security to TPM2 transactions
From: James Bottomley @ 2019-09-09 12:16 UTC (permalink / raw)
To: linux-integrity; +Cc: linux-crypto, linux-security-module, Jarkko Sakkinen
Link to previous cover letter:
https://lore.kernel.org/linux-integrity/1540193596.3202.7.camel@HansenPartnership.com/
This is marked v6 instead of v5 because I did a v5 after feedback on v4
but didn't get around to posting it and then had to rework the whole of
the kernel space handling while I was on holiday. I also added the
documentation of how the whole thing works and the rationale for doing
it in tpm-security.rst (patch 11). The main reason for doing this now
is so we have something to discuss at Plumbers.
The new patch set implements the various splits requested, but the main
changes are that the kernel space is gone and is replaced by a context
save and restore of the generated null seed. This is easier to handle
than a full kernel space given the new threading for TPM spaces, but
conceptually it is still very like a space. I've also made whether
integrity and encryption is turned on a Kconfig option.
James
---
James Bottomley (12):
tpm-buf: move from static inlines to real functions
tpm-buf: add handling for TPM2B types
tpm-buf: add cursor based functions for response parsing
tpm2-space: export the context save and load commands
tpm2-sessions: Add full HMAC and encrypt/decrypt session handling
tpm-buf: add tpm_buf_parameters()
tpm2: add hmac checks to tpm2_pcr_extend()
tpm2: add session encryption protection to tpm2_get_random()
trusted keys: Add session encryption protection to the seal/unseal
path
tpm: add the null key name as a tpm2 sysfs variable
Documentation: add tpm-security.rst
tpm2-sessions: NOT FOR COMMITTING add sessions testing
Documentation/security/tpm/tpm-security.rst | 204 +++++
drivers/char/tpm/Kconfig | 11 +
drivers/char/tpm/Makefile | 4 +
drivers/char/tpm/tpm-buf.c | 202 +++++
drivers/char/tpm/tpm-chip.c | 1 +
drivers/char/tpm/tpm-sysfs.c | 27 +-
drivers/char/tpm/tpm.h | 117 +--
drivers/char/tpm/tpm2-cmd.c | 202 +++--
drivers/char/tpm/tpm2-sessions-test.c | 795 ++++++++++++++++++
drivers/char/tpm/tpm2-sessions.c | 1204 +++++++++++++++++++++++++++
drivers/char/tpm/tpm2-sessions.h | 138 +++
drivers/char/tpm/tpm2-space.c | 8 +-
include/linux/tpm.h | 29 +
13 files changed, 2787 insertions(+), 155 deletions(-)
create mode 100644 Documentation/security/tpm/tpm-security.rst
create mode 100644 drivers/char/tpm/tpm-buf.c
create mode 100644 drivers/char/tpm/tpm2-sessions-test.c
create mode 100644 drivers/char/tpm/tpm2-sessions.c
create mode 100644 drivers/char/tpm/tpm2-sessions.h
--
2.16.4
^ permalink raw reply
* [PATCH v6 01/12] tpm-buf: move from static inlines to real functions
From: James Bottomley @ 2019-09-09 12:17 UTC (permalink / raw)
To: linux-integrity; +Cc: linux-crypto, linux-security-module, Jarkko Sakkinen
In-Reply-To: <1568031408.6613.29.camel@HansenPartnership.com>
This separates out the old tpm_buf_... handling functions from static
inlines in tpm.h and makes them their own tpm-buf.c file. This is a
precursor so we can add new functions for other TPM type handling
Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
v2: added this patch to separate out the API changes
v3: added tpm_buf_reset_cmd()
v6: extract first then add functions
---
drivers/char/tpm/Makefile | 1 +
drivers/char/tpm/tpm-buf.c | 118 +++++++++++++++++++++++++++++++++++++++++++++
drivers/char/tpm/tpm.h | 90 ++++------------------------------
3 files changed, 129 insertions(+), 80 deletions(-)
create mode 100644 drivers/char/tpm/tpm-buf.c
diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index a01c4cab902a..78bd025b808a 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -15,6 +15,7 @@ tpm-y += tpm-sysfs.o
tpm-y += eventlog/common.o
tpm-y += eventlog/tpm1.o
tpm-y += eventlog/tpm2.o
+tpm-y += tpm-buf.o
tpm-$(CONFIG_ACPI) += tpm_ppi.o eventlog/acpi.o
tpm-$(CONFIG_EFI) += eventlog/efi.o
diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
new file mode 100644
index 000000000000..9fa8a9cb0fdf
--- /dev/null
+++ b/drivers/char/tpm/tpm-buf.c
@@ -0,0 +1,118 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Handing for tpm_buf structures to facilitate the building of commands
+ */
+
+#include "tpm.h"
+
+#include <linux/module.h>
+
+static int __tpm_buf_init(struct tpm_buf *buf)
+{
+ buf->data_page = alloc_page(GFP_HIGHUSER);
+ if (!buf->data_page)
+ return -ENOMEM;
+
+ buf->flags = 0;
+ buf->data = kmap(buf->data_page);
+
+ return 0;
+}
+
+void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal)
+{
+ struct tpm_header *head;
+
+ head = (struct tpm_header *) buf->data;
+
+ head->tag = cpu_to_be16(tag);
+ head->length = cpu_to_be32(sizeof(*head));
+ head->ordinal = cpu_to_be32(ordinal);
+}
+EXPORT_SYMBOL_GPL(tpm_buf_reset);
+
+int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal)
+{
+ int rc;
+
+ rc = __tpm_buf_init(buf);
+ if (rc)
+ return rc;
+
+ tpm_buf_reset(buf, tag, ordinal);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(tpm_buf_init);
+
+void tpm_buf_destroy(struct tpm_buf *buf)
+{
+ kunmap(buf->data_page);
+ __free_page(buf->data_page);
+}
+EXPORT_SYMBOL_GPL(tpm_buf_destroy);
+
+u32 tpm_buf_length(struct tpm_buf *buf)
+{
+ struct tpm_header *head = (struct tpm_header *)buf->data;
+ u32 len;
+
+ len = be32_to_cpu(head->length);
+ if (buf->flags & TPM_BUF_2B)
+ len -= sizeof(*head);
+ return len;
+}
+EXPORT_SYMBOL_GPL(tpm_buf_length);
+
+u16 tpm_buf_tag(struct tpm_buf *buf)
+{
+ struct tpm_header *head = (struct tpm_header *)buf->data;
+
+ return be16_to_cpu(head->tag);
+}
+EXPORT_SYMBOL_GPL(tpm_buf_tag);
+
+void tpm_buf_append(struct tpm_buf *buf,
+ const unsigned char *new_data,
+ unsigned int new_len)
+{
+ struct tpm_header *head = (struct tpm_header *) buf->data;
+ u32 len = be32_to_cpu(head->length);
+
+ /* Return silently if overflow has already happened. */
+ if (buf->flags & TPM_BUF_OVERFLOW)
+ return;
+
+ if ((len + new_len) > PAGE_SIZE) {
+ WARN(1, "tpm_buf: overflow\n");
+ buf->flags |= TPM_BUF_OVERFLOW;
+ return;
+ }
+
+ memcpy(&buf->data[len], new_data, new_len);
+ head->length = cpu_to_be32(len + new_len);
+}
+EXPORT_SYMBOL_GPL(tpm_buf_append);
+
+void tpm_buf_append_u8(struct tpm_buf *buf, const u8 value)
+{
+ tpm_buf_append(buf, &value, 1);
+}
+EXPORT_SYMBOL_GPL(tpm_buf_append_u8);
+
+void tpm_buf_append_u16(struct tpm_buf *buf, const u16 value)
+{
+ __be16 value2 = cpu_to_be16(value);
+
+ tpm_buf_append(buf, (u8 *) &value2, 2);
+}
+EXPORT_SYMBOL_GPL(tpm_buf_append_u16);
+
+void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value)
+{
+ __be32 value2 = cpu_to_be32(value);
+
+ tpm_buf_append(buf, (u8 *) &value2, 4);
+}
+EXPORT_SYMBOL_GPL(tpm_buf_append_u32);
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index a7fea3e0ca86..8c5b8bba60d2 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -281,6 +281,7 @@ enum tpm_sub_capabilities {
enum tpm_buf_flags {
TPM_BUF_OVERFLOW = BIT(0),
+ TPM_BUF_2B = BIT(1),
};
struct tpm_buf {
@@ -289,86 +290,15 @@ struct tpm_buf {
u8 *data;
};
-static inline void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal)
-{
- struct tpm_header *head = (struct tpm_header *)buf->data;
-
- head->tag = cpu_to_be16(tag);
- head->length = cpu_to_be32(sizeof(*head));
- head->ordinal = cpu_to_be32(ordinal);
-}
-
-static inline int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal)
-{
- buf->data_page = alloc_page(GFP_HIGHUSER);
- if (!buf->data_page)
- return -ENOMEM;
-
- buf->flags = 0;
- buf->data = kmap(buf->data_page);
- tpm_buf_reset(buf, tag, ordinal);
- return 0;
-}
-
-static inline void tpm_buf_destroy(struct tpm_buf *buf)
-{
- kunmap(buf->data_page);
- __free_page(buf->data_page);
-}
-
-static inline u32 tpm_buf_length(struct tpm_buf *buf)
-{
- struct tpm_header *head = (struct tpm_header *)buf->data;
-
- return be32_to_cpu(head->length);
-}
-
-static inline u16 tpm_buf_tag(struct tpm_buf *buf)
-{
- struct tpm_header *head = (struct tpm_header *)buf->data;
-
- return be16_to_cpu(head->tag);
-}
-
-static inline void tpm_buf_append(struct tpm_buf *buf,
- const unsigned char *new_data,
- unsigned int new_len)
-{
- struct tpm_header *head = (struct tpm_header *)buf->data;
- u32 len = tpm_buf_length(buf);
-
- /* Return silently if overflow has already happened. */
- if (buf->flags & TPM_BUF_OVERFLOW)
- return;
-
- if ((len + new_len) > PAGE_SIZE) {
- WARN(1, "tpm_buf: overflow\n");
- buf->flags |= TPM_BUF_OVERFLOW;
- return;
- }
-
- memcpy(&buf->data[len], new_data, new_len);
- head->length = cpu_to_be32(len + new_len);
-}
-
-static inline void tpm_buf_append_u8(struct tpm_buf *buf, const u8 value)
-{
- tpm_buf_append(buf, &value, 1);
-}
-
-static inline void tpm_buf_append_u16(struct tpm_buf *buf, const u16 value)
-{
- __be16 value2 = cpu_to_be16(value);
-
- tpm_buf_append(buf, (u8 *) &value2, 2);
-}
-
-static inline void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value)
-{
- __be32 value2 = cpu_to_be32(value);
-
- tpm_buf_append(buf, (u8 *) &value2, 4);
-}
+int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal);
+void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal);
+void tpm_buf_destroy(struct tpm_buf *buf);
+u32 tpm_buf_length(struct tpm_buf *buf);
+void tpm_buf_append(struct tpm_buf *buf, const unsigned char *new_data,
+ unsigned int new_len);
+void tpm_buf_append_u8(struct tpm_buf *buf, const u8 value);
+void tpm_buf_append_u16(struct tpm_buf *buf, const u16 value);
+void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value);
extern struct class *tpm_class;
extern struct class *tpmrm_class;
--
2.16.4
^ permalink raw reply related
* [PATCH v6 02/12] tpm-buf: add handling for TPM2B types
From: James Bottomley @ 2019-09-09 12:18 UTC (permalink / raw)
To: linux-integrity; +Cc: linux-crypto, linux-security-module, Jarkko Sakkinen
In-Reply-To: <1568031408.6613.29.camel@HansenPartnership.com>
Most complex TPM commands require appending TPM2B buffers to the
command body. Since TPM2B types are essentially variable size arrays,
it makes it impossible to represent these complex command arguments as
structures and we simply have to build them up using append primitives
like these.
Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
drivers/char/tpm/tpm-buf.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++
drivers/char/tpm/tpm.h | 2 ++
2 files changed, 49 insertions(+)
diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
index 9fa8a9cb0fdf..8c1ed8a14e01 100644
--- a/drivers/char/tpm/tpm-buf.c
+++ b/drivers/char/tpm/tpm-buf.c
@@ -8,6 +8,8 @@
#include <linux/module.h>
+#include <asm/unaligned.h>
+
static int __tpm_buf_init(struct tpm_buf *buf)
{
buf->data_page = alloc_page(GFP_HIGHUSER);
@@ -46,6 +48,24 @@ int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal)
}
EXPORT_SYMBOL_GPL(tpm_buf_init);
+int tpm_buf_init_2b(struct tpm_buf *buf)
+{
+ struct tpm_header *head;
+ int rc;
+
+ rc = __tpm_buf_init(buf);
+ if (rc)
+ return rc;
+
+ head = (struct tpm_header *) buf->data;
+
+ head->length = cpu_to_be32(sizeof(*head));
+
+ buf->flags = TPM_BUF_2B;
+ return 0;
+}
+EXPORT_SYMBOL_GPL(tpm_buf_init_2b);
+
void tpm_buf_destroy(struct tpm_buf *buf)
{
kunmap(buf->data_page);
@@ -53,6 +73,13 @@ void tpm_buf_destroy(struct tpm_buf *buf)
}
EXPORT_SYMBOL_GPL(tpm_buf_destroy);
+static void *tpm_buf_data(struct tpm_buf *buf)
+{
+ if (buf->flags & TPM_BUF_2B)
+ return buf->data + TPM_HEADER_SIZE;
+ return buf->data;
+}
+
u32 tpm_buf_length(struct tpm_buf *buf)
{
struct tpm_header *head = (struct tpm_header *)buf->data;
@@ -116,3 +143,23 @@ void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value)
tpm_buf_append(buf, (u8 *) &value2, 4);
}
EXPORT_SYMBOL_GPL(tpm_buf_append_u32);
+
+static void tpm_buf_reset_int(struct tpm_buf *buf)
+{
+ struct tpm_header *head;
+
+ head = (struct tpm_header *)buf->data;
+ head->length = cpu_to_be32(sizeof(*head));
+}
+
+void tpm_buf_append_2b(struct tpm_buf *buf, struct tpm_buf *tpm2b)
+{
+ u16 len = tpm_buf_length(tpm2b);
+
+ tpm_buf_append_u16(buf, len);
+ tpm_buf_append(buf, tpm_buf_data(tpm2b), len);
+ /* clear the buf for reuse */
+ tpm_buf_reset_int(tpm2b);
+}
+EXPORT_SYMBOL_GPL(tpm_buf_append_2b);
+
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 8c5b8bba60d2..7627917db345 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -292,6 +292,7 @@ struct tpm_buf {
int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal);
void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal);
+int tpm_buf_init_2b(struct tpm_buf *buf);
void tpm_buf_destroy(struct tpm_buf *buf);
u32 tpm_buf_length(struct tpm_buf *buf);
void tpm_buf_append(struct tpm_buf *buf, const unsigned char *new_data,
@@ -299,6 +300,7 @@ void tpm_buf_append(struct tpm_buf *buf, const unsigned char *new_data,
void tpm_buf_append_u8(struct tpm_buf *buf, const u8 value);
void tpm_buf_append_u16(struct tpm_buf *buf, const u16 value);
void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value);
+void tpm_buf_append_2b(struct tpm_buf *buf, struct tpm_buf *tpm2b);
extern struct class *tpm_class;
extern struct class *tpmrm_class;
--
2.16.4
^ permalink raw reply related
* [PATCH v6 03/12] tpm-buf: add cursor based functions for response parsing
From: James Bottomley @ 2019-09-09 12:19 UTC (permalink / raw)
To: linux-integrity; +Cc: linux-crypto, linux-security-module, Jarkko Sakkinen
In-Reply-To: <1568031408.6613.29.camel@HansenPartnership.com>
It's very convenient when parsing responses to have a cursor you
simply move over the response extracting the data. Add such cursor
functions for the TPM unsigned integer types.
Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
drivers/char/tpm/tpm-buf.c | 26 ++++++++++++++++++++++++++
drivers/char/tpm/tpm.h | 4 ++++
2 files changed, 30 insertions(+)
diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
index 8c1ed8a14e01..553adb84b0ac 100644
--- a/drivers/char/tpm/tpm-buf.c
+++ b/drivers/char/tpm/tpm-buf.c
@@ -163,3 +163,29 @@ void tpm_buf_append_2b(struct tpm_buf *buf, struct tpm_buf *tpm2b)
}
EXPORT_SYMBOL_GPL(tpm_buf_append_2b);
+/* functions for unmarshalling data and moving the cursor */
+u8 tpm_get_inc_u8(const u8 **ptr)
+{
+ return *((*ptr)++);
+}
+EXPORT_SYMBOL_GPL(tpm_get_inc_u8);
+
+u16 tpm_get_inc_u16(const u8 **ptr)
+{
+ u16 val;
+
+ val = get_unaligned_be16(*ptr);
+ *ptr += sizeof(val);
+ return val;
+}
+EXPORT_SYMBOL_GPL(tpm_get_inc_u16);
+
+u32 tpm_get_inc_u32(const u8 **ptr)
+{
+ u32 val;
+
+ val = get_unaligned_be32(*ptr);
+ *ptr += sizeof(val);
+ return val;
+}
+EXPORT_SYMBOL_GPL(tpm_get_inc_u32);
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 7627917db345..d942188debc9 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -302,6 +302,10 @@ void tpm_buf_append_u16(struct tpm_buf *buf, const u16 value);
void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value);
void tpm_buf_append_2b(struct tpm_buf *buf, struct tpm_buf *tpm2b);
+u8 tpm_get_inc_u8(const u8 **ptr);
+u16 tpm_get_inc_u16(const u8 **ptr);
+u32 tpm_get_inc_u32(const u8 **ptr);
+
extern struct class *tpm_class;
extern struct class *tpmrm_class;
extern dev_t tpm_devt;
--
2.16.4
^ 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