* Re: [GIT PULL] selinux/selinux-pr-20250926
From: Linus Torvalds @ 2025-09-30 15:48 UTC (permalink / raw)
To: Paul Moore; +Cc: selinux, linux-security-module, linux-kernel
In-Reply-To: <6edbd0e6dc79902981bf9a34e8d41128@paul-moore.com>
On Fri, 26 Sept 2025 at 20:07, Paul Moore <paul@paul-moore.com> wrote:
>
> - Remove our use of get_zeroed_page() in sel_read_bool()
>
> Update sel_read_bool() to use a four byte stack buffer instead of a
> memory page fetched via get_zeroed_page(), and fix a memory in the
> process.
>
> Needless to say we should have done this a long time ago, but it was
> in a very old chunk of code that "just worked" and I don't think
> anyone had taken a real look at it in many years.
Lol.
... and when I looked at this, I went "scnprintf for a 4-byte buffer?"
It uses
len = scnprintf(buffer, sizeof(buffer), "%d %d", ..
and I went "printing two numbers and just four bytes" before I noticed
that they are just booleans and so 'len' always is just 3.
It literally could have done
char buffer[] = { '0' + !a, ' ', '0' + !!b, 0 };
instead, and I guess a compiler could do that transformation in a perfect world.
But this isn't exactly performance-crticial, so nobody cares.
Linus
^ permalink raw reply
* Re: [PATCH v3 01/10] tpm: Cap the number of PCR banks
From: James Bottomley @ 2025-09-30 14:17 UTC (permalink / raw)
To: Jarkko Sakkinen, Jonathan McDowell
Cc: linux-integrity, dpsmith, ross.philipson, Jarkko Sakkinen,
Roberto Sassu, Peter Huewe, Jason Gunthorpe, David Howells,
Paul Moore, James Morris, Serge E. Hallyn, open list,
open list:KEYS/KEYRINGS, open list:SECURITY SUBSYSTEM
In-Reply-To: <aNvO0ZsZz_jkmpoi@kernel.org>
On Tue, 2025-09-30 at 15:36 +0300, Jarkko Sakkinen wrote:
> On Tue, Sep 30, 2025 at 12:09:15PM +0100, Jonathan McDowell wrote:
> > On Mon, Sep 29, 2025 at 10:48:23PM +0300, Jarkko Sakkinen wrote:
[...]
> > > +#define TPM2_MAX_DIGEST_SIZE SHA512_DIGEST_SIZE
> > > +#define TPM2_MAX_BANKS 4
> >
> > Where does this max come from? It matches what I see with swtpm by
> > default (SHA1, SHA2-256, SHA2-384, SHA-512), so I haven't seen
> > anything that exceeds it myself.
>
> I've never seen hardware TPM that would have more than one or two
> banks. We can double it to leave some room. This was tested with
> swtpm defaults.
I've got a hardware TPM that comes with 3 banks by default (it's a
chinese one which has sha1 sha256 and sm2). swtpm isn't a good
indicator because it's default allocation is rather pejorative (it
disables sha1 whereas most field TPMs don't).
However, if you look at how the reference implementation works, the
user is allowed to define any number of banks they want, up to the
number of supported hashes. The only limitation being there can't be
>1 bank for the same hash. Field TPM implementations are allowed to
constrain this, but most don't. The question you should be asking
here is not how many banks does a particular implementation allow by
default, but what's the maximum number a user could configure.
Regards,
James
^ permalink raw reply
* Re: [PATCH] ima: Fall back to default kernel module signature verification
From: Mimi Zohar @ 2025-09-30 13:57 UTC (permalink / raw)
To: Coiby Xu, linux-integrity
Cc: Dmitry Torokhov, Karel Srot, Roberto Sassu, Dmitry Kasatkin,
Eric Snowberg, Paul Moore, James Morris, Serge E. Hallyn,
open list:SECURITY SUBSYSTEM, open list
In-Reply-To: <20250928030358.3873311-1-coxu@redhat.com>
On Sun, 2025-09-28 at 11:03 +0800, Coiby Xu wrote:
> Currently, for any IMA policy that requires appraisal for kernel modules
> e.g. ima_policy=secure_boot, PowerPC architecture specific policy,
> booting will fail because IMA will reject a kernel module which will
> be decompressed in the kernel space and then have its signature
> verified.
>
> This happens because when in-kernel module decompression
> (CONFIG_MODULE_DECOMPRESS) is enabled, kmod will use finit_module
> syscall instead of init_module to load a module. And IMA mandates IMA
> xattr verification for finit_module unless appraise_type=imasig|modsig
> is specified in the rule. However currently initramfs doesn't support
> xattr. And IMA rule "func=MODULE_CHECK appraise_type=imasig|modsig"
> doesn't work either because IMA will treat to-be-decompressed kernel
> module as not having module signature as it can't decompress kernel
> module to check if signature exists.
>
> So fall back to default kernel module signature verification when we have
> no way to verify IMA xattr.
>
> Reported-by: Karel Srot <ksrot@redhat.com>
> Signed-off-by: Coiby Xu <coxu@redhat.com>
> ---
> Another approach will be to make IMA decompress the kernel module to
> check the signature. This requires refactoring kernel module code to
> make the in-kernel module decompressing feature modular and seemingly
> more efforts are needed. A second disadvantage is it feels
> counter-intuitive to verify the same kernel module signature twice. And
> we still need to make ima_policy=secure_boot allow verifying appended
> module signature.
>
> Anyways, I'm open to suggestions and can try the latter approach if
> there are some benefits I'm not aware of or a better approach.
Coiby, there are multiple issues being discussed here. Before deciding on an
appropriate solution, let's frame the issues(s) properly.
1. The finit_module syscall eventually calls init_module_from_file() to read the
module into memory and then decompress it. The problem is that the kernel
module signature verification occurs during the kernel_read_file(), before the
kernel module is decompressed. Thus, the appended kernel module signature
cannot be verified.
2. CPIO doesn't have xattr support. There were multiple attempts at including
xattrs in CPIO, but none were upstreamed [1]. If file signatures stored in
security.ima were available in the initramfs, then finit_module() could verify
them, as opposed to the appended kernel module signature.
3. The issues described above are generic, not limited to Power. When
CONFIG_MODULE_SIG is configured, the arch specific IMA policy rules do not
include an "appraise func=MODULE_CHECK".
4. Unlike the arch specific IMA policy rules, the built-in secure boot IMA
policy, specified on the boot command line as "ima_policy=secure_boot", always
enforces the IMA signature stored in security.ima.
Partial solutions without kernel changes:
- Enable CONFIG_MODULE_SIG (Doesn't solve 4)
- Disable kernel module compression.
Complete solution:
- Pick up and upstream Roberto Sassu's last version of initramfs support [1].
- Somehow prevent kernel_read_file() from failing when the kernel_read_file_id
enumeration is READING_MODULE and the kernel module is compressed. The change
might be limited to ima_post_read_file().
thanks,
Mimi
[1] [PATCH v4 0/3] initramfs: add support for xattrs in the initial ram disk
https://lore.kernel.org/linux-fsdevel/20190523121803.21638-1-roberto.sassu@huawei.com/
^ permalink raw reply
* Re: [PATCH v3 10/10] tpm-buf: Enable managed and stack allocations.
From: Stefano Garzarella @ 2025-09-30 13:20 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: linux-integrity, dpsmith, ross.philipson, Jarkko Sakkinen,
Stefan Berger, Peter Huewe, Jason Gunthorpe, David Howells,
Paul Moore, James Morris, Serge E. Hallyn, James Bottomley,
Mimi Zohar, open list, open list:KEYS/KEYRINGS,
open list:SECURITY SUBSYSTEM
In-Reply-To: <aNvW5KMUO2C0i233@kernel.org>
On Tue, Sep 30, 2025 at 04:11:00PM +0300, Jarkko Sakkinen wrote:
>On Tue, Sep 30, 2025 at 02:44:52PM +0200, Stefano Garzarella wrote:
>> On Mon, Sep 29, 2025 at 10:48:32PM +0300, Jarkko Sakkinen wrote:
>> > From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
>> >
>> > Decouple kzalloc from buffer creation, so that a managed allocation can be
>> > done trivially:
>> >
>> > struct tpm_buf *buf __free(kfree) buf = kzalloc(TPM_BUFSIZE,
>> ^
>> In the code, we use PAGE_SIZE instead of TPM_BUFSIZE with kzalloc().
>> Should we do the same in this example? (Perhaps adding the reason, if you
>> think it would be useful)
>
>I think that should be fixed up in the patch and use TPM_BUFSIZE
>consistently for kzallocs. Thanks for the remark.
I thought it was done on purpose to alleviate pressure on the allocator
regardless of the page size value.
In any case, it's up to you, I would just be consistent between the
example in the commit and the code.
>
>>
>> > GFP_KERNEL);
>> > if (!buf)
>> > return -ENOMEM;
>> > tpm_buf_init(buf, TPM_BUFSIZE);
>> >
>> > Alternatively, stack allocations are also possible:
>> >
>> > u8 buf_data[512];
>> > struct tpm_buf *buf = (struct tpm_buf *)buf_data;
>> > tpm_buf_init(buf, sizeof(buf_data));
>> >
>> > Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
>> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
>> > ---
>> > v3:
>> > - A new patch from the earlier series with more scoped changes and
>> > less abstract commit message.
>> > ---
>> > drivers/char/tpm/tpm-buf.c | 122 +++++----
>> > drivers/char/tpm/tpm-sysfs.c | 20 +-
>> > drivers/char/tpm/tpm.h | 1 -
>> > drivers/char/tpm/tpm1-cmd.c | 147 +++++------
>> > drivers/char/tpm/tpm2-cmd.c | 290 ++++++++++------------
>> > drivers/char/tpm/tpm2-sessions.c | 121 +++++----
>> > drivers/char/tpm/tpm2-space.c | 44 ++--
>> > drivers/char/tpm/tpm_vtpm_proxy.c | 30 +--
>> > include/linux/tpm.h | 18 +-
>> > security/keys/trusted-keys/trusted_tpm1.c | 34 ++-
>> > security/keys/trusted-keys/trusted_tpm2.c | 176 ++++++-------
>> > 11 files changed, 482 insertions(+), 521 deletions(-)
>> >
>> > diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
>> > index c9e6e5d097ca..1cb649938c01 100644
>> > --- a/drivers/char/tpm/tpm-buf.c
>> > +++ b/drivers/char/tpm/tpm-buf.c
>> > @@ -7,82 +7,109 @@
>> > #include <linux/module.h>
>> > #include <linux/tpm.h>
>> >
>> > -/**
>> > - * tpm_buf_init() - Allocate and initialize a TPM command
>> > - * @buf: A &tpm_buf
>> > - * @tag: TPM_TAG_RQU_COMMAND, TPM2_ST_NO_SESSIONS or TPM2_ST_SESSIONS
>> > - * @ordinal: A command ordinal
>> > - *
>> > - * Return: 0 or -ENOMEM
>> > - */
>> > -int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal)
>> > +static void __tpm_buf_size_invariant(struct tpm_buf *buf, u16 buf_size)
>> > {
>> > - buf->data = (u8 *)__get_free_page(GFP_KERNEL);
>> > - if (!buf->data)
>> > - return -ENOMEM;
>> > -
>> > - tpm_buf_reset(buf, tag, ordinal);
>> > - return 0;
>> > + u32 buf_size_2 = (u32)buf->capacity + (u32)sizeof(*buf);
>> > +
>> > + if (!buf->capacity) {
>> > + if (buf_size > TPM_BUFSIZE) {
>> > + WARN(1, "%s: size overflow: %u\n", __func__, buf_size);
>> > + buf->flags |= TPM_BUF_INVALID;
>> > + }
>> > + } else {
>> > + if (buf_size != buf_size_2) {
>> > + WARN(1, "%s: size mismatch: %u != %u\n", __func__, buf_size,
>> > + buf_size_2);
>> > + buf->flags |= TPM_BUF_INVALID;
>> > + }
>> > + }
>> > }
>> > -EXPORT_SYMBOL_GPL(tpm_buf_init);
>> >
>> > -/**
>> > - * tpm_buf_reset() - Initialize a TPM command
>> > - * @buf: A &tpm_buf
>> > - * @tag: TPM_TAG_RQU_COMMAND, TPM2_ST_NO_SESSIONS or TPM2_ST_SESSIONS
>> > - * @ordinal: A command ordinal
>> > - */
>> > -void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal)
>> > +static void __tpm_buf_reset(struct tpm_buf *buf, u16 buf_size, u16 tag, u32 ordinal)
>> > {
>> > struct tpm_header *head = (struct tpm_header *)buf->data;
>> >
>> > + __tpm_buf_size_invariant(buf, buf_size);
>> > +
>> > + if (buf->flags & TPM_BUF_INVALID)
>> > + return;
>> > +
>> > WARN_ON(tag != TPM_TAG_RQU_COMMAND && tag != TPM2_ST_NO_SESSIONS &&
>> > tag != TPM2_ST_SESSIONS && tag != 0);
>> >
>> > buf->flags = 0;
>> > buf->length = sizeof(*head);
>> > + buf->capacity = buf_size - sizeof(*buf);
>> > + buf->handles = 0;
>> > head->tag = cpu_to_be16(tag);
>> > head->length = cpu_to_be32(sizeof(*head));
>> > head->ordinal = cpu_to_be32(ordinal);
>> > +}
>> > +
>> > +static void __tpm_buf_reset_sized(struct tpm_buf *buf, u16 buf_size)
>> > +{
>> > + __tpm_buf_size_invariant(buf, buf_size);
>> > +
>> > + if (buf->flags & TPM_BUF_INVALID)
>> > + return;
>> > +
>> > + buf->flags = TPM_BUF_TPM2B;
>> > + buf->length = 2;
>> > + buf->capacity = buf_size - sizeof(*buf);
>> > buf->handles = 0;
>> > + buf->data[0] = 0;
>> > + buf->data[1] = 0;
>> > }
>> > -EXPORT_SYMBOL_GPL(tpm_buf_reset);
>> >
>> > /**
>> > - * tpm_buf_init_sized() - Allocate and initialize a sized (TPM2B) buffer
>> > - * @buf: A @tpm_buf
>> > - *
>> > - * Return: 0 or -ENOMEM
>> > + * tpm_buf_init() - Initialize a TPM command
>> > + * @buf: A &tpm_buf
>> > + * @buf_size: Size of the buffer.
>> > */
>> > -int tpm_buf_init_sized(struct tpm_buf *buf)
>> > +void tpm_buf_init(struct tpm_buf *buf, u16 buf_size)
>> > {
>> > - buf->data = (u8 *)__get_free_page(GFP_KERNEL);
>> > - if (!buf->data)
>> > - return -ENOMEM;
>> > + memset(buf, 0, buf_size);
>> > + __tpm_buf_reset(buf, buf_size, TPM_TAG_RQU_COMMAND, 0);
>> > +}
>> > +EXPORT_SYMBOL_GPL(tpm_buf_init);
>> >
>> > - tpm_buf_reset_sized(buf);
>> > - return 0;
>> > +/**
>> > + * tpm_buf_init_sized() - Initialize a sized buffer
>> > + * @buf: A &tpm_buf
>> > + * @buf_size: Size of the buffer.
>> > + */
>> > +void tpm_buf_init_sized(struct tpm_buf *buf, u16 buf_size)
>> > +{
>> > + memset(buf, 0, buf_size);
>> > + __tpm_buf_reset_sized(buf, buf_size);
>> > }
>> > EXPORT_SYMBOL_GPL(tpm_buf_init_sized);
>> >
>> > /**
>> > - * tpm_buf_reset_sized() - Initialize a sized buffer
>> > + * tpm_buf_reset() - Re-initialize a TPM command
>> > * @buf: A &tpm_buf
>> > + * @tag: TPM_TAG_RQU_COMMAND, TPM2_ST_NO_SESSIONS or TPM2_ST_SESSIONS
>> > + * @ordinal: A command ordinal
>> > */
>> > -void tpm_buf_reset_sized(struct tpm_buf *buf)
>> > +void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal)
>> > {
>> > - buf->flags = TPM_BUF_TPM2B;
>> > - buf->length = 2;
>> > - buf->data[0] = 0;
>> > - buf->data[1] = 0;
>> > + u16 buf_size = buf->capacity + sizeof(*buf);
>> > +
>> > + __tpm_buf_reset(buf, buf_size, tag, ordinal);
>> > }
>> > -EXPORT_SYMBOL_GPL(tpm_buf_reset_sized);
>> > +EXPORT_SYMBOL_GPL(tpm_buf_reset);
>> >
>> > -void tpm_buf_destroy(struct tpm_buf *buf)
>> > +/**
>> > + * tpm_buf_reset_sized() - Re-initialize a sized buffer
>> > + * @buf: A &tpm_buf
>> > + */
>> > +void tpm_buf_reset_sized(struct tpm_buf *buf)
>> > {
>> > - free_page((unsigned long)buf->data);
>> > + u16 buf_size = buf->capacity + sizeof(*buf);
>> > +
>> > + __tpm_buf_reset_sized(buf, buf_size);
>> > }
>> > -EXPORT_SYMBOL_GPL(tpm_buf_destroy);
>> > +EXPORT_SYMBOL_GPL(tpm_buf_reset_sized);
>> >
>> > /**
>> > * tpm_buf_length() - Return the number of bytes consumed by the data
>> > @@ -92,6 +119,9 @@ EXPORT_SYMBOL_GPL(tpm_buf_destroy);
>> > */
>> > u32 tpm_buf_length(struct tpm_buf *buf)
>>
>> Should we update the return value to u16?
>>
>> > {
>> > + if (buf->flags & TPM_BUF_INVALID)
>> > + return 0;
>> > +
>> > return buf->length;
>> > }
>> > EXPORT_SYMBOL_GPL(tpm_buf_length);
>> > @@ -104,10 +134,12 @@ EXPORT_SYMBOL_GPL(tpm_buf_length);
>> > */
>> > void tpm_buf_append(struct tpm_buf *buf, const u8 *new_data, u16 new_length)
>> > {
>> > + u32 total_length = (u32)buf->length + (u32)new_length;
>> > +
>> > if (buf->flags & TPM_BUF_INVALID)
>> > return;
>> >
>> > - if ((buf->length + new_length) > PAGE_SIZE) {
>> > + if (total_length > (u32)buf->capacity) {
>> > WARN(1, "tpm_buf: write overflow\n");
>> > buf->flags |= TPM_BUF_INVALID;
>> > return;
>>
>> [...]
>>
>> > diff --git a/security/keys/trusted-keys/trusted_tpm1.c b/security/keys/trusted-keys/trusted_tpm1.c
>> > index 636acb66a4f6..6e6a9fb48e63 100644
>> > --- a/security/keys/trusted-keys/trusted_tpm1.c
>> > +++ b/security/keys/trusted-keys/trusted_tpm1.c
>> > @@ -310,9 +310,8 @@ static int TSS_checkhmac2(unsigned char *buffer,
>> > * For key specific tpm requests, we will generate and send our
>> > * own TPM command packets using the drivers send function.
>> > */
>> > -static int trusted_tpm_send(unsigned char *cmd, size_t buflen)
>> > +static int trusted_tpm_send(void *cmd, size_t buflen)
>> > {
>> > - struct tpm_buf buf;
>> > int rc;
>> >
>> > if (!chip)
>> > @@ -322,15 +321,12 @@ static int trusted_tpm_send(unsigned char *cmd, size_t buflen)
>> > if (rc)
>> > return rc;
>> >
>> > - buf.flags = 0;
>> > - buf.length = buflen;
>> > - buf.data = cmd;
>> > dump_tpm_buf(cmd);
>> > - rc = tpm_transmit_cmd(chip, &buf, 4, "sending data");
>> > + rc = tpm_transmit_cmd(chip, cmd, 4, "sending data");
>>
>> Is it fine here to remove the intermediate tpm_buf ?
>>
>> IIUC tpm_transmit_cmd() needs a tpm_buf, while here we are passing just
>> the "data", or in some way it's a nested tpm_buf?
>>
>> (Sorry if it's a stupid question, but I'm still a bit new with this code).
>>
>> > dump_tpm_buf(cmd);
>> >
>> > + /* Convert TPM error to -EPERM. */
>> > if (rc > 0)
>> > - /* TPM error */
>> > rc = -EPERM;
>> >
>> > tpm_put_ops(chip);
>> > @@ -624,23 +620,23 @@ static int tpm_unseal(struct tpm_buf *tb,
>> > static int key_seal(struct trusted_key_payload *p,
>> > struct trusted_key_options *o)
>> > {
>> > - struct tpm_buf tb;
>> > int ret;
>> >
>> > - ret = tpm_buf_init(&tb, 0, 0);
>> > - if (ret)
>> > - return ret;
>> > + struct tpm_buf *tb __free(kfree) = kzalloc(PAGE_SIZE, GFP_KERNEL);
>> > + if (!tb)
>> > + return -ENOMEM;
>> > +
>> > + tpm_buf_init(tb, TPM_BUFSIZE);
>> >
>> > /* include migratable flag at end of sealed key */
>> > p->key[p->key_len] = p->migratable;
>> >
>> > - ret = tpm_seal(&tb, o->keytype, o->keyhandle, o->keyauth,
>> > + ret = tpm_seal(tb, o->keytype, o->keyhandle, o->keyauth,
>> > p->key, p->key_len + 1, p->blob, &p->blob_len,
>> > o->blobauth, o->pcrinfo, o->pcrinfo_len);
>> > if (ret < 0)
>> > pr_info("srkseal failed (%d)\n", ret);
>> >
>> > - tpm_buf_destroy(&tb);
>> > return ret;
>> > }
>> >
>> > @@ -650,14 +646,15 @@ static int key_seal(struct trusted_key_payload *p,
>> > static int key_unseal(struct trusted_key_payload *p,
>> > struct trusted_key_options *o)
>> > {
>> > - struct tpm_buf tb;
>> > int ret;
>> >
>> > - ret = tpm_buf_init(&tb, 0, 0);
>> > - if (ret)
>> > - return ret;
>> > + struct tpm_buf *tb __free(kfree) = kzalloc(PAGE_SIZE, GFP_KERNEL);
>> > + if (!tb)
>> > + return -ENOMEM;
>> > +
>> > + tpm_buf_init(tb, TPM_BUFSIZE);
>> >
>> > - ret = tpm_unseal(&tb, o->keyhandle, o->keyauth, p->blob, p->blob_len,
>> > + ret = tpm_unseal(tb, o->keyhandle, o->keyauth, p->blob, p->blob_len,
>> > o->blobauth, p->key, &p->key_len);
>> > if (ret < 0)
>> > pr_info("srkunseal failed (%d)\n", ret);
>> > @@ -665,7 +662,6 @@ static int key_unseal(struct trusted_key_payload *p,
>> > /* pull migratable flag out of sealed key */
>> > p->migratable = p->key[--p->key_len];
>> >
>> > - tpm_buf_destroy(&tb);
>> > return ret;
>> > }
>>
>> The rest LGTM, but it's a huge patch (not your issue at all), so yeah not
>> sure :-)
>
>It's still a single logical change :-)
Yep, and good set of patches to prepare this!
BTW I'm still bit concerned about the changes in trusted_tpm_send()
which I left a few lines above, can you check it?
Thanks,
Stefano
^ permalink raw reply
* Re: [PATCH v3 10/10] tpm-buf: Enable managed and stack allocations.
From: Jarkko Sakkinen @ 2025-09-30 13:11 UTC (permalink / raw)
To: Stefano Garzarella
Cc: linux-integrity, dpsmith, ross.philipson, Jarkko Sakkinen,
Stefan Berger, Peter Huewe, Jason Gunthorpe, David Howells,
Paul Moore, James Morris, Serge E. Hallyn, James Bottomley,
Mimi Zohar, open list, open list:KEYS/KEYRINGS,
open list:SECURITY SUBSYSTEM
In-Reply-To: <u7zay2gb3dff4ptbh34qw7ini63ar3246ivd4xnxtdxc6ijktx@lutatpeg7f7z>
On Tue, Sep 30, 2025 at 02:44:52PM +0200, Stefano Garzarella wrote:
> On Mon, Sep 29, 2025 at 10:48:32PM +0300, Jarkko Sakkinen wrote:
> > From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
> >
> > Decouple kzalloc from buffer creation, so that a managed allocation can be
> > done trivially:
> >
> > struct tpm_buf *buf __free(kfree) buf = kzalloc(TPM_BUFSIZE,
> ^
> In the code, we use PAGE_SIZE instead of TPM_BUFSIZE with kzalloc().
> Should we do the same in this example? (Perhaps adding the reason, if you
> think it would be useful)
I think that should be fixed up in the patch and use TPM_BUFSIZE
consistently for kzallocs. Thanks for the remark.
>
> > GFP_KERNEL);
> > if (!buf)
> > return -ENOMEM;
> > tpm_buf_init(buf, TPM_BUFSIZE);
> >
> > Alternatively, stack allocations are also possible:
> >
> > u8 buf_data[512];
> > struct tpm_buf *buf = (struct tpm_buf *)buf_data;
> > tpm_buf_init(buf, sizeof(buf_data));
> >
> > Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
> > ---
> > v3:
> > - A new patch from the earlier series with more scoped changes and
> > less abstract commit message.
> > ---
> > drivers/char/tpm/tpm-buf.c | 122 +++++----
> > drivers/char/tpm/tpm-sysfs.c | 20 +-
> > drivers/char/tpm/tpm.h | 1 -
> > drivers/char/tpm/tpm1-cmd.c | 147 +++++------
> > drivers/char/tpm/tpm2-cmd.c | 290 ++++++++++------------
> > drivers/char/tpm/tpm2-sessions.c | 121 +++++----
> > drivers/char/tpm/tpm2-space.c | 44 ++--
> > drivers/char/tpm/tpm_vtpm_proxy.c | 30 +--
> > include/linux/tpm.h | 18 +-
> > security/keys/trusted-keys/trusted_tpm1.c | 34 ++-
> > security/keys/trusted-keys/trusted_tpm2.c | 176 ++++++-------
> > 11 files changed, 482 insertions(+), 521 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
> > index c9e6e5d097ca..1cb649938c01 100644
> > --- a/drivers/char/tpm/tpm-buf.c
> > +++ b/drivers/char/tpm/tpm-buf.c
> > @@ -7,82 +7,109 @@
> > #include <linux/module.h>
> > #include <linux/tpm.h>
> >
> > -/**
> > - * tpm_buf_init() - Allocate and initialize a TPM command
> > - * @buf: A &tpm_buf
> > - * @tag: TPM_TAG_RQU_COMMAND, TPM2_ST_NO_SESSIONS or TPM2_ST_SESSIONS
> > - * @ordinal: A command ordinal
> > - *
> > - * Return: 0 or -ENOMEM
> > - */
> > -int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal)
> > +static void __tpm_buf_size_invariant(struct tpm_buf *buf, u16 buf_size)
> > {
> > - buf->data = (u8 *)__get_free_page(GFP_KERNEL);
> > - if (!buf->data)
> > - return -ENOMEM;
> > -
> > - tpm_buf_reset(buf, tag, ordinal);
> > - return 0;
> > + u32 buf_size_2 = (u32)buf->capacity + (u32)sizeof(*buf);
> > +
> > + if (!buf->capacity) {
> > + if (buf_size > TPM_BUFSIZE) {
> > + WARN(1, "%s: size overflow: %u\n", __func__, buf_size);
> > + buf->flags |= TPM_BUF_INVALID;
> > + }
> > + } else {
> > + if (buf_size != buf_size_2) {
> > + WARN(1, "%s: size mismatch: %u != %u\n", __func__, buf_size,
> > + buf_size_2);
> > + buf->flags |= TPM_BUF_INVALID;
> > + }
> > + }
> > }
> > -EXPORT_SYMBOL_GPL(tpm_buf_init);
> >
> > -/**
> > - * tpm_buf_reset() - Initialize a TPM command
> > - * @buf: A &tpm_buf
> > - * @tag: TPM_TAG_RQU_COMMAND, TPM2_ST_NO_SESSIONS or TPM2_ST_SESSIONS
> > - * @ordinal: A command ordinal
> > - */
> > -void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal)
> > +static void __tpm_buf_reset(struct tpm_buf *buf, u16 buf_size, u16 tag, u32 ordinal)
> > {
> > struct tpm_header *head = (struct tpm_header *)buf->data;
> >
> > + __tpm_buf_size_invariant(buf, buf_size);
> > +
> > + if (buf->flags & TPM_BUF_INVALID)
> > + return;
> > +
> > WARN_ON(tag != TPM_TAG_RQU_COMMAND && tag != TPM2_ST_NO_SESSIONS &&
> > tag != TPM2_ST_SESSIONS && tag != 0);
> >
> > buf->flags = 0;
> > buf->length = sizeof(*head);
> > + buf->capacity = buf_size - sizeof(*buf);
> > + buf->handles = 0;
> > head->tag = cpu_to_be16(tag);
> > head->length = cpu_to_be32(sizeof(*head));
> > head->ordinal = cpu_to_be32(ordinal);
> > +}
> > +
> > +static void __tpm_buf_reset_sized(struct tpm_buf *buf, u16 buf_size)
> > +{
> > + __tpm_buf_size_invariant(buf, buf_size);
> > +
> > + if (buf->flags & TPM_BUF_INVALID)
> > + return;
> > +
> > + buf->flags = TPM_BUF_TPM2B;
> > + buf->length = 2;
> > + buf->capacity = buf_size - sizeof(*buf);
> > buf->handles = 0;
> > + buf->data[0] = 0;
> > + buf->data[1] = 0;
> > }
> > -EXPORT_SYMBOL_GPL(tpm_buf_reset);
> >
> > /**
> > - * tpm_buf_init_sized() - Allocate and initialize a sized (TPM2B) buffer
> > - * @buf: A @tpm_buf
> > - *
> > - * Return: 0 or -ENOMEM
> > + * tpm_buf_init() - Initialize a TPM command
> > + * @buf: A &tpm_buf
> > + * @buf_size: Size of the buffer.
> > */
> > -int tpm_buf_init_sized(struct tpm_buf *buf)
> > +void tpm_buf_init(struct tpm_buf *buf, u16 buf_size)
> > {
> > - buf->data = (u8 *)__get_free_page(GFP_KERNEL);
> > - if (!buf->data)
> > - return -ENOMEM;
> > + memset(buf, 0, buf_size);
> > + __tpm_buf_reset(buf, buf_size, TPM_TAG_RQU_COMMAND, 0);
> > +}
> > +EXPORT_SYMBOL_GPL(tpm_buf_init);
> >
> > - tpm_buf_reset_sized(buf);
> > - return 0;
> > +/**
> > + * tpm_buf_init_sized() - Initialize a sized buffer
> > + * @buf: A &tpm_buf
> > + * @buf_size: Size of the buffer.
> > + */
> > +void tpm_buf_init_sized(struct tpm_buf *buf, u16 buf_size)
> > +{
> > + memset(buf, 0, buf_size);
> > + __tpm_buf_reset_sized(buf, buf_size);
> > }
> > EXPORT_SYMBOL_GPL(tpm_buf_init_sized);
> >
> > /**
> > - * tpm_buf_reset_sized() - Initialize a sized buffer
> > + * tpm_buf_reset() - Re-initialize a TPM command
> > * @buf: A &tpm_buf
> > + * @tag: TPM_TAG_RQU_COMMAND, TPM2_ST_NO_SESSIONS or TPM2_ST_SESSIONS
> > + * @ordinal: A command ordinal
> > */
> > -void tpm_buf_reset_sized(struct tpm_buf *buf)
> > +void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal)
> > {
> > - buf->flags = TPM_BUF_TPM2B;
> > - buf->length = 2;
> > - buf->data[0] = 0;
> > - buf->data[1] = 0;
> > + u16 buf_size = buf->capacity + sizeof(*buf);
> > +
> > + __tpm_buf_reset(buf, buf_size, tag, ordinal);
> > }
> > -EXPORT_SYMBOL_GPL(tpm_buf_reset_sized);
> > +EXPORT_SYMBOL_GPL(tpm_buf_reset);
> >
> > -void tpm_buf_destroy(struct tpm_buf *buf)
> > +/**
> > + * tpm_buf_reset_sized() - Re-initialize a sized buffer
> > + * @buf: A &tpm_buf
> > + */
> > +void tpm_buf_reset_sized(struct tpm_buf *buf)
> > {
> > - free_page((unsigned long)buf->data);
> > + u16 buf_size = buf->capacity + sizeof(*buf);
> > +
> > + __tpm_buf_reset_sized(buf, buf_size);
> > }
> > -EXPORT_SYMBOL_GPL(tpm_buf_destroy);
> > +EXPORT_SYMBOL_GPL(tpm_buf_reset_sized);
> >
> > /**
> > * tpm_buf_length() - Return the number of bytes consumed by the data
> > @@ -92,6 +119,9 @@ EXPORT_SYMBOL_GPL(tpm_buf_destroy);
> > */
> > u32 tpm_buf_length(struct tpm_buf *buf)
>
> Should we update the return value to u16?
>
> > {
> > + if (buf->flags & TPM_BUF_INVALID)
> > + return 0;
> > +
> > return buf->length;
> > }
> > EXPORT_SYMBOL_GPL(tpm_buf_length);
> > @@ -104,10 +134,12 @@ EXPORT_SYMBOL_GPL(tpm_buf_length);
> > */
> > void tpm_buf_append(struct tpm_buf *buf, const u8 *new_data, u16 new_length)
> > {
> > + u32 total_length = (u32)buf->length + (u32)new_length;
> > +
> > if (buf->flags & TPM_BUF_INVALID)
> > return;
> >
> > - if ((buf->length + new_length) > PAGE_SIZE) {
> > + if (total_length > (u32)buf->capacity) {
> > WARN(1, "tpm_buf: write overflow\n");
> > buf->flags |= TPM_BUF_INVALID;
> > return;
>
> [...]
>
> > diff --git a/security/keys/trusted-keys/trusted_tpm1.c b/security/keys/trusted-keys/trusted_tpm1.c
> > index 636acb66a4f6..6e6a9fb48e63 100644
> > --- a/security/keys/trusted-keys/trusted_tpm1.c
> > +++ b/security/keys/trusted-keys/trusted_tpm1.c
> > @@ -310,9 +310,8 @@ static int TSS_checkhmac2(unsigned char *buffer,
> > * For key specific tpm requests, we will generate and send our
> > * own TPM command packets using the drivers send function.
> > */
> > -static int trusted_tpm_send(unsigned char *cmd, size_t buflen)
> > +static int trusted_tpm_send(void *cmd, size_t buflen)
> > {
> > - struct tpm_buf buf;
> > int rc;
> >
> > if (!chip)
> > @@ -322,15 +321,12 @@ static int trusted_tpm_send(unsigned char *cmd, size_t buflen)
> > if (rc)
> > return rc;
> >
> > - buf.flags = 0;
> > - buf.length = buflen;
> > - buf.data = cmd;
> > dump_tpm_buf(cmd);
> > - rc = tpm_transmit_cmd(chip, &buf, 4, "sending data");
> > + rc = tpm_transmit_cmd(chip, cmd, 4, "sending data");
>
> Is it fine here to remove the intermediate tpm_buf ?
>
> IIUC tpm_transmit_cmd() needs a tpm_buf, while here we are passing just
> the "data", or in some way it's a nested tpm_buf?
>
> (Sorry if it's a stupid question, but I'm still a bit new with this code).
>
> > dump_tpm_buf(cmd);
> >
> > + /* Convert TPM error to -EPERM. */
> > if (rc > 0)
> > - /* TPM error */
> > rc = -EPERM;
> >
> > tpm_put_ops(chip);
> > @@ -624,23 +620,23 @@ static int tpm_unseal(struct tpm_buf *tb,
> > static int key_seal(struct trusted_key_payload *p,
> > struct trusted_key_options *o)
> > {
> > - struct tpm_buf tb;
> > int ret;
> >
> > - ret = tpm_buf_init(&tb, 0, 0);
> > - if (ret)
> > - return ret;
> > + struct tpm_buf *tb __free(kfree) = kzalloc(PAGE_SIZE, GFP_KERNEL);
> > + if (!tb)
> > + return -ENOMEM;
> > +
> > + tpm_buf_init(tb, TPM_BUFSIZE);
> >
> > /* include migratable flag at end of sealed key */
> > p->key[p->key_len] = p->migratable;
> >
> > - ret = tpm_seal(&tb, o->keytype, o->keyhandle, o->keyauth,
> > + ret = tpm_seal(tb, o->keytype, o->keyhandle, o->keyauth,
> > p->key, p->key_len + 1, p->blob, &p->blob_len,
> > o->blobauth, o->pcrinfo, o->pcrinfo_len);
> > if (ret < 0)
> > pr_info("srkseal failed (%d)\n", ret);
> >
> > - tpm_buf_destroy(&tb);
> > return ret;
> > }
> >
> > @@ -650,14 +646,15 @@ static int key_seal(struct trusted_key_payload *p,
> > static int key_unseal(struct trusted_key_payload *p,
> > struct trusted_key_options *o)
> > {
> > - struct tpm_buf tb;
> > int ret;
> >
> > - ret = tpm_buf_init(&tb, 0, 0);
> > - if (ret)
> > - return ret;
> > + struct tpm_buf *tb __free(kfree) = kzalloc(PAGE_SIZE, GFP_KERNEL);
> > + if (!tb)
> > + return -ENOMEM;
> > +
> > + tpm_buf_init(tb, TPM_BUFSIZE);
> >
> > - ret = tpm_unseal(&tb, o->keyhandle, o->keyauth, p->blob, p->blob_len,
> > + ret = tpm_unseal(tb, o->keyhandle, o->keyauth, p->blob, p->blob_len,
> > o->blobauth, p->key, &p->key_len);
> > if (ret < 0)
> > pr_info("srkunseal failed (%d)\n", ret);
> > @@ -665,7 +662,6 @@ static int key_unseal(struct trusted_key_payload *p,
> > /* pull migratable flag out of sealed key */
> > p->migratable = p->key[--p->key_len];
> >
> > - tpm_buf_destroy(&tb);
> > return ret;
> > }
>
> The rest LGTM, but it's a huge patch (not your issue at all), so yeah not
> sure :-)
It's still a single logical change :-)
>
> Thanks,
> Stefano
>
BR, Jarkko
^ permalink raw reply
* Re: [PATCH v3 10/10] tpm-buf: Enable managed and stack allocations.
From: Stefano Garzarella @ 2025-09-30 12:44 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: linux-integrity, dpsmith, ross.philipson, Jarkko Sakkinen,
Stefan Berger, Peter Huewe, Jason Gunthorpe, David Howells,
Paul Moore, James Morris, Serge E. Hallyn, James Bottomley,
Mimi Zohar, open list, open list:KEYS/KEYRINGS,
open list:SECURITY SUBSYSTEM
In-Reply-To: <20250929194832.2913286-11-jarkko@kernel.org>
On Mon, Sep 29, 2025 at 10:48:32PM +0300, Jarkko Sakkinen wrote:
>From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
>
>Decouple kzalloc from buffer creation, so that a managed allocation can be
>done trivially:
>
> struct tpm_buf *buf __free(kfree) buf = kzalloc(TPM_BUFSIZE,
^
In the code, we use PAGE_SIZE instead of TPM_BUFSIZE with kzalloc().
Should we do the same in this example? (Perhaps adding the reason, if
you think it would be useful)
> GFP_KERNEL);
> if (!buf)
> return -ENOMEM;
> tpm_buf_init(buf, TPM_BUFSIZE);
>
>Alternatively, stack allocations are also possible:
>
> u8 buf_data[512];
> struct tpm_buf *buf = (struct tpm_buf *)buf_data;
> tpm_buf_init(buf, sizeof(buf_data));
>
>Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
>Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
>---
>v3:
>- A new patch from the earlier series with more scoped changes and
> less abstract commit message.
>---
> drivers/char/tpm/tpm-buf.c | 122 +++++----
> drivers/char/tpm/tpm-sysfs.c | 20 +-
> drivers/char/tpm/tpm.h | 1 -
> drivers/char/tpm/tpm1-cmd.c | 147 +++++------
> drivers/char/tpm/tpm2-cmd.c | 290 ++++++++++------------
> drivers/char/tpm/tpm2-sessions.c | 121 +++++----
> drivers/char/tpm/tpm2-space.c | 44 ++--
> drivers/char/tpm/tpm_vtpm_proxy.c | 30 +--
> include/linux/tpm.h | 18 +-
> security/keys/trusted-keys/trusted_tpm1.c | 34 ++-
> security/keys/trusted-keys/trusted_tpm2.c | 176 ++++++-------
> 11 files changed, 482 insertions(+), 521 deletions(-)
>
>diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
>index c9e6e5d097ca..1cb649938c01 100644
>--- a/drivers/char/tpm/tpm-buf.c
>+++ b/drivers/char/tpm/tpm-buf.c
>@@ -7,82 +7,109 @@
> #include <linux/module.h>
> #include <linux/tpm.h>
>
>-/**
>- * tpm_buf_init() - Allocate and initialize a TPM command
>- * @buf: A &tpm_buf
>- * @tag: TPM_TAG_RQU_COMMAND, TPM2_ST_NO_SESSIONS or TPM2_ST_SESSIONS
>- * @ordinal: A command ordinal
>- *
>- * Return: 0 or -ENOMEM
>- */
>-int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal)
>+static void __tpm_buf_size_invariant(struct tpm_buf *buf, u16 buf_size)
> {
>- buf->data = (u8 *)__get_free_page(GFP_KERNEL);
>- if (!buf->data)
>- return -ENOMEM;
>-
>- tpm_buf_reset(buf, tag, ordinal);
>- return 0;
>+ u32 buf_size_2 = (u32)buf->capacity + (u32)sizeof(*buf);
>+
>+ if (!buf->capacity) {
>+ if (buf_size > TPM_BUFSIZE) {
>+ WARN(1, "%s: size overflow: %u\n", __func__, buf_size);
>+ buf->flags |= TPM_BUF_INVALID;
>+ }
>+ } else {
>+ if (buf_size != buf_size_2) {
>+ WARN(1, "%s: size mismatch: %u != %u\n", __func__, buf_size,
>+ buf_size_2);
>+ buf->flags |= TPM_BUF_INVALID;
>+ }
>+ }
> }
>-EXPORT_SYMBOL_GPL(tpm_buf_init);
>
>-/**
>- * tpm_buf_reset() - Initialize a TPM command
>- * @buf: A &tpm_buf
>- * @tag: TPM_TAG_RQU_COMMAND, TPM2_ST_NO_SESSIONS or TPM2_ST_SESSIONS
>- * @ordinal: A command ordinal
>- */
>-void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal)
>+static void __tpm_buf_reset(struct tpm_buf *buf, u16 buf_size, u16 tag, u32 ordinal)
> {
> struct tpm_header *head = (struct tpm_header *)buf->data;
>
>+ __tpm_buf_size_invariant(buf, buf_size);
>+
>+ if (buf->flags & TPM_BUF_INVALID)
>+ return;
>+
> WARN_ON(tag != TPM_TAG_RQU_COMMAND && tag != TPM2_ST_NO_SESSIONS &&
> tag != TPM2_ST_SESSIONS && tag != 0);
>
> buf->flags = 0;
> buf->length = sizeof(*head);
>+ buf->capacity = buf_size - sizeof(*buf);
>+ buf->handles = 0;
> head->tag = cpu_to_be16(tag);
> head->length = cpu_to_be32(sizeof(*head));
> head->ordinal = cpu_to_be32(ordinal);
>+}
>+
>+static void __tpm_buf_reset_sized(struct tpm_buf *buf, u16 buf_size)
>+{
>+ __tpm_buf_size_invariant(buf, buf_size);
>+
>+ if (buf->flags & TPM_BUF_INVALID)
>+ return;
>+
>+ buf->flags = TPM_BUF_TPM2B;
>+ buf->length = 2;
>+ buf->capacity = buf_size - sizeof(*buf);
> buf->handles = 0;
>+ buf->data[0] = 0;
>+ buf->data[1] = 0;
> }
>-EXPORT_SYMBOL_GPL(tpm_buf_reset);
>
> /**
>- * tpm_buf_init_sized() - Allocate and initialize a sized (TPM2B) buffer
>- * @buf: A @tpm_buf
>- *
>- * Return: 0 or -ENOMEM
>+ * tpm_buf_init() - Initialize a TPM command
>+ * @buf: A &tpm_buf
>+ * @buf_size: Size of the buffer.
> */
>-int tpm_buf_init_sized(struct tpm_buf *buf)
>+void tpm_buf_init(struct tpm_buf *buf, u16 buf_size)
> {
>- buf->data = (u8 *)__get_free_page(GFP_KERNEL);
>- if (!buf->data)
>- return -ENOMEM;
>+ memset(buf, 0, buf_size);
>+ __tpm_buf_reset(buf, buf_size, TPM_TAG_RQU_COMMAND, 0);
>+}
>+EXPORT_SYMBOL_GPL(tpm_buf_init);
>
>- tpm_buf_reset_sized(buf);
>- return 0;
>+/**
>+ * tpm_buf_init_sized() - Initialize a sized buffer
>+ * @buf: A &tpm_buf
>+ * @buf_size: Size of the buffer.
>+ */
>+void tpm_buf_init_sized(struct tpm_buf *buf, u16 buf_size)
>+{
>+ memset(buf, 0, buf_size);
>+ __tpm_buf_reset_sized(buf, buf_size);
> }
> EXPORT_SYMBOL_GPL(tpm_buf_init_sized);
>
> /**
>- * tpm_buf_reset_sized() - Initialize a sized buffer
>+ * tpm_buf_reset() - Re-initialize a TPM command
> * @buf: A &tpm_buf
>+ * @tag: TPM_TAG_RQU_COMMAND, TPM2_ST_NO_SESSIONS or TPM2_ST_SESSIONS
>+ * @ordinal: A command ordinal
> */
>-void tpm_buf_reset_sized(struct tpm_buf *buf)
>+void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal)
> {
>- buf->flags = TPM_BUF_TPM2B;
>- buf->length = 2;
>- buf->data[0] = 0;
>- buf->data[1] = 0;
>+ u16 buf_size = buf->capacity + sizeof(*buf);
>+
>+ __tpm_buf_reset(buf, buf_size, tag, ordinal);
> }
>-EXPORT_SYMBOL_GPL(tpm_buf_reset_sized);
>+EXPORT_SYMBOL_GPL(tpm_buf_reset);
>
>-void tpm_buf_destroy(struct tpm_buf *buf)
>+/**
>+ * tpm_buf_reset_sized() - Re-initialize a sized buffer
>+ * @buf: A &tpm_buf
>+ */
>+void tpm_buf_reset_sized(struct tpm_buf *buf)
> {
>- free_page((unsigned long)buf->data);
>+ u16 buf_size = buf->capacity + sizeof(*buf);
>+
>+ __tpm_buf_reset_sized(buf, buf_size);
> }
>-EXPORT_SYMBOL_GPL(tpm_buf_destroy);
>+EXPORT_SYMBOL_GPL(tpm_buf_reset_sized);
>
> /**
> * tpm_buf_length() - Return the number of bytes consumed by the data
>@@ -92,6 +119,9 @@ EXPORT_SYMBOL_GPL(tpm_buf_destroy);
> */
> u32 tpm_buf_length(struct tpm_buf *buf)
Should we update the return value to u16?
> {
>+ if (buf->flags & TPM_BUF_INVALID)
>+ return 0;
>+
> return buf->length;
> }
> EXPORT_SYMBOL_GPL(tpm_buf_length);
>@@ -104,10 +134,12 @@ EXPORT_SYMBOL_GPL(tpm_buf_length);
> */
> void tpm_buf_append(struct tpm_buf *buf, const u8 *new_data, u16 new_length)
> {
>+ u32 total_length = (u32)buf->length + (u32)new_length;
>+
> if (buf->flags & TPM_BUF_INVALID)
> return;
>
>- if ((buf->length + new_length) > PAGE_SIZE) {
>+ if (total_length > (u32)buf->capacity) {
> WARN(1, "tpm_buf: write overflow\n");
> buf->flags |= TPM_BUF_INVALID;
> return;
[...]
>diff --git a/security/keys/trusted-keys/trusted_tpm1.c b/security/keys/trusted-keys/trusted_tpm1.c
>index 636acb66a4f6..6e6a9fb48e63 100644
>--- a/security/keys/trusted-keys/trusted_tpm1.c
>+++ b/security/keys/trusted-keys/trusted_tpm1.c
>@@ -310,9 +310,8 @@ static int TSS_checkhmac2(unsigned char *buffer,
> * For key specific tpm requests, we will generate and send our
> * own TPM command packets using the drivers send function.
> */
>-static int trusted_tpm_send(unsigned char *cmd, size_t buflen)
>+static int trusted_tpm_send(void *cmd, size_t buflen)
> {
>- struct tpm_buf buf;
> int rc;
>
> if (!chip)
>@@ -322,15 +321,12 @@ static int trusted_tpm_send(unsigned char *cmd, size_t buflen)
> if (rc)
> return rc;
>
>- buf.flags = 0;
>- buf.length = buflen;
>- buf.data = cmd;
> dump_tpm_buf(cmd);
>- rc = tpm_transmit_cmd(chip, &buf, 4, "sending data");
>+ rc = tpm_transmit_cmd(chip, cmd, 4, "sending data");
Is it fine here to remove the intermediate tpm_buf ?
IIUC tpm_transmit_cmd() needs a tpm_buf, while here we are passing just
the "data", or in some way it's a nested tpm_buf?
(Sorry if it's a stupid question, but I'm still a bit new with this
code).
> dump_tpm_buf(cmd);
>
>+ /* Convert TPM error to -EPERM. */
> if (rc > 0)
>- /* TPM error */
> rc = -EPERM;
>
> tpm_put_ops(chip);
>@@ -624,23 +620,23 @@ static int tpm_unseal(struct tpm_buf *tb,
> static int key_seal(struct trusted_key_payload *p,
> struct trusted_key_options *o)
> {
>- struct tpm_buf tb;
> int ret;
>
>- ret = tpm_buf_init(&tb, 0, 0);
>- if (ret)
>- return ret;
>+ struct tpm_buf *tb __free(kfree) = kzalloc(PAGE_SIZE, GFP_KERNEL);
>+ if (!tb)
>+ return -ENOMEM;
>+
>+ tpm_buf_init(tb, TPM_BUFSIZE);
>
> /* include migratable flag at end of sealed key */
> p->key[p->key_len] = p->migratable;
>
>- ret = tpm_seal(&tb, o->keytype, o->keyhandle, o->keyauth,
>+ ret = tpm_seal(tb, o->keytype, o->keyhandle, o->keyauth,
> p->key, p->key_len + 1, p->blob, &p->blob_len,
> o->blobauth, o->pcrinfo, o->pcrinfo_len);
> if (ret < 0)
> pr_info("srkseal failed (%d)\n", ret);
>
>- tpm_buf_destroy(&tb);
> return ret;
> }
>
>@@ -650,14 +646,15 @@ static int key_seal(struct trusted_key_payload *p,
> static int key_unseal(struct trusted_key_payload *p,
> struct trusted_key_options *o)
> {
>- struct tpm_buf tb;
> int ret;
>
>- ret = tpm_buf_init(&tb, 0, 0);
>- if (ret)
>- return ret;
>+ struct tpm_buf *tb __free(kfree) = kzalloc(PAGE_SIZE,
>GFP_KERNEL);
>+ if (!tb)
>+ return -ENOMEM;
>+
>+ tpm_buf_init(tb, TPM_BUFSIZE);
>
>- ret = tpm_unseal(&tb, o->keyhandle, o->keyauth, p->blob, p->blob_len,
>+ ret = tpm_unseal(tb, o->keyhandle, o->keyauth, p->blob, p->blob_len,
> o->blobauth, p->key, &p->key_len);
> if (ret < 0)
> pr_info("srkunseal failed (%d)\n", ret);
>@@ -665,7 +662,6 @@ static int key_unseal(struct trusted_key_payload *p,
> /* pull migratable flag out of sealed key */
> p->migratable = p->key[--p->key_len];
>
>- tpm_buf_destroy(&tb);
> return ret;
> }
The rest LGTM, but it's a huge patch (not your issue at all), so yeah
not sure :-)
Thanks,
Stefano
^ permalink raw reply
* Re: [PATCH v3 07/10] tpm-buf: check for corruption in tpm_buf_append_handle()
From: Jarkko Sakkinen @ 2025-09-30 12:43 UTC (permalink / raw)
To: Jonathan McDowell
Cc: linux-integrity, dpsmith, ross.philipson, Jarkko Sakkinen,
Peter Huewe, Jason Gunthorpe, David Howells, Paul Moore,
James Morris, Serge E. Hallyn, James Bottomley, Mimi Zohar,
open list, open list:KEYS/KEYRINGS, open list:SECURITY SUBSYSTEM
In-Reply-To: <aNu7R8J8h8Kmon0H@earth.li>
On Tue, Sep 30, 2025 at 12:13:11PM +0100, Jonathan McDowell wrote:
> On Mon, Sep 29, 2025 at 10:48:29PM +0300, Jarkko Sakkinen wrote:
> > From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
> >
> > Unify TPM_BUF_BOUNDARY_ERROR and TPM_BUF_OVERFLOW into TPM_BUF_INVALID
> > flag because semantically they are identical.
> >
> > Test and set TPM_BUF_INVALID in tpm_buf_append_handle() following the
> > pattern from other functions in tpm-buf.c.
> >
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
>
> Reviewed-by: Jonathan McDowell <noodles@meta.com>
Thanks.
>
> > ---
> > v3:
> > - No changes.
> > v2:
> > - A new patch.
> > ---
> > drivers/char/tpm/tpm-buf.c | 14 ++++++++------
> > include/linux/tpm.h | 8 +++-----
> > security/keys/trusted-keys/trusted_tpm2.c | 6 +++---
> > 3 files changed, 14 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
> > index dc882fc9fa9e..69ee77400539 100644
> > --- a/drivers/char/tpm/tpm-buf.c
> > +++ b/drivers/char/tpm/tpm-buf.c
> > @@ -104,13 +104,12 @@ EXPORT_SYMBOL_GPL(tpm_buf_length);
> > */
> > void tpm_buf_append(struct tpm_buf *buf, const u8 *new_data, u16 new_length)
> > {
> > - /* Return silently if overflow has already happened. */
> > - if (buf->flags & TPM_BUF_OVERFLOW)
> > + if (buf->flags & TPM_BUF_INVALID)
> > return;
> >
> > if ((buf->length + new_length) > PAGE_SIZE) {
> > WARN(1, "tpm_buf: write overflow\n");
> > - buf->flags |= TPM_BUF_OVERFLOW;
> > + buf->flags |= TPM_BUF_INVALID;
> > return;
> > }
> >
> > @@ -157,8 +156,12 @@ EXPORT_SYMBOL_GPL(tpm_buf_append_u32);
> > */
> > void tpm_buf_append_handle(struct tpm_chip *chip, struct tpm_buf *buf, u32 handle)
> > {
> > + if (buf->flags & TPM_BUF_INVALID)
> > + return;
> > +
> > if (buf->flags & TPM_BUF_TPM2B) {
> > dev_err(&chip->dev, "Invalid buffer type (TPM2B)\n");
> > + buf->flags |= TPM_BUF_INVALID;
> > return;
> > }
> >
> > @@ -177,14 +180,13 @@ static void tpm_buf_read(struct tpm_buf *buf, off_t *offset, size_t count, void
> > {
> > off_t next_offset;
> >
> > - /* Return silently if overflow has already happened. */
> > - if (buf->flags & TPM_BUF_BOUNDARY_ERROR)
> > + if (buf->flags & TPM_BUF_INVALID)
> > return;
> >
> > next_offset = *offset + count;
> > if (next_offset > buf->length) {
> > WARN(1, "tpm_buf: read out of boundary\n");
> > - buf->flags |= TPM_BUF_BOUNDARY_ERROR;
> > + buf->flags |= TPM_BUF_INVALID;
> > return;
> > }
> >
> > diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> > index e72e7657faa2..5283f32781c4 100644
> > --- a/include/linux/tpm.h
> > +++ b/include/linux/tpm.h
> > @@ -366,12 +366,10 @@ struct tpm_header {
> > } __packed;
> >
> > enum tpm_buf_flags {
> > - /* the capacity exceeded: */
> > - TPM_BUF_OVERFLOW = BIT(0),
> > /* TPM2B format: */
> > - TPM_BUF_TPM2B = BIT(1),
> > - /* read out of boundary: */
> > - TPM_BUF_BOUNDARY_ERROR = BIT(2),
> > + TPM_BUF_TPM2B = BIT(0),
> > + /* The buffer is in invalid and unusable state: */
> > + TPM_BUF_INVALID = BIT(1),
> > };
> >
> > /*
> > diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
> > index 8e3b283a59b2..119d5152c0db 100644
> > --- a/security/keys/trusted-keys/trusted_tpm2.c
> > +++ b/security/keys/trusted-keys/trusted_tpm2.c
> > @@ -295,7 +295,7 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
> > /* creation PCR */
> > tpm_buf_append_u32(&buf, 0);
> >
> > - if (buf.flags & TPM_BUF_OVERFLOW) {
> > + if (buf.flags & TPM_BUF_INVALID) {
> > rc = -E2BIG;
> > tpm2_end_auth_session(chip);
> > goto out;
> > @@ -308,7 +308,7 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
> > goto out;
> >
> > blob_len = tpm_buf_read_u32(&buf, &offset);
> > - if (blob_len > MAX_BLOB_SIZE || buf.flags & TPM_BUF_BOUNDARY_ERROR) {
> > + if (blob_len > MAX_BLOB_SIZE || buf.flags & TPM_BUF_INVALID) {
> > rc = -E2BIG;
> > goto out;
> > }
> > @@ -414,7 +414,7 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
> >
> > tpm_buf_append(&buf, blob, blob_len);
> >
> > - if (buf.flags & TPM_BUF_OVERFLOW) {
> > + if (buf.flags & TPM_BUF_INVALID) {
> > rc = -E2BIG;
> > tpm2_end_auth_session(chip);
> > goto out;
> > --
> > 2.39.5
> >
> >
>
> J.
>
> --
> Web [ Reality is for people with no grasp of fantasy. ]
> site: https:// [ ] Made by
> www.earth.li/~noodles/ [ ] HuggieTag 0.0.24
BR, Jarkko
^ permalink raw reply
* Re: [PATCH v3 05/10] tpm2-sessions: Umask tpm_buf_append_hmac_session()
From: Jarkko Sakkinen @ 2025-09-30 12:41 UTC (permalink / raw)
To: Jonathan McDowell
Cc: linux-integrity, dpsmith, ross.philipson, Jarkko Sakkinen,
Peter Huewe, Jason Gunthorpe, David Howells, Paul Moore,
James Morris, Serge E. Hallyn, James Bottomley, Mimi Zohar,
open list, open list:KEYS/KEYRINGS, open list:SECURITY SUBSYSTEM
In-Reply-To: <aNu6834tzirFzKM7@earth.li>
On Tue, Sep 30, 2025 at 12:11:47PM +0100, Jonathan McDowell wrote:
> On Mon, Sep 29, 2025 at 10:48:27PM +0300, Jarkko Sakkinen wrote:
> > From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
> >
> > Open code tpm_buf_append_hmac_session_opt() in order to unmask the code
> > paths in the call sites of tpm_buf_append_hmac_session().
> >
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
>
> Reviewed-by: Jonathan McDowell <noodles@meta.com>
Thanks (short summary has a typo tho)
>
> > ---
> > v3:
> > - No changes.
> > v2:
> > - Uncorrupt the patch.
> > ---
> > drivers/char/tpm/tpm2-cmd.c | 14 +++++++++++---
> > include/linux/tpm.h | 23 -----------------------
> > security/keys/trusted-keys/trusted_tpm2.c | 12 ++++++++++--
> > 3 files changed, 21 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> > index 86b1a4d859b9..c7bfa705ea8f 100644
> > --- a/drivers/char/tpm/tpm2-cmd.c
> > +++ b/drivers/char/tpm/tpm2-cmd.c
> > @@ -257,9 +257,17 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max)
> >
> > do {
> > tpm_buf_reset(&buf, TPM2_ST_SESSIONS, TPM2_CC_GET_RANDOM);
> > - tpm_buf_append_hmac_session_opt(chip, &buf, TPM2_SA_ENCRYPT
> > - | TPM2_SA_CONTINUE_SESSION,
> > - NULL, 0);
> > + if (tpm2_chip_auth(chip)) {
> > + tpm_buf_append_hmac_session(chip, &buf,
> > + TPM2_SA_ENCRYPT |
> > + TPM2_SA_CONTINUE_SESSION,
> > + NULL, 0);
> > + } else {
> > + offset = buf.handles * 4 + TPM_HEADER_SIZE;
> > + head = (struct tpm_header *)buf.data;
> > + if (tpm_buf_length(&buf) == offset)
> > + head->tag = cpu_to_be16(TPM2_ST_NO_SESSIONS);
> > + }
> > tpm_buf_append_u16(&buf, num_bytes);
> > tpm_buf_fill_hmac_session(chip, &buf);
> > err = tpm_transmit_cmd(chip, &buf,
> > diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> > index 1fa02e18e688..e72e7657faa2 100644
> > --- a/include/linux/tpm.h
> > +++ b/include/linux/tpm.h
> > @@ -532,29 +532,6 @@ void tpm_buf_append_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf,
> > int passphraselen);
> > void tpm_buf_append_auth(struct tpm_chip *chip, struct tpm_buf *buf,
> > u8 *passphrase, int passphraselen);
> > -static inline void tpm_buf_append_hmac_session_opt(struct tpm_chip *chip,
> > - struct tpm_buf *buf,
> > - u8 attributes,
> > - u8 *passphrase,
> > - int passphraselen)
> > -{
> > - struct tpm_header *head;
> > - int offset;
> > -
> > - if (tpm2_chip_auth(chip)) {
> > - tpm_buf_append_hmac_session(chip, buf, attributes, passphrase, passphraselen);
> > - } else {
> > - offset = buf->handles * 4 + TPM_HEADER_SIZE;
> > - head = (struct tpm_header *)buf->data;
> > -
> > - /*
> > - * If the only sessions are optional, the command tag must change to
> > - * TPM2_ST_NO_SESSIONS.
> > - */
> > - if (tpm_buf_length(buf) == offset)
> > - head->tag = cpu_to_be16(TPM2_ST_NO_SESSIONS);
> > - }
> > -}
> >
> > #ifdef CONFIG_TCG_TPM2_HMAC
> >
> > diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
> > index e165b117bbca..c414a7006d78 100644
> > --- a/security/keys/trusted-keys/trusted_tpm2.c
> > +++ b/security/keys/trusted-keys/trusted_tpm2.c
> > @@ -482,8 +482,10 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,
> > struct trusted_key_options *options,
> > u32 blob_handle)
> > {
> > + struct tpm_header *head;
> > struct tpm_buf buf;
> > u16 data_len;
> > + int offset;
> > u8 *data;
> > int rc;
> >
> > @@ -518,8 +520,14 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,
> > tpm2_buf_append_auth(&buf, options->policyhandle,
> > NULL /* nonce */, 0, 0,
> > options->blobauth, options->blobauth_len);
> > - tpm_buf_append_hmac_session_opt(chip, &buf, TPM2_SA_ENCRYPT,
> > - NULL, 0);
> > + if (tpm2_chip_auth(chip)) {
> > + tpm_buf_append_hmac_session(chip, &buf, TPM2_SA_ENCRYPT, NULL, 0);
> > + } else {
> > + offset = buf.handles * 4 + TPM_HEADER_SIZE;
> > + head = (struct tpm_header *)buf.data;
> > + if (tpm_buf_length(&buf) == offset)
> > + head->tag = cpu_to_be16(TPM2_ST_NO_SESSIONS);
> > + }
> > }
> >
> > tpm_buf_fill_hmac_session(chip, &buf);
> > --
> > 2.39.5
> >
> >
>
> J.
>
> --
> ... I love you the way a bomb loves a crowd.
BR, Jarkko
^ permalink raw reply
* Re: [PATCH v3 04/10] tpm2-sessions: Remove 'attributes' from tpm_buf_append_auth
From: Jarkko Sakkinen @ 2025-09-30 12:39 UTC (permalink / raw)
To: Jonathan McDowell
Cc: linux-integrity, dpsmith, ross.philipson, Jarkko Sakkinen,
Peter Huewe, Jason Gunthorpe, David Howells, Paul Moore,
James Morris, Serge E. Hallyn, Roberto Sassu, Mimi Zohar,
open list, open list:KEYS/KEYRINGS, open list:SECURITY SUBSYSTEM
In-Reply-To: <aNu6sZc-JTPhFTLV@earth.li>
On Tue, Sep 30, 2025 at 12:10:41PM +0100, Jonathan McDowell wrote:
> On Mon, Sep 29, 2025 at 10:48:26PM +0300, Jarkko Sakkinen wrote:
> > From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
> >
> > In a previous bug fix, 'attributes' was added by mistake to
> > tpm_buf_append_auth(). Remove the parameter.
> >
> > Fixes: 27184f8905ba ("tpm: Opt-in in disable PCR integrity protection")
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
>
> Reviewed-by: Jonathan McDowell <noodles@meta.com>
Thank you.
>
> > ---
> > v3:
> > - No changes
> > v2:
> > - Uncorrupt the patch.
> > ---
> > drivers/char/tpm/tpm2-cmd.c | 2 +-
> > drivers/char/tpm/tpm2-sessions.c | 5 ++---
> > include/linux/tpm.h | 2 +-
> > 3 files changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> > index a7cddd4b5626..86b1a4d859b9 100644
> > --- a/drivers/char/tpm/tpm2-cmd.c
> > +++ b/drivers/char/tpm/tpm2-cmd.c
> > @@ -191,7 +191,7 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
> > tpm_buf_append_hmac_session(chip, &buf, 0, NULL, 0);
> > } else {
> > tpm_buf_append_handle(chip, &buf, pcr_idx);
> > - tpm_buf_append_auth(chip, &buf, 0, NULL, 0);
> > + tpm_buf_append_auth(chip, &buf, NULL, 0);
> > }
> >
> > tpm_buf_append_u32(&buf, chip->nr_allocated_banks);
> > diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
> > index 6d03c224e6b2..13f019d1312a 100644
> > --- a/drivers/char/tpm/tpm2-sessions.c
> > +++ b/drivers/char/tpm/tpm2-sessions.c
> > @@ -266,7 +266,7 @@ void tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
> > EXPORT_SYMBOL_GPL(tpm_buf_append_name);
> >
> > void tpm_buf_append_auth(struct tpm_chip *chip, struct tpm_buf *buf,
> > - u8 attributes, u8 *passphrase, int passphrase_len)
> > + u8 *passphrase, int passphrase_len)
> > {
> > /* offset tells us where the sessions area begins */
> > int offset = buf->handles * 4 + TPM_HEADER_SIZE;
> > @@ -327,8 +327,7 @@ void tpm_buf_append_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf,
> > #endif
> >
> > if (!tpm2_chip_auth(chip)) {
> > - tpm_buf_append_auth(chip, buf, attributes, passphrase,
> > - passphrase_len);
> > + tpm_buf_append_auth(chip, buf, passphrase, passphrase_len);
> > return;
> > }
> >
> > diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> > index 51846317d662..1fa02e18e688 100644
> > --- a/include/linux/tpm.h
> > +++ b/include/linux/tpm.h
> > @@ -531,7 +531,7 @@ void tpm_buf_append_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf,
> > u8 attributes, u8 *passphrase,
> > int passphraselen);
> > void tpm_buf_append_auth(struct tpm_chip *chip, struct tpm_buf *buf,
> > - u8 attributes, u8 *passphrase, int passphraselen);
> > + u8 *passphrase, int passphraselen);
> > static inline void tpm_buf_append_hmac_session_opt(struct tpm_chip *chip,
> > struct tpm_buf *buf,
> > u8 attributes,
> > --
> > 2.39.5
> >
> >
>
> J.
>
> --
> 101 things you can't have too | .''`. Debian GNU/Linux Developer
> much of : 17 - Money. | : :' : Happy to accept PGP signed
> | `. `' or encrypted mail - RSA
> | `- key on the keyservers.
BR, Jarkko
^ permalink raw reply
* Re: [PATCH v3 03/10] KEYS: trusted: Use tpm_ret_to_err() in trusted_tpm2
From: Jarkko Sakkinen @ 2025-09-30 12:39 UTC (permalink / raw)
To: Stefano Garzarella
Cc: linux-integrity, dpsmith, ross.philipson, Jarkko Sakkinen,
David Howells, Paul Moore, James Morris, Serge E. Hallyn,
James Bottomley, Mimi Zohar, open list:KEYS/KEYRINGS,
open list:SECURITY SUBSYSTEM, open list
In-Reply-To: <7icpto3rnm2f4u6zaxl4xy7zvar3q4hhpgelq2gnazszdkwc3m@5dtivkqs5xdg>
On Tue, Sep 30, 2025 at 02:12:35PM +0200, Stefano Garzarella wrote:
> On Mon, Sep 29, 2025 at 10:48:25PM +0300, Jarkko Sakkinen wrote:
> > From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
> >
> > Use tpm_ret_to_err() to transmute TPM return codes in trusted_tpm2.
> >
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
> > ---
> > v3:
> > - No changes.
> > v2:
> > - New patch split out from the fix.
> > ---
> > security/keys/trusted-keys/trusted_tpm2.c | 26 ++++++-----------------
> > 1 file changed, 7 insertions(+), 19 deletions(-)
>
> Nice clean up!
>
> Acked-by: Stefano Garzarella <sgarzare@redhat.com>
Thank you.
>
> >
> > diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
> > index 024be262702f..e165b117bbca 100644
> > --- a/security/keys/trusted-keys/trusted_tpm2.c
> > +++ b/security/keys/trusted-keys/trusted_tpm2.c
> > @@ -348,25 +348,19 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
> > }
> >
> > blob_len = tpm2_key_encode(payload, options, &buf.data[offset], blob_len);
> > + if (blob_len < 0)
> > + rc = blob_len;
> >
> > out:
> > tpm_buf_destroy(&sized);
> > tpm_buf_destroy(&buf);
> >
> > - if (rc > 0) {
> > - if (tpm2_rc_value(rc) == TPM2_RC_HASH)
> > - rc = -EINVAL;
> > - else
> > - rc = -EPERM;
> > - }
> > - if (blob_len < 0)
> > - rc = blob_len;
> > - else
> > + if (!rc)
> > payload->blob_len = blob_len;
> >
> > out_put:
> > tpm_put_ops(chip);
> > - return rc;
> > + return tpm_ret_to_err(rc);
> > }
> >
> > /**
> > @@ -468,10 +462,7 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
> > kfree(blob);
> > tpm_buf_destroy(&buf);
> >
> > - if (rc > 0)
> > - rc = -EPERM;
> > -
> > - return rc;
> > + return tpm_ret_to_err(rc);
> > }
> >
> > /**
> > @@ -534,8 +525,6 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,
> > tpm_buf_fill_hmac_session(chip, &buf);
> > rc = tpm_transmit_cmd(chip, &buf, 6, "unsealing");
> > rc = tpm_buf_check_hmac_response(chip, &buf, rc);
> > - if (rc > 0)
> > - rc = -EPERM;
> >
> > if (!rc) {
> > data_len = be16_to_cpup(
> > @@ -568,7 +557,7 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,
> >
> > out:
> > tpm_buf_destroy(&buf);
> > - return rc;
> > + return tpm_ret_to_err(rc);
> > }
> >
> > /**
> > @@ -600,6 +589,5 @@ int tpm2_unseal_trusted(struct tpm_chip *chip,
> >
> > out:
> > tpm_put_ops(chip);
> > -
> > - return rc;
> > + return tpm_ret_to_err(rc);
> > }
> > --
> > 2.39.5
> >
> >
>
BR, Jarkko
^ permalink raw reply
* Re: [PATCH v3 02/10] tpm: Use -EPERM as fallback error code in tpm_ret_to_err
From: Jarkko Sakkinen @ 2025-09-30 12:37 UTC (permalink / raw)
To: Stefano Garzarella
Cc: linux-integrity, dpsmith, ross.philipson, Jarkko Sakkinen,
Peter Huewe, Jason Gunthorpe, David Howells, Paul Moore,
James Morris, Serge E. Hallyn, open list, open list:KEYS/KEYRINGS,
open list:SECURITY SUBSYSTEM
In-Reply-To: <hjay4b2lomj6k63tbnuk55q6mm4sdj2d7yycw64ybhu372l6bd@uqetr6ipbtg4>
On Tue, Sep 30, 2025 at 02:11:23PM +0200, Stefano Garzarella wrote:
> On Mon, Sep 29, 2025 at 10:48:24PM +0300, Jarkko Sakkinen wrote:
> > From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
> >
> > Using -EFAULT as the tpm_ret_to_err() fallback error code causes makes it
> > incompatible on how trusted keys transmute TPM return codes.
> >
> > Change the fallback as -EPERM in order to gain compatibility with trusted
> > keys. In addition, map TPM_RC_HASH to -EINVAL in order to be compatible
> > with tpm2_seal_trusted() return values.
> >
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
> > ---
> > v3:
> > - Removed fixes tag as it hardly categorizes as a bug fix.
> > v2:
> > - Split trusted_tpm2 change to a separate patch.
> > ---
> > include/linux/tpm.h | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
>
> LGTM now!
>
> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Thank you.
>
> >
> > diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> > index fc7df87dfb9a..51846317d662 100644
> > --- a/include/linux/tpm.h
> > +++ b/include/linux/tpm.h
> > @@ -453,8 +453,10 @@ static inline ssize_t tpm_ret_to_err(ssize_t ret)
> > return 0;
> > case TPM2_RC_SESSION_MEMORY:
> > return -ENOMEM;
> > + case TPM2_RC_HASH:
> > + return -EINVAL;
> > default:
> > - return -EFAULT;
> > + return -EPERM;
> > }
> > }
> >
> > --
> > 2.39.5
> >
> >
>
BR, Jarkko
^ permalink raw reply
* Re: [PATCH v3 01/10] tpm: Cap the number of PCR banks
From: Jarkko Sakkinen @ 2025-09-30 12:36 UTC (permalink / raw)
To: Jonathan McDowell
Cc: linux-integrity, dpsmith, ross.philipson, Jarkko Sakkinen,
Roberto Sassu, Peter Huewe, Jason Gunthorpe, David Howells,
Paul Moore, James Morris, Serge E. Hallyn, open list,
open list:KEYS/KEYRINGS, open list:SECURITY SUBSYSTEM
In-Reply-To: <aNu6W0GagfCliWTx@earth.li>
On Tue, Sep 30, 2025 at 12:09:15PM +0100, Jonathan McDowell wrote:
> On Mon, Sep 29, 2025 at 10:48:23PM +0300, Jarkko Sakkinen wrote:
> > From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
> >
> > tpm2_get_pcr_allocation() does not cap any upper limit for the number of
> > banks. Cap the limit to four banks so that out of bounds values coming
> > from external I/O cause on only limited harm.
> >
> > Cc: Roberto Sassu <roberto.sassu@huawei.com>
> > Fixes: bcfff8384f6c ("tpm: dynamically allocate the allocated_banks array")
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
> > ---
> > v3:
> > - Wrote a more clear commit message.
> > - Fixed pr_err() message.
> > v2:
> > - A new patch.
> > ---
> > drivers/char/tpm/tpm-chip.c | 13 +++++++++----
> > drivers/char/tpm/tpm.h | 1 -
> > drivers/char/tpm/tpm1-cmd.c | 25 -------------------------
> > drivers/char/tpm/tpm2-cmd.c | 8 +++-----
> > include/linux/tpm.h | 18 ++++++++----------
> > 5 files changed, 20 insertions(+), 45 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> > index 687f6d8cd601..9a6538f76f50 100644
> > --- a/drivers/char/tpm/tpm-chip.c
> > +++ b/drivers/char/tpm/tpm-chip.c
> > @@ -559,14 +559,19 @@ static int tpm_add_hwrng(struct tpm_chip *chip)
> >
> > static int tpm_get_pcr_allocation(struct tpm_chip *chip)
> > {
> > - int rc;
> > + int rc = 0;
> >
> > if (tpm_is_firmware_upgrade(chip))
> > return 0;
> >
> > - rc = (chip->flags & TPM_CHIP_FLAG_TPM2) ?
> > - tpm2_get_pcr_allocation(chip) :
> > - tpm1_get_pcr_allocation(chip);
> > + if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) {
> > + chip->allocated_banks[0].alg_id = TPM_ALG_SHA1;
> > + chip->allocated_banks[0].digest_size = hash_digest_size[HASH_ALGO_SHA1];
> > + chip->allocated_banks[0].crypto_id = HASH_ALGO_SHA1;
> > + chip->nr_allocated_banks = 1;
> > + } else {
> > + rc = tpm2_get_pcr_allocation(chip);
> > + }
> >
> > if (rc > 0)
> > return -ENODEV;
> > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> > index 57ef8589f5f5..769fa6b00c54 100644
> > --- a/drivers/char/tpm/tpm.h
> > +++ b/drivers/char/tpm/tpm.h
> > @@ -252,7 +252,6 @@ int tpm1_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8 *res_buf);
> > ssize_t tpm1_getcap(struct tpm_chip *chip, u32 subcap_id, cap_t *cap,
> > const char *desc, size_t min_cap_length);
> > int tpm1_get_random(struct tpm_chip *chip, u8 *out, size_t max);
> > -int tpm1_get_pcr_allocation(struct tpm_chip *chip);
> > unsigned long tpm_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
> > int tpm_pm_suspend(struct device *dev);
> > int tpm_pm_resume(struct device *dev);
> > diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
> > index cf64c7385105..5c49bdff33de 100644
> > --- a/drivers/char/tpm/tpm1-cmd.c
> > +++ b/drivers/char/tpm/tpm1-cmd.c
> > @@ -786,28 +786,3 @@ int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr)
> >
> > return rc;
> > }
> > -
> > -/**
> > - * tpm1_get_pcr_allocation() - initialize the allocated bank
> > - * @chip: TPM chip to use.
> > - *
> > - * The function initializes the SHA1 allocated bank to extend PCR
> > - *
> > - * Return:
> > - * * 0 on success,
> > - * * < 0 on error.
> > - */
> > -int tpm1_get_pcr_allocation(struct tpm_chip *chip)
> > -{
> > - chip->allocated_banks = kcalloc(1, sizeof(*chip->allocated_banks),
> > - GFP_KERNEL);
> > - if (!chip->allocated_banks)
> > - return -ENOMEM;
> > -
> > - chip->allocated_banks[0].alg_id = TPM_ALG_SHA1;
> > - chip->allocated_banks[0].digest_size = hash_digest_size[HASH_ALGO_SHA1];
> > - chip->allocated_banks[0].crypto_id = HASH_ALGO_SHA1;
> > - chip->nr_allocated_banks = 1;
> > -
> > - return 0;
> > -}
> > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> > index 7d77f6fbc152..a7cddd4b5626 100644
> > --- a/drivers/char/tpm/tpm2-cmd.c
> > +++ b/drivers/char/tpm/tpm2-cmd.c
> > @@ -538,11 +538,9 @@ ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
> >
> > nr_possible_banks = be32_to_cpup(
> > (__be32 *)&buf.data[TPM_HEADER_SIZE + 5]);
> > -
> > - chip->allocated_banks = kcalloc(nr_possible_banks,
> > - sizeof(*chip->allocated_banks),
> > - GFP_KERNEL);
> > - if (!chip->allocated_banks) {
> > + if (nr_possible_banks > TPM2_MAX_BANKS) {
> > + pr_err("tpm: unexpected number of banks: %u > %u",
> > + nr_possible_banks, TPM2_MAX_BANKS);
> > rc = -ENOMEM;
> > goto out;
> > }
> > diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> > index 900c81a2bc41..fc7df87dfb9a 100644
> > --- a/include/linux/tpm.h
> > +++ b/include/linux/tpm.h
> > @@ -27,7 +27,12 @@
> > #include <crypto/aes.h>
> >
> > #define TPM_DIGEST_SIZE 20 /* Max TPM v1.2 PCR size */
> > -#define TPM_MAX_DIGEST_SIZE SHA512_DIGEST_SIZE
> > +#define TPM_HEADER_SIZE 10
> > +
> > +#define TPM2_PLATFORM_PCR 24
> > +#define TPM2_PCR_SELECT_MIN 3
>
> By changing this to 3 we lose the fact it's related to TPM2_PLATFORM_PCR
> - it's the number of bytes required to hold a bitmap with at least
> TPM2_PLATFORM_PCR entries. Can we at least have a comment about that
> fact?
I'll revert this as it is essentially a spurious change.
>
> > +#define TPM2_MAX_DIGEST_SIZE SHA512_DIGEST_SIZE
> > +#define TPM2_MAX_BANKS 4
>
> Where does this max come from? It matches what I see with swtpm by
> default (SHA1, SHA2-256, SHA2-384, SHA-512), so I haven't seen anything
> that exceeds it myself.
I've never seen hardware TPM that would have more than one or two banks.
We can double it to leave some room. This was tested with swtpm
defaults.
>
> > struct tpm_chip;
> > struct trusted_key_payload;
> > @@ -69,7 +74,7 @@ enum tpm2_curves {
> >
> > struct tpm_digest {
> > u16 alg_id;
> > - u8 digest[TPM_MAX_DIGEST_SIZE];
> > + u8 digest[TPM2_MAX_DIGEST_SIZE];
> > } __packed;
> >
> > struct tpm_bank_info {
> > @@ -190,7 +195,7 @@ struct tpm_chip {
> > unsigned int groups_cnt;
> >
> > u32 nr_allocated_banks;
> > - struct tpm_bank_info *allocated_banks;
> > + struct tpm_bank_info allocated_banks[TPM2_MAX_BANKS];
> > #ifdef CONFIG_ACPI
> > acpi_handle acpi_dev_handle;
> > char ppi_version[TPM_PPI_VERSION_LEN + 1];
> > @@ -217,13 +222,6 @@ struct tpm_chip {
> > #endif
> > };
> >
> > -#define TPM_HEADER_SIZE 10
> > -
> > -enum tpm2_const {
> > - TPM2_PLATFORM_PCR = 24,
> > - TPM2_PCR_SELECT_MIN = ((TPM2_PLATFORM_PCR + 7) / 8),
> > -};
> > -
> > enum tpm2_timeouts {
> > TPM2_TIMEOUT_A = 750,
> > TPM2_TIMEOUT_B = 4000,
> > --
> > 2.39.5
> >
> >
>
> J.
>
> --
> "How the f**k did you work that out?" -- Pythagoras
BR, Jarkko
^ permalink raw reply
* Re: [RFC PATCH v2] lsm,selinux: introduce LSM_ATTR_UNSHARE and wire it up for SELinux
From: Stephen Smalley @ 2025-09-30 12:32 UTC (permalink / raw)
To: linux-security-module, selinux
Cc: paul, omosnace, john.johansen, serge, casey
In-Reply-To: <20250918135904.9997-2-stephen.smalley.work@gmail.com>
On Thu, Sep 18, 2025 at 10:07 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> RFC-only, will ultimately split the LSM-only changes to their own
> patch for submission. I have now tested this with the corresponding
> selinux userspace change that you can find at
> https://lore.kernel.org/selinux/20250918135118.9896-2-stephen.smalley.work@gmail.com/
> and also verified that my modified systemd-nspawn still works when
> starting containers with their own SELinux namespace.
As it turns out, I had NOT tested this with my modified systemd-nspawn
- it was picking up the wrong libselinux and fails if I used the one
updated to call the lsm_set_self_attr() system call instead of the
/sys/fs/selinux/unshare interface. This was due to systemd-nspawn
setting seccomp filters prior to the point at which it was doing the
unshare and lsm_set_self_attr() not being included in the allowlist. I
fixed this by moving the selinux_unshare() call before setting the
seccomp filters and that solved the problem. Updated systemd patches
pushed to my systemd fork for anyone trying this themselves.
>
> This defines a new LSM_ATTR_UNSHARE attribute for the
> lsm_set_self_attr(2) system call and wires it up for SELinux to invoke
> the underlying function for unsharing the SELinux namespace. As with
> the selinuxfs interface, this immediately unshares the SELinux
> namespace of the current process just like an unshare(2) system call
> would do for other namespaces. I have not yet explored the
> alternatives of deferring the unshare to the next unshare(2),
> clone(2), or execve(2) call and would want to first confirm that doing
> so does not introduce any issues in the kernel or make it harder to
> integrate with existing container runtimes.
>
> Differences between this syscall interface and the selinuxfs interface
> that need discussion before moving forward:
>
> 1. The syscall interface does not currently check any Linux capability
> or DAC permissions, whereas the selinuxfs interface can only be set by
> uid-0 or CAP_DAC_OVERRIDE processes. We need to decide what if any
> capability or DAC check should apply to this syscall interface and if
> any, add the checks to either the LSM framework code or to the SELinux
> hook function.
>
> Pros: Checking a capability or DAC permissions prevents misuse of this
> interface by unprivileged processes, particularly on systems with
> policies that do not yet define any of the new SELinux permissions
> introduced for controlling this operation. This is a potential concern
> on Linux distributions that do not tightly coordinate kernel updates
> with policy updates (or where users may choose to deploy upstream
> kernels on their own), but not on Android.
>
> Cons: Checking a capability or DAC permissions requires any process
> that uses this facility to have the corresponding capability or
> permissions, which might otherwise be unnecessary and create
> additional risks. This is less likely if we use a capability already
> required by container runtimes and similar components that might
> leverage this facility for unsharing SELinux namespaces.
>
> 2. The syscall interface checks a new SELinux unshare_selinuxns
> permission in the process2 class between the task SID and itself,
> similar to other checks for setting process attributes. This means
> that:
> allow domain self:process2 *; -or-
> allow domain self:process2 ~anything-other-than-unshare_selinuxns; -or-
> allow domain self:process2 unshare_selinuxns;
> would allow a process to unshare its SELinux namespace.
>
> The selinuxfs interface checks a new unshare permission in the
> security class between the task SID and the security initial SID,
> likewise similar to other checks for setting selinuxfs attributes.
> This means that:
> allow domain security_t:security *; -or-
> allow domain security_t:security ~anything-other-than-unshare; -or-
> allow domain security_t:security unshare;
> would allow a process to unshare its SELinux namespace.
>
> Technically, the selinuxfs interface also currently requires open and
> write access to the selinuxfs node; hence:
> allow domain security_t:file { open write };
> is also required for the selinuxfs interface.
>
> We need to decide what we want the SELinux check(s) to be for the
> syscall and whether it should be more like the former (process
> attributes) or more like the latter (security policy settings). Note
> that the permission name itself is unimportant here and only differs
> because it seemed less evident in the process2 class that we are
> talking about a SELinux namespace otherwise.
>
> Regardless, either form of allow rule can be prohibited in policies
> via neverallow rules on systems that enforce their usage
> (e.g. Android, not necessarily on Linux distributions).
>
> 3. The selinuxfs interface currently offers more functionality than I
> have implemented here for the sycall interface, including:
>
> a) the ability to read the selinuxfs node to see if your namespace has
> been unshared, which should be easily implementable via
> lsm_get_self_attr(2). However, questions remain as to when that
> should return 1 versus 0 (currently returns 1 whenever your namespace
> is NOT the initial SELinux namespace, useful for the testsuite to
> detect it is in a child, but could instead be reset to 0 by a
> subsequent policy load to indicate completion of the setup of the
> namespace, thus hiding from child processes that they are in a child
> namespace once its policy has been loaded).
>
> b) the abilities to get and set the maximum number of SELinux
> namespaces (via a /sys/fs/selinux/maxns node) and to get and set the
> maximum depth for SELinux namespaces (via a /sys/fs/selinux/maxnsdepth
> node). These could be left in selinuxfs or migrated to some other LSM
> management APIs since they are global in scope, not per-process
> attributes.
>
> Signed-off-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> ---
> v2 fixes a typo (PROCESS->PROCESS2) and is now tested.
>
> include/uapi/linux/lsm.h | 1 +
> security/selinux/hooks.c | 8 ++++++++
> security/selinux/include/classmap.h | 4 +++-
> 3 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/lsm.h b/include/uapi/linux/lsm.h
> index 938593dfd5da..fb1b4a8aa639 100644
> --- a/include/uapi/linux/lsm.h
> +++ b/include/uapi/linux/lsm.h
> @@ -83,6 +83,7 @@ struct lsm_ctx {
> #define LSM_ATTR_KEYCREATE 103
> #define LSM_ATTR_PREV 104
> #define LSM_ATTR_SOCKCREATE 105
> +#define LSM_ATTR_UNSHARE 106
>
> /*
> * LSM_FLAG_XXX definitions identify special handling instructions
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index f48483383d6e..1e34a16b7954 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -6816,6 +6816,10 @@ static int selinux_lsm_setattr(u64 attr, void *value, size_t size)
> error = avc_has_perm(state, mysid, mysid, SECCLASS_PROCESS,
> PROCESS__SETCURRENT, NULL);
> break;
> + case LSM_ATTR_UNSHARE:
> + error = avc_has_perm(state, mysid, mysid, SECCLASS_PROCESS2,
> + PROCESS2__UNSHARE_SELINUXNS, NULL);
> + break;
> default:
> error = -EOPNOTSUPP;
> break;
> @@ -6927,6 +6931,10 @@ static int selinux_lsm_setattr(u64 attr, void *value, size_t size)
> }
>
> tsec->sid = sid;
> + } else if (attr == LSM_ATTR_UNSHARE) {
> + error = selinux_state_create(new);
> + if (error)
> + goto abort_change;
> } else {
> error = -EINVAL;
> goto abort_change;
> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
> index be52ebb6b94a..07fe316308cd 100644
> --- a/security/selinux/include/classmap.h
> +++ b/security/selinux/include/classmap.h
> @@ -60,7 +60,9 @@ const struct security_class_mapping secclass_map[] = {
> "siginh", "setrlimit", "rlimitinh", "dyntransition",
> "setcurrent", "execmem", "execstack", "execheap",
> "setkeycreate", "setsockcreate", "getrlimit", NULL } },
> - { "process2", { "nnp_transition", "nosuid_transition", NULL } },
> + { "process2",
> + { "nnp_transition", "nosuid_transition", "unshare_selinuxns",
> + NULL } },
> { "system",
> { "ipc_info", "syslog_read", "syslog_mod", "syslog_console",
> "module_request", "module_load", "firmware_load",
> --
> 2.50.1
>
^ permalink raw reply
* [PATCH] smack: /smack/doi: accept previously used values
From: Konstantin Andreev @ 2025-09-30 12:31 UTC (permalink / raw)
To: casey; +Cc: linux-security-module
Writing to /smack/doi a value that has ever been
written there in the past disables networking for
non-ambient labels.
E.g.
# cat /smack/doi
3
# netlabelctl -p cipso list
Configured CIPSO mappings (1)
DOI value : 3
mapping type : PASS_THROUGH
# netlabelctl -p map list
Configured NetLabel domain mappings (3)
domain: "_" (IPv4)
protocol: UNLABELED
domain: DEFAULT (IPv4)
protocol: CIPSO, DOI = 3
domain: DEFAULT (IPv6)
protocol: UNLABELED
# cat /smack/ambient
_
# cat /proc/$$/attr/smack/current
_
# ping -c1 10.1.95.12
64 bytes from 10.1.95.12: icmp_seq=1 ttl=64 time=0.964 ms
# echo foo >/proc/$$/attr/smack/current
# ping -c1 10.1.95.12
64 bytes from 10.1.95.12: icmp_seq=1 ttl=64 time=0.956 ms
unknown option 86
# echo 4 >/smack/doi
# echo 3 >/smack/doi
!> [ 214.050395] smk_cipso_doi:691 cipso add rc = -17
# echo 3 >/smack/doi
!> [ 249.402261] smk_cipso_doi:678 remove rc = -2
!> [ 249.402261] smk_cipso_doi:691 cipso add rc = -17
# ping -c1 10.1.95.12
!!> ping: 10.1.95.12: Address family for hostname not supported
# echo _ >/proc/$$/attr/smack/current
# ping -c1 10.1.95.12
64 bytes from 10.1.95.12: icmp_seq=1 ttl=64 time=0.617 ms
This happens because Smack keeps decommissioned DOIs,
fails to re-add them, and consequently refuses to add
the “default” domain map:
# netlabelctl -p cipso list
Configured CIPSO mappings (2)
DOI value : 3
mapping type : PASS_THROUGH
DOI value : 4
mapping type : PASS_THROUGH
# netlabelctl -p map list
Configured NetLabel domain mappings (2)
domain: "_" (IPv4)
protocol: UNLABELED
!> (no ipv4 map for default domain here)
domain: DEFAULT (IPv6)
protocol: UNLABELED
Fix by clearing decommissioned DOI definitions and
serializing concurrent DOI updates with a new lock.
Also:
- allow /smack/doi to live unconfigured, since
adding a map (netlbl_cfg_cipsov4_map_add) may fail.
CIPSO_V4_DOI_UNKNOWN(0) indicates the unconfigured DOI
- add new DOI before removing the old default map,
so the old map remains if the add fails
(2008-02-04, Casey Schaufler)
Fixes: e114e473771c ("Smack: Simplified Mandatory Access Control Kernel")
Signed-off-by: Konstantin Andreev <andreev@swemel.ru>
---
The patch is verified to work on top of "smack: /smack/doi must be > 0" patch.
Link: https://lore.kernel.org/linux-security-module/20250930121602.138337-1-andreev@swemel.ru
security/smack/smackfs.c | 71 +++++++++++++++++++++++++---------------
1 file changed, 45 insertions(+), 26 deletions(-)
diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
index 316c2ea401e8..d27d9140dda2 100644
--- a/security/smack/smackfs.c
+++ b/security/smack/smackfs.c
@@ -70,6 +70,7 @@ enum smk_inos {
static DEFINE_MUTEX(smack_cipso_lock);
static DEFINE_MUTEX(smack_ambient_lock);
static DEFINE_MUTEX(smk_net4addr_lock);
+static DEFINE_MUTEX(smk_cipso_doi_lock);
#if IS_ENABLED(CONFIG_IPV6)
static DEFINE_MUTEX(smk_net6addr_lock);
#endif /* CONFIG_IPV6 */
@@ -141,7 +142,7 @@ struct smack_parsed_rule {
int smk_access2;
};
-static u32 smk_cipso_doi_value = SMACK_CIPSO_DOI_DEFAULT;
+static u32 smk_cipso_doi_value = CIPSO_V4_DOI_UNKNOWN;
/*
* Values for parsing cipso rules
@@ -663,43 +664,60 @@ static const struct file_operations smk_load_ops = {
};
/**
- * smk_cipso_doi - initialize the CIPSO domain
+ * smk_cipso_doi - set netlabel maps
+ * @ndoi: new value for our CIPSO DOI
+ * @gfp_flags: kmalloc allocation context
*/
-static void smk_cipso_doi(void)
+static int
+smk_cipso_doi(u32 ndoi, gfp_t gfp_flags)
{
- int rc;
+ int rc = 0;
struct cipso_v4_doi *doip;
struct netlbl_audit nai;
+ mutex_lock(&smk_cipso_doi_lock);
+
+ if (smk_cipso_doi_value == ndoi)
+ goto clr_doi_lock;
+
smk_netlabel_audit_set(&nai);
- rc = netlbl_cfg_map_del(NULL, PF_INET, NULL, NULL, &nai);
- if (rc != 0)
- printk(KERN_WARNING "%s:%d remove rc = %d\n",
- __func__, __LINE__, rc);
-
- doip = kmalloc(sizeof(struct cipso_v4_doi), GFP_KERNEL | __GFP_NOFAIL);
+ doip = kmalloc(sizeof(struct cipso_v4_doi), gfp_flags);
+ if (!doip) {
+ rc = -ENOMEM;
+ goto clr_doi_lock;
+ }
doip->map.std = NULL;
- doip->doi = smk_cipso_doi_value;
+ doip->doi = ndoi;
doip->type = CIPSO_V4_MAP_PASS;
doip->tags[0] = CIPSO_V4_TAG_RBITMAP;
for (rc = 1; rc < CIPSO_V4_TAG_MAXCNT; rc++)
doip->tags[rc] = CIPSO_V4_TAG_INVALID;
rc = netlbl_cfg_cipsov4_add(doip, &nai);
- if (rc != 0) {
- printk(KERN_WARNING "%s:%d cipso add rc = %d\n",
- __func__, __LINE__, rc);
+ if (rc) {
kfree(doip);
- return;
+ goto clr_doi_lock;
}
- rc = netlbl_cfg_cipsov4_map_add(doip->doi, NULL, NULL, NULL, &nai);
- if (rc != 0) {
- printk(KERN_WARNING "%s:%d map add rc = %d\n",
- __func__, __LINE__, rc);
- netlbl_cfg_cipsov4_del(doip->doi, &nai);
- return;
+
+ if (smk_cipso_doi_value != CIPSO_V4_DOI_UNKNOWN) {
+ rc = netlbl_cfg_map_del(NULL, PF_INET, NULL, NULL, &nai);
+ if (rc && rc != -ENOENT)
+ goto clr_ndoi_def;
+
+ netlbl_cfg_cipsov4_del(smk_cipso_doi_value, &nai);
}
+
+ rc = netlbl_cfg_cipsov4_map_add(ndoi, NULL, NULL, NULL, &nai);
+ if (rc) {
+ smk_cipso_doi_value = CIPSO_V4_DOI_UNKNOWN; // no default map
+clr_ndoi_def: netlbl_cfg_cipsov4_del(ndoi, &nai);
+ } else
+ smk_cipso_doi_value = ndoi;
+
+clr_doi_lock:
+ mutex_unlock(&smk_cipso_doi_lock);
+ return rc;
}
/**
@@ -1599,11 +1617,8 @@ static ssize_t smk_write_doi(struct file *file, const char __user *buf,
if (u == CIPSO_V4_DOI_UNKNOWN || u > U32_MAX)
return -EINVAL;
- smk_cipso_doi_value = u;
- smk_cipso_doi();
-
- return count;
+ return smk_cipso_doi(u, GFP_KERNEL) ? : count;
}
static const struct file_operations smk_doi_ops = {
@@ -2984,6 +2999,7 @@ static int __init init_smk_fs(void)
{
int err;
int rc;
+ struct netlbl_audit nai;
if (smack_enabled == 0)
return 0;
@@ -3002,7 +3018,10 @@ static int __init init_smk_fs(void)
}
}
- smk_cipso_doi();
+ smk_netlabel_audit_set(&nai);
+ (void) netlbl_cfg_map_del(NULL, PF_INET, NULL, NULL, &nai);
+ (void) smk_cipso_doi(SMACK_CIPSO_DOI_DEFAULT,
+ GFP_KERNEL | __GFP_NOFAIL);
smk_unlbl_ambient(NULL);
rc = smack_populate_secattr(&smack_known_floor);
--
2.43.0
^ permalink raw reply related
* [PATCH] smack: /smack/doi must be > 0
From: Konstantin Andreev @ 2025-09-30 12:16 UTC (permalink / raw)
To: casey; +Cc: linux-security-module
/smack/doi allows writing and keeping negative doi values.
Correct values are 0 < doi <= (max 32-bit positive integer)
(2008-02-04, Casey Schaufler)
Fixes: e114e473771c ("Smack: Simplified Mandatory Access Control Kernel")
Signed-off-by: Konstantin Andreev <andreev@swemel.ru>
---
The patch applies on top of smack/next, commit 6ddd169d0288 ("smack: fix
kernel-doc warnings for smk_import_valid_label()")
security/smack/smackfs.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
index b1e5e62f5cbd..316c2ea401e8 100644
--- a/security/smack/smackfs.c
+++ b/security/smack/smackfs.c
@@ -141,7 +141,7 @@ struct smack_parsed_rule {
int smk_access2;
};
-static int smk_cipso_doi_value = SMACK_CIPSO_DOI_DEFAULT;
+static u32 smk_cipso_doi_value = SMACK_CIPSO_DOI_DEFAULT;
/*
* Values for parsing cipso rules
@@ -1562,7 +1562,7 @@ static ssize_t smk_read_doi(struct file *filp, char __user *buf,
if (*ppos != 0)
return 0;
- sprintf(temp, "%d", smk_cipso_doi_value);
+ sprintf(temp, "%lu", (unsigned long)smk_cipso_doi_value);
rc = simple_read_from_buffer(buf, count, ppos, temp, strlen(temp));
return rc;
@@ -1581,7 +1581,7 @@ static ssize_t smk_write_doi(struct file *file, const char __user *buf,
size_t count, loff_t *ppos)
{
char temp[80];
- int i;
+ unsigned long u;
if (!smack_privileged(CAP_MAC_ADMIN))
return -EPERM;
@@ -1594,10 +1594,12 @@ static ssize_t smk_write_doi(struct file *file, const char __user *buf,
temp[count] = '\0';
- if (sscanf(temp, "%d", &i) != 1)
+ if (kstrtoul(temp, 10, &u))
return -EINVAL;
- smk_cipso_doi_value = i;
+ if (u == CIPSO_V4_DOI_UNKNOWN || u > U32_MAX)
+ return -EINVAL;
+ smk_cipso_doi_value = u;
smk_cipso_doi();
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v3 03/10] KEYS: trusted: Use tpm_ret_to_err() in trusted_tpm2
From: Stefano Garzarella @ 2025-09-30 12:12 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: linux-integrity, dpsmith, ross.philipson, Jarkko Sakkinen,
David Howells, Paul Moore, James Morris, Serge E. Hallyn,
James Bottomley, Mimi Zohar, open list:KEYS/KEYRINGS,
open list:SECURITY SUBSYSTEM, open list
In-Reply-To: <20250929194832.2913286-4-jarkko@kernel.org>
On Mon, Sep 29, 2025 at 10:48:25PM +0300, Jarkko Sakkinen wrote:
>From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
>
>Use tpm_ret_to_err() to transmute TPM return codes in trusted_tpm2.
>
>Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
>---
>v3:
>- No changes.
>v2:
>- New patch split out from the fix.
>---
> security/keys/trusted-keys/trusted_tpm2.c | 26 ++++++-----------------
> 1 file changed, 7 insertions(+), 19 deletions(-)
Nice clean up!
Acked-by: Stefano Garzarella <sgarzare@redhat.com>
>
>diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
>index 024be262702f..e165b117bbca 100644
>--- a/security/keys/trusted-keys/trusted_tpm2.c
>+++ b/security/keys/trusted-keys/trusted_tpm2.c
>@@ -348,25 +348,19 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
> }
>
> blob_len = tpm2_key_encode(payload, options, &buf.data[offset], blob_len);
>+ if (blob_len < 0)
>+ rc = blob_len;
>
> out:
> tpm_buf_destroy(&sized);
> tpm_buf_destroy(&buf);
>
>- if (rc > 0) {
>- if (tpm2_rc_value(rc) == TPM2_RC_HASH)
>- rc = -EINVAL;
>- else
>- rc = -EPERM;
>- }
>- if (blob_len < 0)
>- rc = blob_len;
>- else
>+ if (!rc)
> payload->blob_len = blob_len;
>
> out_put:
> tpm_put_ops(chip);
>- return rc;
>+ return tpm_ret_to_err(rc);
> }
>
> /**
>@@ -468,10 +462,7 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
> kfree(blob);
> tpm_buf_destroy(&buf);
>
>- if (rc > 0)
>- rc = -EPERM;
>-
>- return rc;
>+ return tpm_ret_to_err(rc);
> }
>
> /**
>@@ -534,8 +525,6 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,
> tpm_buf_fill_hmac_session(chip, &buf);
> rc = tpm_transmit_cmd(chip, &buf, 6, "unsealing");
> rc = tpm_buf_check_hmac_response(chip, &buf, rc);
>- if (rc > 0)
>- rc = -EPERM;
>
> if (!rc) {
> data_len = be16_to_cpup(
>@@ -568,7 +557,7 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,
>
> out:
> tpm_buf_destroy(&buf);
>- return rc;
>+ return tpm_ret_to_err(rc);
> }
>
> /**
>@@ -600,6 +589,5 @@ int tpm2_unseal_trusted(struct tpm_chip *chip,
>
> out:
> tpm_put_ops(chip);
>-
>- return rc;
>+ return tpm_ret_to_err(rc);
> }
>--
>2.39.5
>
>
^ permalink raw reply
* Re: [PATCH v3 02/10] tpm: Use -EPERM as fallback error code in tpm_ret_to_err
From: Stefano Garzarella @ 2025-09-30 12:11 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: linux-integrity, dpsmith, ross.philipson, Jarkko Sakkinen,
Peter Huewe, Jason Gunthorpe, David Howells, Paul Moore,
James Morris, Serge E. Hallyn, open list, open list:KEYS/KEYRINGS,
open list:SECURITY SUBSYSTEM
In-Reply-To: <20250929194832.2913286-3-jarkko@kernel.org>
On Mon, Sep 29, 2025 at 10:48:24PM +0300, Jarkko Sakkinen wrote:
>From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
>
>Using -EFAULT as the tpm_ret_to_err() fallback error code causes makes it
>incompatible on how trusted keys transmute TPM return codes.
>
>Change the fallback as -EPERM in order to gain compatibility with trusted
>keys. In addition, map TPM_RC_HASH to -EINVAL in order to be compatible
>with tpm2_seal_trusted() return values.
>
>Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
>---
>v3:
>- Removed fixes tag as it hardly categorizes as a bug fix.
>v2:
>- Split trusted_tpm2 change to a separate patch.
>---
> include/linux/tpm.h | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
LGTM now!
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
>
>diff --git a/include/linux/tpm.h b/include/linux/tpm.h
>index fc7df87dfb9a..51846317d662 100644
>--- a/include/linux/tpm.h
>+++ b/include/linux/tpm.h
>@@ -453,8 +453,10 @@ static inline ssize_t tpm_ret_to_err(ssize_t ret)
> return 0;
> case TPM2_RC_SESSION_MEMORY:
> return -ENOMEM;
>+ case TPM2_RC_HASH:
>+ return -EINVAL;
> default:
>- return -EFAULT;
>+ return -EPERM;
> }
> }
>
>--
>2.39.5
>
>
^ permalink raw reply
* Re: [PATCH v3 08/10] tpm-buf: Remove chip parameter from tpm_buf_append_handle
From: Jonathan McDowell @ 2025-09-30 11:14 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: linux-integrity, dpsmith, ross.philipson, Jarkko Sakkinen,
Peter Huewe, Jason Gunthorpe, David Howells, Paul Moore,
James Morris, Serge E. Hallyn, open list, open list:KEYS/KEYRINGS,
open list:SECURITY SUBSYSTEM
In-Reply-To: <20250929194832.2913286-9-jarkko@kernel.org>
On Mon, Sep 29, 2025 at 10:48:30PM +0300, Jarkko Sakkinen wrote:
> From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
>
> Remove chip parameter from tpm_buf_append_handle() in order to maintain
> decoupled state with tpm-buf. This is mandatory change in order to re-use
> the module in early boot code of Trenchboot, and the binding itself brings
> no benefit. Use WARN like in other functions, as the error condition can
> happen only as a net effect of a trivial programming mistake.
>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
Reviewed-by: Jonathan McDowell <noodles@meta.com>
> ---
> v3:
> - No changes.
> v2:
> - A new patch.
> ---
> drivers/char/tpm/tpm-buf.c | 5 ++---
> drivers/char/tpm/tpm2-cmd.c | 2 +-
> drivers/char/tpm/tpm2-sessions.c | 2 +-
> include/linux/tpm.h | 2 +-
> 4 files changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
> index 69ee77400539..1b9dee0d0681 100644
> --- a/drivers/char/tpm/tpm-buf.c
> +++ b/drivers/char/tpm/tpm-buf.c
> @@ -147,20 +147,19 @@ EXPORT_SYMBOL_GPL(tpm_buf_append_u32);
>
> /**
> * tpm_buf_append_handle() - Add a handle
> - * @chip: &tpm_chip instance
> * @buf: &tpm_buf instance
> * @handle: a TPM object handle
> *
> * Add a handle to the buffer, and increase the count tracking the number of
> * handles in the command buffer. Works only for command buffers.
> */
> -void tpm_buf_append_handle(struct tpm_chip *chip, struct tpm_buf *buf, u32 handle)
> +void tpm_buf_append_handle(struct tpm_buf *buf, u32 handle)
> {
> if (buf->flags & TPM_BUF_INVALID)
> return;
>
> if (buf->flags & TPM_BUF_TPM2B) {
> - dev_err(&chip->dev, "Invalid buffer type (TPM2B)\n");
> + WARN(1, "tpm-buf: invalid type: TPM2B\n");
> buf->flags |= TPM_BUF_INVALID;
> return;
> }
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index c7bfa705ea8f..b69ff7266450 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -190,7 +190,7 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
> tpm_buf_append_name(chip, &buf, pcr_idx, NULL);
> tpm_buf_append_hmac_session(chip, &buf, 0, NULL, 0);
> } else {
> - tpm_buf_append_handle(chip, &buf, pcr_idx);
> + tpm_buf_append_handle(&buf, pcr_idx);
> tpm_buf_append_auth(chip, &buf, NULL, 0);
> }
>
> diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
> index 13f019d1312a..bbc05f0997a8 100644
> --- a/drivers/char/tpm/tpm2-sessions.c
> +++ b/drivers/char/tpm/tpm2-sessions.c
> @@ -232,7 +232,7 @@ void tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
> #endif
>
> if (!tpm2_chip_auth(chip)) {
> - tpm_buf_append_handle(chip, buf, handle);
> + tpm_buf_append_handle(buf, handle);
> return;
> }
>
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index 5283f32781c4..b2d89df70c18 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -423,7 +423,7 @@ void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value);
> u8 tpm_buf_read_u8(struct tpm_buf *buf, off_t *offset);
> u16 tpm_buf_read_u16(struct tpm_buf *buf, off_t *offset);
> u32 tpm_buf_read_u32(struct tpm_buf *buf, off_t *offset);
> -void tpm_buf_append_handle(struct tpm_chip *chip, struct tpm_buf *buf, u32 handle);
> +void tpm_buf_append_handle(struct tpm_buf *buf, u32 handle);
>
> /*
> * Check if TPM device is in the firmware upgrade mode.
> --
> 2.39.5
>
>
J.
--
101 things you can't have too much of : 24 - Taglines.
This .sig brought to you by the letter X and the number 45
Product of the Republic of HuggieTag
^ permalink raw reply
* Re: [PATCH v3 07/10] tpm-buf: check for corruption in tpm_buf_append_handle()
From: Jonathan McDowell @ 2025-09-30 11:13 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: linux-integrity, dpsmith, ross.philipson, Jarkko Sakkinen,
Peter Huewe, Jason Gunthorpe, David Howells, Paul Moore,
James Morris, Serge E. Hallyn, James Bottomley, Mimi Zohar,
open list, open list:KEYS/KEYRINGS, open list:SECURITY SUBSYSTEM
In-Reply-To: <20250929194832.2913286-8-jarkko@kernel.org>
On Mon, Sep 29, 2025 at 10:48:29PM +0300, Jarkko Sakkinen wrote:
> From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
>
> Unify TPM_BUF_BOUNDARY_ERROR and TPM_BUF_OVERFLOW into TPM_BUF_INVALID
> flag because semantically they are identical.
>
> Test and set TPM_BUF_INVALID in tpm_buf_append_handle() following the
> pattern from other functions in tpm-buf.c.
>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
Reviewed-by: Jonathan McDowell <noodles@meta.com>
> ---
> v3:
> - No changes.
> v2:
> - A new patch.
> ---
> drivers/char/tpm/tpm-buf.c | 14 ++++++++------
> include/linux/tpm.h | 8 +++-----
> security/keys/trusted-keys/trusted_tpm2.c | 6 +++---
> 3 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
> index dc882fc9fa9e..69ee77400539 100644
> --- a/drivers/char/tpm/tpm-buf.c
> +++ b/drivers/char/tpm/tpm-buf.c
> @@ -104,13 +104,12 @@ EXPORT_SYMBOL_GPL(tpm_buf_length);
> */
> void tpm_buf_append(struct tpm_buf *buf, const u8 *new_data, u16 new_length)
> {
> - /* Return silently if overflow has already happened. */
> - if (buf->flags & TPM_BUF_OVERFLOW)
> + if (buf->flags & TPM_BUF_INVALID)
> return;
>
> if ((buf->length + new_length) > PAGE_SIZE) {
> WARN(1, "tpm_buf: write overflow\n");
> - buf->flags |= TPM_BUF_OVERFLOW;
> + buf->flags |= TPM_BUF_INVALID;
> return;
> }
>
> @@ -157,8 +156,12 @@ EXPORT_SYMBOL_GPL(tpm_buf_append_u32);
> */
> void tpm_buf_append_handle(struct tpm_chip *chip, struct tpm_buf *buf, u32 handle)
> {
> + if (buf->flags & TPM_BUF_INVALID)
> + return;
> +
> if (buf->flags & TPM_BUF_TPM2B) {
> dev_err(&chip->dev, "Invalid buffer type (TPM2B)\n");
> + buf->flags |= TPM_BUF_INVALID;
> return;
> }
>
> @@ -177,14 +180,13 @@ static void tpm_buf_read(struct tpm_buf *buf, off_t *offset, size_t count, void
> {
> off_t next_offset;
>
> - /* Return silently if overflow has already happened. */
> - if (buf->flags & TPM_BUF_BOUNDARY_ERROR)
> + if (buf->flags & TPM_BUF_INVALID)
> return;
>
> next_offset = *offset + count;
> if (next_offset > buf->length) {
> WARN(1, "tpm_buf: read out of boundary\n");
> - buf->flags |= TPM_BUF_BOUNDARY_ERROR;
> + buf->flags |= TPM_BUF_INVALID;
> return;
> }
>
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index e72e7657faa2..5283f32781c4 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -366,12 +366,10 @@ struct tpm_header {
> } __packed;
>
> enum tpm_buf_flags {
> - /* the capacity exceeded: */
> - TPM_BUF_OVERFLOW = BIT(0),
> /* TPM2B format: */
> - TPM_BUF_TPM2B = BIT(1),
> - /* read out of boundary: */
> - TPM_BUF_BOUNDARY_ERROR = BIT(2),
> + TPM_BUF_TPM2B = BIT(0),
> + /* The buffer is in invalid and unusable state: */
> + TPM_BUF_INVALID = BIT(1),
> };
>
> /*
> diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
> index 8e3b283a59b2..119d5152c0db 100644
> --- a/security/keys/trusted-keys/trusted_tpm2.c
> +++ b/security/keys/trusted-keys/trusted_tpm2.c
> @@ -295,7 +295,7 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
> /* creation PCR */
> tpm_buf_append_u32(&buf, 0);
>
> - if (buf.flags & TPM_BUF_OVERFLOW) {
> + if (buf.flags & TPM_BUF_INVALID) {
> rc = -E2BIG;
> tpm2_end_auth_session(chip);
> goto out;
> @@ -308,7 +308,7 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
> goto out;
>
> blob_len = tpm_buf_read_u32(&buf, &offset);
> - if (blob_len > MAX_BLOB_SIZE || buf.flags & TPM_BUF_BOUNDARY_ERROR) {
> + if (blob_len > MAX_BLOB_SIZE || buf.flags & TPM_BUF_INVALID) {
> rc = -E2BIG;
> goto out;
> }
> @@ -414,7 +414,7 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
>
> tpm_buf_append(&buf, blob, blob_len);
>
> - if (buf.flags & TPM_BUF_OVERFLOW) {
> + if (buf.flags & TPM_BUF_INVALID) {
> rc = -E2BIG;
> tpm2_end_auth_session(chip);
> goto out;
> --
> 2.39.5
>
>
J.
--
Web [ Reality is for people with no grasp of fantasy. ]
site: https:// [ ] Made by
www.earth.li/~noodles/ [ ] HuggieTag 0.0.24
^ permalink raw reply
* Re: [PATCH v3 05/10] tpm2-sessions: Umask tpm_buf_append_hmac_session()
From: Jonathan McDowell @ 2025-09-30 11:11 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: linux-integrity, dpsmith, ross.philipson, Jarkko Sakkinen,
Peter Huewe, Jason Gunthorpe, David Howells, Paul Moore,
James Morris, Serge E. Hallyn, James Bottomley, Mimi Zohar,
open list, open list:KEYS/KEYRINGS, open list:SECURITY SUBSYSTEM
In-Reply-To: <20250929194832.2913286-6-jarkko@kernel.org>
On Mon, Sep 29, 2025 at 10:48:27PM +0300, Jarkko Sakkinen wrote:
> From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
>
> Open code tpm_buf_append_hmac_session_opt() in order to unmask the code
> paths in the call sites of tpm_buf_append_hmac_session().
>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
Reviewed-by: Jonathan McDowell <noodles@meta.com>
> ---
> v3:
> - No changes.
> v2:
> - Uncorrupt the patch.
> ---
> drivers/char/tpm/tpm2-cmd.c | 14 +++++++++++---
> include/linux/tpm.h | 23 -----------------------
> security/keys/trusted-keys/trusted_tpm2.c | 12 ++++++++++--
> 3 files changed, 21 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index 86b1a4d859b9..c7bfa705ea8f 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -257,9 +257,17 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max)
>
> do {
> tpm_buf_reset(&buf, TPM2_ST_SESSIONS, TPM2_CC_GET_RANDOM);
> - tpm_buf_append_hmac_session_opt(chip, &buf, TPM2_SA_ENCRYPT
> - | TPM2_SA_CONTINUE_SESSION,
> - NULL, 0);
> + if (tpm2_chip_auth(chip)) {
> + tpm_buf_append_hmac_session(chip, &buf,
> + TPM2_SA_ENCRYPT |
> + TPM2_SA_CONTINUE_SESSION,
> + NULL, 0);
> + } else {
> + offset = buf.handles * 4 + TPM_HEADER_SIZE;
> + head = (struct tpm_header *)buf.data;
> + if (tpm_buf_length(&buf) == offset)
> + head->tag = cpu_to_be16(TPM2_ST_NO_SESSIONS);
> + }
> tpm_buf_append_u16(&buf, num_bytes);
> tpm_buf_fill_hmac_session(chip, &buf);
> err = tpm_transmit_cmd(chip, &buf,
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index 1fa02e18e688..e72e7657faa2 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -532,29 +532,6 @@ void tpm_buf_append_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf,
> int passphraselen);
> void tpm_buf_append_auth(struct tpm_chip *chip, struct tpm_buf *buf,
> u8 *passphrase, int passphraselen);
> -static inline void tpm_buf_append_hmac_session_opt(struct tpm_chip *chip,
> - struct tpm_buf *buf,
> - u8 attributes,
> - u8 *passphrase,
> - int passphraselen)
> -{
> - struct tpm_header *head;
> - int offset;
> -
> - if (tpm2_chip_auth(chip)) {
> - tpm_buf_append_hmac_session(chip, buf, attributes, passphrase, passphraselen);
> - } else {
> - offset = buf->handles * 4 + TPM_HEADER_SIZE;
> - head = (struct tpm_header *)buf->data;
> -
> - /*
> - * If the only sessions are optional, the command tag must change to
> - * TPM2_ST_NO_SESSIONS.
> - */
> - if (tpm_buf_length(buf) == offset)
> - head->tag = cpu_to_be16(TPM2_ST_NO_SESSIONS);
> - }
> -}
>
> #ifdef CONFIG_TCG_TPM2_HMAC
>
> diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
> index e165b117bbca..c414a7006d78 100644
> --- a/security/keys/trusted-keys/trusted_tpm2.c
> +++ b/security/keys/trusted-keys/trusted_tpm2.c
> @@ -482,8 +482,10 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,
> struct trusted_key_options *options,
> u32 blob_handle)
> {
> + struct tpm_header *head;
> struct tpm_buf buf;
> u16 data_len;
> + int offset;
> u8 *data;
> int rc;
>
> @@ -518,8 +520,14 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,
> tpm2_buf_append_auth(&buf, options->policyhandle,
> NULL /* nonce */, 0, 0,
> options->blobauth, options->blobauth_len);
> - tpm_buf_append_hmac_session_opt(chip, &buf, TPM2_SA_ENCRYPT,
> - NULL, 0);
> + if (tpm2_chip_auth(chip)) {
> + tpm_buf_append_hmac_session(chip, &buf, TPM2_SA_ENCRYPT, NULL, 0);
> + } else {
> + offset = buf.handles * 4 + TPM_HEADER_SIZE;
> + head = (struct tpm_header *)buf.data;
> + if (tpm_buf_length(&buf) == offset)
> + head->tag = cpu_to_be16(TPM2_ST_NO_SESSIONS);
> + }
> }
>
> tpm_buf_fill_hmac_session(chip, &buf);
> --
> 2.39.5
>
>
J.
--
... I love you the way a bomb loves a crowd.
^ permalink raw reply
* Re: [PATCH v3 04/10] tpm2-sessions: Remove 'attributes' from tpm_buf_append_auth
From: Jonathan McDowell @ 2025-09-30 11:10 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: linux-integrity, dpsmith, ross.philipson, Jarkko Sakkinen,
Peter Huewe, Jason Gunthorpe, David Howells, Paul Moore,
James Morris, Serge E. Hallyn, Roberto Sassu, Mimi Zohar,
open list, open list:KEYS/KEYRINGS, open list:SECURITY SUBSYSTEM
In-Reply-To: <20250929194832.2913286-5-jarkko@kernel.org>
On Mon, Sep 29, 2025 at 10:48:26PM +0300, Jarkko Sakkinen wrote:
> From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
>
> In a previous bug fix, 'attributes' was added by mistake to
> tpm_buf_append_auth(). Remove the parameter.
>
> Fixes: 27184f8905ba ("tpm: Opt-in in disable PCR integrity protection")
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
Reviewed-by: Jonathan McDowell <noodles@meta.com>
> ---
> v3:
> - No changes
> v2:
> - Uncorrupt the patch.
> ---
> drivers/char/tpm/tpm2-cmd.c | 2 +-
> drivers/char/tpm/tpm2-sessions.c | 5 ++---
> include/linux/tpm.h | 2 +-
> 3 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index a7cddd4b5626..86b1a4d859b9 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -191,7 +191,7 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
> tpm_buf_append_hmac_session(chip, &buf, 0, NULL, 0);
> } else {
> tpm_buf_append_handle(chip, &buf, pcr_idx);
> - tpm_buf_append_auth(chip, &buf, 0, NULL, 0);
> + tpm_buf_append_auth(chip, &buf, NULL, 0);
> }
>
> tpm_buf_append_u32(&buf, chip->nr_allocated_banks);
> diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
> index 6d03c224e6b2..13f019d1312a 100644
> --- a/drivers/char/tpm/tpm2-sessions.c
> +++ b/drivers/char/tpm/tpm2-sessions.c
> @@ -266,7 +266,7 @@ void tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
> EXPORT_SYMBOL_GPL(tpm_buf_append_name);
>
> void tpm_buf_append_auth(struct tpm_chip *chip, struct tpm_buf *buf,
> - u8 attributes, u8 *passphrase, int passphrase_len)
> + u8 *passphrase, int passphrase_len)
> {
> /* offset tells us where the sessions area begins */
> int offset = buf->handles * 4 + TPM_HEADER_SIZE;
> @@ -327,8 +327,7 @@ void tpm_buf_append_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf,
> #endif
>
> if (!tpm2_chip_auth(chip)) {
> - tpm_buf_append_auth(chip, buf, attributes, passphrase,
> - passphrase_len);
> + tpm_buf_append_auth(chip, buf, passphrase, passphrase_len);
> return;
> }
>
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index 51846317d662..1fa02e18e688 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -531,7 +531,7 @@ void tpm_buf_append_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf,
> u8 attributes, u8 *passphrase,
> int passphraselen);
> void tpm_buf_append_auth(struct tpm_chip *chip, struct tpm_buf *buf,
> - u8 attributes, u8 *passphrase, int passphraselen);
> + u8 *passphrase, int passphraselen);
> static inline void tpm_buf_append_hmac_session_opt(struct tpm_chip *chip,
> struct tpm_buf *buf,
> u8 attributes,
> --
> 2.39.5
>
>
J.
--
101 things you can't have too | .''`. Debian GNU/Linux Developer
much of : 17 - Money. | : :' : Happy to accept PGP signed
| `. `' or encrypted mail - RSA
| `- key on the keyservers.
^ permalink raw reply
* Re: [PATCH v3 01/10] tpm: Cap the number of PCR banks
From: Jonathan McDowell @ 2025-09-30 11:09 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: linux-integrity, dpsmith, ross.philipson, Jarkko Sakkinen,
Roberto Sassu, Peter Huewe, Jason Gunthorpe, David Howells,
Paul Moore, James Morris, Serge E. Hallyn, open list,
open list:KEYS/KEYRINGS, open list:SECURITY SUBSYSTEM
In-Reply-To: <20250929194832.2913286-2-jarkko@kernel.org>
On Mon, Sep 29, 2025 at 10:48:23PM +0300, Jarkko Sakkinen wrote:
> From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
>
> tpm2_get_pcr_allocation() does not cap any upper limit for the number of
> banks. Cap the limit to four banks so that out of bounds values coming
> from external I/O cause on only limited harm.
>
> Cc: Roberto Sassu <roberto.sassu@huawei.com>
> Fixes: bcfff8384f6c ("tpm: dynamically allocate the allocated_banks array")
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
> ---
> v3:
> - Wrote a more clear commit message.
> - Fixed pr_err() message.
> v2:
> - A new patch.
> ---
> drivers/char/tpm/tpm-chip.c | 13 +++++++++----
> drivers/char/tpm/tpm.h | 1 -
> drivers/char/tpm/tpm1-cmd.c | 25 -------------------------
> drivers/char/tpm/tpm2-cmd.c | 8 +++-----
> include/linux/tpm.h | 18 ++++++++----------
> 5 files changed, 20 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 687f6d8cd601..9a6538f76f50 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -559,14 +559,19 @@ static int tpm_add_hwrng(struct tpm_chip *chip)
>
> static int tpm_get_pcr_allocation(struct tpm_chip *chip)
> {
> - int rc;
> + int rc = 0;
>
> if (tpm_is_firmware_upgrade(chip))
> return 0;
>
> - rc = (chip->flags & TPM_CHIP_FLAG_TPM2) ?
> - tpm2_get_pcr_allocation(chip) :
> - tpm1_get_pcr_allocation(chip);
> + if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) {
> + chip->allocated_banks[0].alg_id = TPM_ALG_SHA1;
> + chip->allocated_banks[0].digest_size = hash_digest_size[HASH_ALGO_SHA1];
> + chip->allocated_banks[0].crypto_id = HASH_ALGO_SHA1;
> + chip->nr_allocated_banks = 1;
> + } else {
> + rc = tpm2_get_pcr_allocation(chip);
> + }
>
> if (rc > 0)
> return -ENODEV;
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 57ef8589f5f5..769fa6b00c54 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -252,7 +252,6 @@ int tpm1_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8 *res_buf);
> ssize_t tpm1_getcap(struct tpm_chip *chip, u32 subcap_id, cap_t *cap,
> const char *desc, size_t min_cap_length);
> int tpm1_get_random(struct tpm_chip *chip, u8 *out, size_t max);
> -int tpm1_get_pcr_allocation(struct tpm_chip *chip);
> unsigned long tpm_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
> int tpm_pm_suspend(struct device *dev);
> int tpm_pm_resume(struct device *dev);
> diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
> index cf64c7385105..5c49bdff33de 100644
> --- a/drivers/char/tpm/tpm1-cmd.c
> +++ b/drivers/char/tpm/tpm1-cmd.c
> @@ -786,28 +786,3 @@ int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr)
>
> return rc;
> }
> -
> -/**
> - * tpm1_get_pcr_allocation() - initialize the allocated bank
> - * @chip: TPM chip to use.
> - *
> - * The function initializes the SHA1 allocated bank to extend PCR
> - *
> - * Return:
> - * * 0 on success,
> - * * < 0 on error.
> - */
> -int tpm1_get_pcr_allocation(struct tpm_chip *chip)
> -{
> - chip->allocated_banks = kcalloc(1, sizeof(*chip->allocated_banks),
> - GFP_KERNEL);
> - if (!chip->allocated_banks)
> - return -ENOMEM;
> -
> - chip->allocated_banks[0].alg_id = TPM_ALG_SHA1;
> - chip->allocated_banks[0].digest_size = hash_digest_size[HASH_ALGO_SHA1];
> - chip->allocated_banks[0].crypto_id = HASH_ALGO_SHA1;
> - chip->nr_allocated_banks = 1;
> -
> - return 0;
> -}
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index 7d77f6fbc152..a7cddd4b5626 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -538,11 +538,9 @@ ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>
> nr_possible_banks = be32_to_cpup(
> (__be32 *)&buf.data[TPM_HEADER_SIZE + 5]);
> -
> - chip->allocated_banks = kcalloc(nr_possible_banks,
> - sizeof(*chip->allocated_banks),
> - GFP_KERNEL);
> - if (!chip->allocated_banks) {
> + if (nr_possible_banks > TPM2_MAX_BANKS) {
> + pr_err("tpm: unexpected number of banks: %u > %u",
> + nr_possible_banks, TPM2_MAX_BANKS);
> rc = -ENOMEM;
> goto out;
> }
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index 900c81a2bc41..fc7df87dfb9a 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -27,7 +27,12 @@
> #include <crypto/aes.h>
>
> #define TPM_DIGEST_SIZE 20 /* Max TPM v1.2 PCR size */
> -#define TPM_MAX_DIGEST_SIZE SHA512_DIGEST_SIZE
> +#define TPM_HEADER_SIZE 10
> +
> +#define TPM2_PLATFORM_PCR 24
> +#define TPM2_PCR_SELECT_MIN 3
By changing this to 3 we lose the fact it's related to TPM2_PLATFORM_PCR
- it's the number of bytes required to hold a bitmap with at least
TPM2_PLATFORM_PCR entries. Can we at least have a comment about that
fact?
> +#define TPM2_MAX_DIGEST_SIZE SHA512_DIGEST_SIZE
> +#define TPM2_MAX_BANKS 4
Where does this max come from? It matches what I see with swtpm by
default (SHA1, SHA2-256, SHA2-384, SHA-512), so I haven't seen anything
that exceeds it myself.
> struct tpm_chip;
> struct trusted_key_payload;
> @@ -69,7 +74,7 @@ enum tpm2_curves {
>
> struct tpm_digest {
> u16 alg_id;
> - u8 digest[TPM_MAX_DIGEST_SIZE];
> + u8 digest[TPM2_MAX_DIGEST_SIZE];
> } __packed;
>
> struct tpm_bank_info {
> @@ -190,7 +195,7 @@ struct tpm_chip {
> unsigned int groups_cnt;
>
> u32 nr_allocated_banks;
> - struct tpm_bank_info *allocated_banks;
> + struct tpm_bank_info allocated_banks[TPM2_MAX_BANKS];
> #ifdef CONFIG_ACPI
> acpi_handle acpi_dev_handle;
> char ppi_version[TPM_PPI_VERSION_LEN + 1];
> @@ -217,13 +222,6 @@ struct tpm_chip {
> #endif
> };
>
> -#define TPM_HEADER_SIZE 10
> -
> -enum tpm2_const {
> - TPM2_PLATFORM_PCR = 24,
> - TPM2_PCR_SELECT_MIN = ((TPM2_PLATFORM_PCR + 7) / 8),
> -};
> -
> enum tpm2_timeouts {
> TPM2_TIMEOUT_A = 750,
> TPM2_TIMEOUT_B = 4000,
> --
> 2.39.5
>
>
J.
--
"How the f**k did you work that out?" -- Pythagoras
^ permalink raw reply
* linux-6.17/tools/testing/selftests/landlock/fs_test.c:5631: Test for pointer < 0 ?
From: David Binderman @ 2025-09-30 9:57 UTC (permalink / raw)
To: mic@digikod.net, gnoack@google.com, shuah@kernel.org,
linux-security-module@vger.kernel.org,
linux-kselftest@vger.kernel.org, LKML
Hello there,
Static analyser cppcheck says:
linux-6.17/tools/testing/selftests/landlock/fs_test.c:5631:23: style: A pointer can not be negative so it is either pointless or an error to check if it is. [pointerLessThanZero]
Source code is
if (log_match_cursor < 0)
return (long long)log_match_cursor;
but
char *log_match_cursor = log_match;
Suggest remove code.
Regards
David Binderman
^ permalink raw reply
* Re: [PATCH] ima: setting security.ima to fix security.evm for a file with IMA signature
From: Coiby Xu @ 2025-09-30 2:31 UTC (permalink / raw)
To: Mimi Zohar
Cc: linux-integrity, Roberto Sassu, Dmitry Kasatkin, Eric Snowberg,
Paul Moore, James Morris, Serge E. Hallyn,
open list:SECURITY SUBSYSTEM, open list
In-Reply-To: <cbcbdb3e4aed17f387ae1d357906796551e2f470.camel@linux.ibm.com>
On Wed, Sep 10, 2025 at 07:15:19AM -0400, Mimi Zohar wrote:
>On Wed, 2025-09-10 at 09:20 +0800, Coiby Xu wrote:
>> On Tue, Sep 09, 2025 at 11:31:20AM -0400, Mimi Zohar wrote:
>> > On Tue, 2025-09-09 at 12:19 +0800, Coiby Xu wrote:
>> > > When both IMA and EVM fix modes are enabled, accessing a file with IMA
>> > > signature won't cause security.evm to be fixed. But this doesn't happen
>> > > to a file with correct IMA hash already set because accessing it will
>> > > cause setting security.ima again which triggers fixing security.evm
>> > > thanks to security_inode_post_setxattr->evm_update_evmxattr.
>> > >
>> > > Let's use the same mechanism to fix security.evm for a file with IMA
>> > > signature.
>> > >
>> > > Signed-off-by: Coiby Xu <coxu@redhat.com>
>> >
>> > Agreed, re-writing the file signature stored as security.ima would force
>> > security.evm to be updated.
>> >
>> > Unfortunately, I'm missing something. ima_appraise_measurement() first verifies
>> > the existing security.evm xattr, before verifying the security.ima xattr. If
>> > the EVM HMAC fails to verify, it immediately exits ima_appraise_measurement().
>> > security.ima in this case is never verified.
>> >
>> > This patch seems to address the case where the existing security.evm is valid,
>> > but the file signature stored in security.ima is invalid. (To get to the new
>> > code, the "status" flag is not INTEGRITY_PASS.) Re-writing the same invalid
>> > file signature would solve an invalid security.evm, but not an invalid IMA file
>> > signature. What am I missing?
>>
>> Hi, Mimi,
>>
>> Thanks for raising the question! This patch is to address the case where
>> IMA signature is already added but security.evm doesn't yet exist. So
>> EVM HMAC fails to verify but there is no exiting
>> ima_appraise_measurement immediately.
>>
>> And you are right that re-writing an invalid IMA file won't fix an
>> invalid IMA file signature. And even when IMA signature is valid, the
>> verification may fail because the key is missing from .ima keyring. This
>> happens because we need to turn off secure boot to enable fix mode. As a
>> result, CA keys won't be loaded into .machine keyring.
>
>> Btw, if I'm not
>> mistaken, current IMA code assumes we are not supposed to fix IMA file
>> signature.
>
>Right, unlike file hashes, new or the same file signature can be written, but
>cannot be "fixed" in the literal sense, as that would require calculating the
>file hash and signing it with a private key.
Thanks for the confirmation! I also added some code comments to explain
the IMA iint cache atomic_flags including IMA_DIGSIG in v2.
>
>This patch triggers "fixing" the EVM HMAC by re-writing the existing IMA file
>signature. I assume the same result could be achieved by simply re-installing
>the file signature. In both cases, the EVM HMAC key needs to exist and be
>loaded.
--
Best regards,
Coiby
^ permalink raw reply
* Re: [PATCH] ima: setting security.ima to fix security.evm for a file with IMA signature
From: Coiby Xu @ 2025-09-30 2:28 UTC (permalink / raw)
To: Mimi Zohar
Cc: linux-integrity, Roberto Sassu, Dmitry Kasatkin, Eric Snowberg,
Paul Moore, James Morris, Serge E. Hallyn,
open list:SECURITY SUBSYSTEM, open list
In-Reply-To: <e74e81465e168a7f43583d5783850cc5fe7ca1c5.camel@linux.ibm.com>
On Wed, Sep 24, 2025 at 11:27:03PM -0400, Mimi Zohar wrote:
>On Wed, 2025-09-24 at 16:03 +0800, Coiby Xu wrote:
>> On Mon, Sep 15, 2025 at 05:05:42PM -0400, Mimi Zohar wrote:
>> > On Wed, 2025-09-10 at 09:20 +0800, Coiby Xu wrote:
>> > > On Tue, Sep 09, 2025 at 11:31:20AM -0400, Mimi Zohar wrote:
>> > > > On Tue, 2025-09-09 at 12:19 +0800, Coiby Xu wrote:
>> > > > > When both IMA and EVM fix modes are enabled, accessing a file with IMA
>> > > > > signature won't cause security.evm to be fixed. But this doesn't happen
>> > > > > to a file with correct IMA hash already set because accessing it will
>> > > > > cause setting security.ima again which triggers fixing security.evm
>> > > > > thanks to security_inode_post_setxattr->evm_update_evmxattr.
>> > > > >
>> > > > > Let's use the same mechanism to fix security.evm for a file with IMA
>> > > > > signature.
>> > > > >
>> [...]
>> > > > > ---
>> > > > > security/integrity/ima/ima_appraise.c | 27 +++++++++++++++++++++------
>> > > > > 1 file changed, 21 insertions(+), 6 deletions(-)
>> > > > >
>> > > > > diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
>> > > > > index f435eff4667f..18c3907c5e44 100644
>> > > > > --- a/security/integrity/ima/ima_appraise.c
>> > > > > +++ b/security/integrity/ima/ima_appraise.c
>> > > > > @@ -595,12 +595,27 @@ int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint,
>> > > > > integrity_audit_msg(audit_msgno, inode, filename,
>> > > > > op, cause, rc, 0);
>> > > > > } else if (status != INTEGRITY_PASS) {
>> > > > > - /* Fix mode, but don't replace file signatures. */
>> > > > > - if ((ima_appraise & IMA_APPRAISE_FIX) && !try_modsig &&
>> > > > > - (!xattr_value ||
>> > > > > - xattr_value->type != EVM_IMA_XATTR_DIGSIG)) {
>> > > > > - if (!ima_fix_xattr(dentry, iint))
>> > > > > - status = INTEGRITY_PASS;
>> > > > > + /*
>> > > > > + * Fix mode, but don't replace file signatures.
>> > > > > + *
>> > > > > + * When EVM fix mode is also enabled, security.evm will be
>> > > > > + * fixed automatically when security.ima is set because of
>> > > > > + * security_inode_post_setxattr->evm_update_evmxattr.
>> > > > > + */
>> > > > > + if ((ima_appraise & IMA_APPRAISE_FIX) && !try_modsig) {
>> > > > > + if (!xattr_value ||
>> > > > > + xattr_value->type != EVM_IMA_XATTR_DIGSIG) {
>> > > > > + if (ima_fix_xattr(dentry, iint))
>> > > > > + status = INTEGRITY_PASS;
>> > > > > + } else if (xattr_value->type == EVM_IMA_XATTR_DIGSIG &&
>> > > > > + evm_revalidate_status(XATTR_NAME_IMA)) {
>> > > > > + if (!__vfs_setxattr_noperm(&nop_mnt_idmap,
>> > > > > + dentry,
>> > > > > + XATTR_NAME_IMA,
>> > > > > + xattr_value,
>> > > > > + xattr_len, 0))
>> > > > > + status = INTEGRITY_PASS;
>> > > > > + }
>> > > > > }
>> >
>> > Instead of re-writing the IMA signature without a clear explanation, define a
>> > new EVM function named evm_fix_hmac() and add a call here in IMA. Only in EVM
>> > fix mode would evm_fix_hmac() update the EVM hmac.
>> >
>> > } else if (status != INTEGRITY_PASS) {
>> > /*
>> > * IMA fix mode updates the IMA file hash, which triggers EVM
>> > * to update security.evm. ....
>> > *
>> > * Similarly, trigger fixing EVM HMAC for IMA file signatures.
>> > */
>> > if ((ima_appraise & IMA_APPRAISE_FIX) && !try_modsig) {
>> > if (!xattr_value ||
>> > xattr_value->type != EVM_IMA_XATTR_DIGSIG) {
>> > if (ima_fix_xattr(dentry, iint))
>> > status = INTEGRITY_PASS;
>> > } else if (status == INTEGRITY_NOLABEL) {
>> > evm_fix_hmac(dentry, XATTR_NAME_IMA, ....);
>> > }
>> > }
>>
>> Thanks for the advice! I wonder if we should use existing
>> evm_update_evmxattr instead of defining a new EVM function.
>>
>> /*
>> * Calculate the hmac and update security.evm xattr
>> *
>> * Expects to be called with i_mutex locked.
>> */
>> int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name,
>> const char *xattr_value, size_t xattr_value_len)
>> {
>> }
>>
>>
>> I already tried evm_update_evmxattr and can confirm it works. But later
>> I switched to __vfs_setxattr_noperm because I thought it's consistent
>> with current logic of adding security.evm when there is already correct
>> security.ima and it's a slightly smaller change.
>
>Calling evm_inode_updatexattr() is limited to EVM. Only after verifying the
>existing EVM value is evm_inode_updatexattr() called. For example, in
>evm_inode_setxattr() the existing EVM value is verified and then updated in
>evm_inode_post_setxattr(), by calling evm_inode_updatexattr().
>
>In this case, the new function evm_fix_hmac() would call evm_update_evmxattr()
>only after verifying the EVM mode is set to fix.
Thanks for the clarification! I've sent v2 to add the new function
evm_fix_hmac.
--
Best regards,
Coiby
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox