* Re: linux-next: umh: fix processed error when UMH_WAIT_PROC is used seems to break linux bridge on s390x (bisected)
From: Christian Borntraeger @ 2020-06-25 13:26 UTC (permalink / raw)
To: Luis Chamberlain
Cc: Christoph Hellwig, ast, axboe, bfields, bridge, chainsaw,
christian.brauner, chuck.lever, davem, dhowells, gregkh,
jarkko.sakkinen, jmorris, josh, keescook, keyrings, kuba,
lars.ellenberg, linux-fsdevel, linux-kernel, linux-nfs,
linux-security-module, nikolay, philipp.reisner, ravenexp, roopa,
serge, slyfox, viro, yangtiezhu, netdev, markward, linux-s390
In-Reply-To: <1263e370-7cee-24d8-b98c-117bf7c90a83@de.ibm.com>
On 24.06.20 20:37, Christian Borntraeger wrote:
>
>
> On 24.06.20 20:32, Christian Borntraeger wrote:
> [...]>
>> So the translations look correct. But your change is actually a sematic change
>> if(ret) will only trigger if there is an error
>> if (KWIFEXITED(ret)) will always trigger when the process ends. So we will always overwrite -ECHILD
>> and we did not do it before.
>>
>
> So the right fix is
>
> diff --git a/kernel/umh.c b/kernel/umh.c
> index f81e8698e36e..a3a3196e84d1 100644
> --- a/kernel/umh.c
> +++ b/kernel/umh.c
> @@ -154,7 +154,7 @@ static void call_usermodehelper_exec_sync(struct subprocess_info *sub_info)
> * the real error code is already in sub_info->retval or
> * sub_info->retval is 0 anyway, so don't mess with it then.
> */
> - if (KWIFEXITED(ret))
> + if (KWEXITSTATUS(ret))
> sub_info->retval = KWEXITSTATUS(ret);
> }
Ping. Shall I send this as a proper patch or are we merging this fixup in Andrews patch queue?
^ permalink raw reply
* Re: [RFC][PATCH] net/bpfilter: Remove this broken and apparently unmantained
From: Greg Kroah-Hartman @ 2020-06-25 13:25 UTC (permalink / raw)
To: Stephen Smalley
Cc: Tetsuo Handa, Alexei Starovoitov, Eric W. Biederman,
Linus Torvalds, Kees Cook, Andrew Morton, Alexei Starovoitov,
David Miller, Al Viro, bpf, linux-fsdevel, Daniel Borkmann,
Jakub Kicinski, Masahiro Yamada, Gary Lin, Bruno Meneguele,
linux-security-module, Casey Schaufler
In-Reply-To: <CAEjxPJ4e9rWWssp0CyM7GM7NP_QKkswHK7URwLZFqo5+wGecQw@mail.gmail.com>
On Thu, Jun 25, 2020 at 08:56:10AM -0400, Stephen Smalley wrote:
> On Wed, Jun 24, 2020 at 7:16 PM Tetsuo Handa
> <penguin-kernel@i-love.sakura.ne.jp> wrote:
> > What is unhappy for pathname based LSMs is that fork_usermode_blob() creates
> > a file with empty filename. I can imagine that somebody would start abusing
> > fork_usermode_blob() as an interface for starting programs like modprobe, hotplug,
> > udevd and sshd. When such situation happened, how fork_usermode_blob() provides
> > information for identifying the intent of such execve() requests?
> >
> > fork_usermode_blob() might also be an unhappy behavior for inode based LSMs (like
> > SELinux and Smack) because it seems that fork_usermode_blob() can't have a chance
> > to associate appropriate security labels based on the content of the byte array
> > because files are created on-demand. Is fork_usermode_blob() friendly to inode
> > based LSMs?
>
> No, because we cannot label the inode based on the program's purpose
> and therefore cannot configure an automatic transition to a suitable
> security context for the process, unlike call_usermodehelper().
Why, what prevents this? Can you not just do that based on the "blob
address" or signature of it or something like that? Right now you all
do this based on inode of a random file on a disk, what's the difference
between a random blob in memory?
> It is
> important to note that the goal of such transitions is not merely to
> restrict the program from doing bad things but also to protect the
> program from untrustworthy inputs, e.g. one can run kmod/modprobe in a
> domain that can only read from authorized kernel modules, prevent
> following untrusted symlinks, etc. Further, at present, the
> implementation creates the inode via shmem_kernel_file_setup(), which
> is supposed to be for inodes private to the kernel not exposed to
> userspace (hence marked S_PRIVATE), which I believe in this case will
> end up leaving the inode unlabeled but still end up firing checks in
> the bprm hooks on the file inode, thereby potentially yielding denials
> in SELinux on the exec of unlabeled files. Not exactly what we would
> want. If users were to switch from using call_usermodehelper() to
> fork_usermode_blob() we would need them to label the inode in some
> manner to reflect the program purpose prior to exec. I suppose they
> could pass in some string key and SELinux could look it up in policy
> to get a context to use or something.
Sure, that would work.
> On a different note, will the usermode blob be measured by IMA prior
> to execution? What ensures that the blob was actually embedded in the
> kernel image and wasn't just supplied as data through exploitation of
> a kernel vulnerability or malicious kernel module?
No reason it couldn't be passed to IMA for measuring, if people want to
do that.
thanks,
greg k-h
^ permalink raw reply
* Re: [RFC][PATCH] net/bpfilter: Remove this broken and apparently unmantained
From: Stephen Smalley @ 2020-06-25 12:56 UTC (permalink / raw)
To: Tetsuo Handa, Greg Kroah-Hartman
Cc: Alexei Starovoitov, Eric W. Biederman, Linus Torvalds, Kees Cook,
Andrew Morton, Alexei Starovoitov, David Miller, Al Viro, bpf,
linux-fsdevel, Daniel Borkmann, Jakub Kicinski, Masahiro Yamada,
Gary Lin, Bruno Meneguele, linux-security-module, Casey Schaufler
In-Reply-To: <2f55102e-5d11-5569-8248-13618d517e93@i-love.sakura.ne.jp>
On Wed, Jun 24, 2020 at 7:16 PM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
> What is unhappy for pathname based LSMs is that fork_usermode_blob() creates
> a file with empty filename. I can imagine that somebody would start abusing
> fork_usermode_blob() as an interface for starting programs like modprobe, hotplug,
> udevd and sshd. When such situation happened, how fork_usermode_blob() provides
> information for identifying the intent of such execve() requests?
>
> fork_usermode_blob() might also be an unhappy behavior for inode based LSMs (like
> SELinux and Smack) because it seems that fork_usermode_blob() can't have a chance
> to associate appropriate security labels based on the content of the byte array
> because files are created on-demand. Is fork_usermode_blob() friendly to inode
> based LSMs?
No, because we cannot label the inode based on the program's purpose
and therefore cannot configure an automatic transition to a suitable
security context for the process, unlike call_usermodehelper(). It is
important to note that the goal of such transitions is not merely to
restrict the program from doing bad things but also to protect the
program from untrustworthy inputs, e.g. one can run kmod/modprobe in a
domain that can only read from authorized kernel modules, prevent
following untrusted symlinks, etc. Further, at present, the
implementation creates the inode via shmem_kernel_file_setup(), which
is supposed to be for inodes private to the kernel not exposed to
userspace (hence marked S_PRIVATE), which I believe in this case will
end up leaving the inode unlabeled but still end up firing checks in
the bprm hooks on the file inode, thereby potentially yielding denials
in SELinux on the exec of unlabeled files. Not exactly what we would
want. If users were to switch from using call_usermodehelper() to
fork_usermode_blob() we would need them to label the inode in some
manner to reflect the program purpose prior to exec. I suppose they
could pass in some string key and SELinux could look it up in policy
to get a context to use or something.
On a different note, will the usermode blob be measured by IMA prior
to execution? What ensures that the blob was actually embedded in the
kernel image and wasn't just supplied as data through exploitation of
a kernel vulnerability or malicious kernel module? Yes, things are
already bad at that point but it would be good to be able to detect
launch of the malicious userspace payload regardless (kernel exploit
can't undo the measurement extended into the TPM even if it tampers
with the IMA measurement list in the kernel, nor fake a quote signed
by the TPM).
^ permalink raw reply
* [PATCH v6 1/2] acpi: Extend TPM2 ACPI table with missing log fields
From: Stefan Berger @ 2020-06-25 12:42 UTC (permalink / raw)
To: linux-integrity, linux-kernel, jarkko.sakkinen, linux-acpi,
linux-security-module
Cc: Stefan Berger, Rafael J . Wysocki, Jiandi An
In-Reply-To: <20200625124222.1954580-1-stefanb@linux.vnet.ibm.com>
From: Stefan Berger <stefanb@linux.ibm.com>
Recent extensions of the TPM2 ACPI table added 3 more fields
including 12 bytes of start method specific parameters and Log Area
Minimum Length (u32) and Log Area Start Address (u64). So, we define
a new structure acpi_tpm2_phy that holds these optional new fields.
The new fields allow non-UEFI systems to access the TPM2's log.
The specification that has the new fields is the following:
TCG ACPI Specification
Family "1.2" and "2.0"
Version 1.2, Revision 8
https://trustedcomputinggroup.org/wp-content/uploads/TCG_ACPIGeneralSpecification_v1.20_r8.pdf
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
Cc: linux-acpi@vger.kernel.org
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Jiandi An <anjiandi@codeaurora.org>
---
include/acpi/actbl3.h | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/include/acpi/actbl3.h b/include/acpi/actbl3.h
index b0b163b9efc6..bdcac69fa6bd 100644
--- a/include/acpi/actbl3.h
+++ b/include/acpi/actbl3.h
@@ -415,6 +415,13 @@ struct acpi_table_tpm2 {
/* Platform-specific data follows */
};
+/* Optional trailer for revision 4 holding platform-specific data */
+struct acpi_tpm2_phy {
+ u8 start_method_specific[12];
+ u32 log_area_minimum_length;
+ u64 log_area_start_address;
+};
+
/* Values for start_method above */
#define ACPI_TPM2_NOT_ALLOWED 0
--
2.26.2
^ permalink raw reply related
* [PATCH v6 0/2] tpm2: Make TPM2 logs accessible for non-UEFI firmware
From: Stefan Berger @ 2020-06-25 12:42 UTC (permalink / raw)
To: linux-integrity, linux-kernel, jarkko.sakkinen, linux-acpi,
linux-security-module
Cc: Stefan Berger
From: Stefan Berger <stefanb@linux.ibm.com>
This series of patches adds an optional extensions for the TPM2 ACPI table
with additional fields found in the TPM2 TCG ACPI specification (reference
is in the patch) that allow access to the log's address and its size. We
then modify the code that so far only enables access to a TPM 1.2's log for
a TPM2 as well. This then enables access to the TPM2's log on non-UEFI
system that for example run SeaBIOS.
Stefan
v5->v6:
- Moved extensions of TPM2 table into acpi_tpm2_phy.
v4->v5:
- Added R-bs and A-bs.
v3->v4:
- Repost as one series
v2->v3:
- Split the series into two separate patches
- Added comments to ACPI table fields
- Added check for null pointer to log area and zero log size
v1->v2:
- Repost of the series
Stefan Berger (2):
acpi: Extend TPM2 ACPI table with missing log fields
tpm: Add support for event log pointer found in TPM2 ACPI table
drivers/char/tpm/eventlog/acpi.c | 59 ++++++++++++++++++++------------
include/acpi/actbl3.h | 7 ++++
2 files changed, 45 insertions(+), 21 deletions(-)
--
2.26.2
^ permalink raw reply
* [PATCH v6 2/2] tpm: Add support for event log pointer found in TPM2 ACPI table
From: Stefan Berger @ 2020-06-25 12:42 UTC (permalink / raw)
To: linux-integrity, linux-kernel, jarkko.sakkinen, linux-acpi,
linux-security-module
Cc: Stefan Berger
In-Reply-To: <20200625124222.1954580-1-stefanb@linux.vnet.ibm.com>
From: Stefan Berger <stefanb@linux.ibm.com>
In case a TPM2 is attached, search for a TPM2 ACPI table when trying
to get the event log from ACPI. If one is found, use it to get the
start and length of the log area. This allows non-UEFI systems, such
as SeaBIOS, to pass an event log when using a TPM2.
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
drivers/char/tpm/eventlog/acpi.c | 59 ++++++++++++++++++++------------
1 file changed, 38 insertions(+), 21 deletions(-)
diff --git a/drivers/char/tpm/eventlog/acpi.c b/drivers/char/tpm/eventlog/acpi.c
index 63ada5e53f13..8b9e33d57f70 100644
--- a/drivers/char/tpm/eventlog/acpi.c
+++ b/drivers/char/tpm/eventlog/acpi.c
@@ -49,9 +49,9 @@ int tpm_read_log_acpi(struct tpm_chip *chip)
void __iomem *virt;
u64 len, start;
struct tpm_bios_log *log;
-
- if (chip->flags & TPM_CHIP_FLAG_TPM2)
- return -ENODEV;
+ struct acpi_table_tpm2 *tbl;
+ struct acpi_tpm2_phy *t2phy;
+ int format;
log = &chip->log;
@@ -61,23 +61,40 @@ int tpm_read_log_acpi(struct tpm_chip *chip)
if (!chip->acpi_dev_handle)
return -ENODEV;
- /* Find TCPA entry in RSDT (ACPI_LOGICAL_ADDRESSING) */
- status = acpi_get_table(ACPI_SIG_TCPA, 1,
- (struct acpi_table_header **)&buff);
-
- if (ACPI_FAILURE(status))
- return -ENODEV;
-
- switch(buff->platform_class) {
- case BIOS_SERVER:
- len = buff->server.log_max_len;
- start = buff->server.log_start_addr;
- break;
- case BIOS_CLIENT:
- default:
- len = buff->client.log_max_len;
- start = buff->client.log_start_addr;
- break;
+ if (chip->flags & TPM_CHIP_FLAG_TPM2) {
+ status = acpi_get_table("TPM2", 1,
+ (struct acpi_table_header **)&tbl);
+ if (ACPI_FAILURE(status))
+ return -ENODEV;
+ if (tbl->header.length <
+ sizeof(*tbl) + sizeof(struct acpi_tpm2_phy))
+ return -ENODEV;
+ t2phy = (void *)tbl + sizeof(*tbl);
+ len = t2phy->log_area_minimum_length;
+ start = t2phy->log_area_start_address;
+ if (!start || !len)
+ return -ENODEV;
+ format = EFI_TCG2_EVENT_LOG_FORMAT_TCG_2;
+ } else {
+ /* Find TCPA entry in RSDT (ACPI_LOGICAL_ADDRESSING) */
+ status = acpi_get_table(ACPI_SIG_TCPA, 1,
+ (struct acpi_table_header **)&buff);
+
+ if (ACPI_FAILURE(status))
+ return -ENODEV;
+
+ switch (buff->platform_class) {
+ case BIOS_SERVER:
+ len = buff->server.log_max_len;
+ start = buff->server.log_start_addr;
+ break;
+ case BIOS_CLIENT:
+ default:
+ len = buff->client.log_max_len;
+ start = buff->client.log_start_addr;
+ break;
+ }
+ format = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
}
if (!len) {
dev_warn(&chip->dev, "%s: TCPA log area empty\n", __func__);
@@ -98,7 +115,7 @@ int tpm_read_log_acpi(struct tpm_chip *chip)
memcpy_fromio(log->bios_event_log, virt, len);
acpi_os_unmap_iomem(virt, len);
- return EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
+ return format;
err:
kfree(log->bios_event_log);
--
2.26.2
^ permalink raw reply related
* Re: [PATCH v5 1/2] acpi: Extend TPM2 ACPI table with missing log fields
From: Stefan Berger @ 2020-06-25 12:10 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Stefan Berger, Jiandi An, linux-integrity, linux-kernel,
linux-acpi, linux-security-module, Rafael J . Wysocki
In-Reply-To: <20200625023431.GB270125@linux.intel.com>
On 6/24/20 10:34 PM, Jarkko Sakkinen wrote:
> On Wed, Jun 24, 2020 at 08:38:25PM -0400, Stefan Berger wrote:
>> On 6/24/20 8:00 PM, Jarkko Sakkinen wrote:
>>> On Tue, Jun 23, 2020 at 08:06:35AM -0400, Stefan Berger wrote:
>>>> From: Stefan Berger <stefanb@linux.ibm.com>
>>>>
>>>> Recent extensions of the TPM2 ACPI table added 3 more fields
>>>> including 12 bytes of start method specific parameters and Log Area
>>>> Minimum Length (u32) and Log Area Start Address (u64). So, we extend
>>>> the existing structure with these fields to allow non-UEFI systems
>>>> to access the TPM2's log.
>>>>
>>>> The specification that has the new fields is the following:
>>>> TCG ACPI Specification
>>>> Family "1.2" and "2.0"
>>>> Version 1.2, Revision 8
>>>>
>>>> Adapt all existing table size calculations to use
>>>> offsetof(struct acpi_table_tpm2, start_method_specific)
>>>> [where start_method_specific is a newly added field]
>>>> rather than sizeof(struct acpi_table_tpm2) so that the addition
>>>> of the new fields does not affect current systems that may not
>>>> have them.
>>>>
>>> I found at least one regression from this patch. Please remove my
>>> reviewed-by comment form the next version.
>>>
>>> Should have:
>>>
>>> Link: https://trustedcomputinggroup.org/wp-content/uploads/TCG_ACPIGeneralSpecification_v1.20_r8.pdf
>>>
>>> Please, add this.
>>>
>>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>>>> Cc: linux-acpi@vger.kernel.org
>>>> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>>>> ---
>>>> drivers/char/tpm/tpm_crb.c | 13 ++++++++++---
>>>> drivers/char/tpm/tpm_tis.c | 4 +++-
>>>> include/acpi/actbl3.h | 5 +++--
>>>> 3 files changed, 16 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
>>>> index a9dcf31eadd2..0565aa5482f9 100644
>>>> --- a/drivers/char/tpm/tpm_crb.c
>>>> +++ b/drivers/char/tpm/tpm_crb.c
>>>> @@ -669,7 +669,9 @@ static int crb_acpi_add(struct acpi_device *device)
>>>> status = acpi_get_table(ACPI_SIG_TPM2, 1,
>>>> (struct acpi_table_header **) &buf);
>>>> - if (ACPI_FAILURE(status) || buf->header.length < sizeof(*buf)) {
>>>> + if (ACPI_FAILURE(status) || buf->header.length <
>>>> + offsetof(struct acpi_table_tpm2,
>>>> + start_method_specific)) {
>>>> dev_err(dev, FW_BUG "failed to get TPM2 ACPI table\n");
>>>> return -EINVAL;
>>>> }
>>>> @@ -684,14 +686,19 @@ static int crb_acpi_add(struct acpi_device *device)
>>>> return -ENOMEM;
>>>> if (sm == ACPI_TPM2_COMMAND_BUFFER_WITH_ARM_SMC) {
>>>> - if (buf->header.length < (sizeof(*buf) + sizeof(*crb_smc))) {
>>>> + if (buf->header.length <
>>>> + (offsetof(struct acpi_table_tpm2,
>>>> + start_method_specific) +
>>> Should be
>>>
>>> offsetof(struct acpti_table_tpm2, log_area_minimum_length)
>>
>> The old code had sizeof(*buf) with buf being 'struct acpi_table_tpm2' and
>> that was equivalent to offsetof(struct acpi_table_tpm2,
>> start_method_specific) since 'start_method_specific' is the first new field
>> that we are adding right here. Also see 3rd paragraph in the patch
>> description. The replacement rule described there should apply to all
>> sizeof() calculations on 'struct acpi_table_tpm2.'
> Aren't you ignoring sizeof(*crb_smc) then?
It's still there.
^ permalink raw reply
* Re: [RFC][PATCH] net/bpfilter: Remove this broken and apparently unmantained
From: Greg KH @ 2020-06-25 12:07 UTC (permalink / raw)
To: Tetsuo Handa
Cc: Alexei Starovoitov, Eric W. Biederman, Linus Torvalds, Kees Cook,
Andrew Morton, Alexei Starovoitov, David Miller, Al Viro, bpf,
linux-fsdevel, Daniel Borkmann, Jakub Kicinski, Masahiro Yamada,
Gary Lin, Bruno Meneguele, linux-security-module, Casey Schaufler
In-Reply-To: <778297d2-512a-8361-cf05-42d9379e6977@i-love.sakura.ne.jp>
On Thu, Jun 25, 2020 at 08:03:26PM +0900, Tetsuo Handa wrote:
> On 2020/06/25 18:57, Greg KH wrote:
> > On Thu, Jun 25, 2020 at 03:38:14PM +0900, Tetsuo Handa wrote:
> >> My questions are:
> >>
> >> (1) "Signed and in-tree kernel module" assertion is pointless.
> >> In future, some of in-tree kernel modules might start using fork_usermode_blob()
> >> instead of call_usermodehelper(), with instructions containing what your initial
> >> use case does not use. There is no guarantee that such thing can't happen.
> >
> > I hope that this would happen for some tools, what's wrong with that?
> > That means we can ship those programs from within the kernel source tree
> > instead of trying to rely on keeping a specific user/kernel api stable
> > for forever.
> >
> > That would be a good thing, right?
>
> Some in-tree users might start embedding byte array containing userspace programs
> like /bin/sh when building kernels. How can we prove that such thing won't happen?
We have the code, we can read it, we can say, "hey, looks like you are
including bash, do you want to do that?" :)
> I consider that the byte array can contain arbitrary instructions (regardless of
> some tools used for building the byte array).
Sure, and is this a problem?
> >> Assuming that there will be multiple blobs, we need a way to identify these blobs.
> >> How does fork_usermode_blob() provide information for identification?
> >
> > If the kernel itself was running these blobs, why would LSM care about
> > it? These are coming from "within the building!" don't you trust the
> > kernel already?
> >
> > I don't understand the issue here.
>
> The byte array came from the kernel, but due to possibility of "root can poke into
> kernel or any process memory.", that byte array can become as untrusted as byte
> array coming from userspace. There is no concept like "the kernel itself _is_ running
> these blobs". Only a fact "the byte array _was_ copied from the kernel address space
> (rather than from some file on the filesystem)" exists. We need a mechanism (ideally,
> without counting on LSMs) for avoid peeking/poking etc. into/from the byte array
> which was copied from the kernel address space to user address space.
And what are you going to do with that if you can "look at the array"?
I really don't understand the objection here, why is this any different
than any other random kernel driver for what it can do?
> >> call_usermodehelper() can teach LSM modules via pre-existing file's pathname and
> >> inode's security label at security_bprm_creds_for_exec()/security_bprm_check() etc.
> >> But since fork_usermode_blob() accepts only "byte array" and "length of byte array"
> >> arguments, I'm not sure whether LSM modules can obtain information needed for
> >> inspection. How does fork_usermode_blob() tell that information?
> >
> > It would seem that the "security context" for those would be the same as
> > anything created before userspace launches today, right? You handle
> > that ok, and this should be just the same.
>
> I don't think so. Today when call_usermodehelper() is called, LSMs switch their security
> context (at least TOMOYO does it) for further syscalls from the usermode process started
> by the kernel context. But when fork_usermode_blob() is called, how LSMs can switch their
> security context for further syscalls from the usermode process started by the kernel
> context?
Ok, that makes a bit more sense. Why not just do the same thing that
you do today with call_usermodehelper()? The logic in a way is the
same, right?
> > But again, as these programs are coming from "within the kernel", why
> > would you want to disallow them? If you don't want to allow them, don't
> > build them into your kernel? :)
>
> I'm talking about not only "disallow unauthorized execve() request" but also "disallow
> unauthorized syscalls after execve() request". Coming from the kernel is not important.
Ok, then do the same thing that you do for call_usermodehelper() to
prevent this.
> >> Thus, LSM modules (including pathname based security) want to control how that byte
> >> array can behave. And how does fork_usermode_blob() tell necessary information?
> >
> > Think of these blobs just as any other kernel module would be today.
>
> No, I can't. How can we guarantee that the byte array came from kernel remains intact
> despite the possibility of "root can poke into kernel or any process memory" ?
You guarantee it the same way you guarantee that the wifi driver really
is running the code you think it is running. There is no difference
here.
> > Right now I, as a kernel module, can read/write to any file in the
> > system, and do all sorts of other fun things. You can't mediate that
> > today from a LSM, and this is just one other example of this.
>
> Some functions (e.g. kernel_sock_shutdown()) bypass permission checks by LSMs
> comes from a sort of trustness that the byte array kept inside kernel address
> space remains secure/intact.
And what is going to change that "trustness" here? The byte array came
from the kernel address space to start with. Are you thinking something
outside of the kernel will then tamper with those bytes to do something
else with them? If so, shouldn't you be preventing that userspace
program that does the tampering from doing that in the first place with
the LSM running?
> > The "only" change is that now this code is running in userspace context,
> > which for an overall security/system issue, should be better than
> > running it in kernel context, right?
>
> As soon as exposing that byte array outside of kernel address space, processes
> running such byte array are considered insecure/tampered.
Why? Do you mean that you do not trust any program once it has been
started either? If you can, why not do the same thing here?
> We can't prove that
> the byte array exposed to outside of kernel address space does only limited
> set of instructions, and we have to perform permission checks by LSMs.
Those checks should come through the same way you check any other
userspace program through an LSM. Fix up the context like mentioned
above with call_usermodehelper() and you should be fine, right?
> And LSMs need to receive the intent (or "security context" argument) from fork_usermode_blob()
> for restricting further syscalls by the usermode process started via fork_usermode_blob().
>
> >
> > Perhaps we just add new LSM hooks for every place that we call this new
> > function to run a blob? That will give you the needed "the kernel is
> > about to run a blob that we think is a userspace USB IR filter driver",
> > or whatever the blob does.
>
> Yes, that would be the intent (or "security context" argument) fork_usermode_blob()
> is missing. Though I don't know how such stringuish argument can be represented for
> individual LSM modules...
The same way we do it today for any LSM callback? i.e. by a new
function call :)
thanks,
greg k-h
^ permalink raw reply
* Re: [RFC][PATCH] net/bpfilter: Remove this broken and apparently unmantained
From: Tetsuo Handa @ 2020-06-25 11:03 UTC (permalink / raw)
To: Greg KH
Cc: Alexei Starovoitov, Eric W. Biederman, Linus Torvalds, Kees Cook,
Andrew Morton, Alexei Starovoitov, David Miller, Al Viro, bpf,
linux-fsdevel, Daniel Borkmann, Jakub Kicinski, Masahiro Yamada,
Gary Lin, Bruno Meneguele, linux-security-module, Casey Schaufler
In-Reply-To: <20200625095725.GA3303921@kroah.com>
On 2020/06/25 18:57, Greg KH wrote:
> On Thu, Jun 25, 2020 at 03:38:14PM +0900, Tetsuo Handa wrote:
>> My questions are:
>>
>> (1) "Signed and in-tree kernel module" assertion is pointless.
>> In future, some of in-tree kernel modules might start using fork_usermode_blob()
>> instead of call_usermodehelper(), with instructions containing what your initial
>> use case does not use. There is no guarantee that such thing can't happen.
>
> I hope that this would happen for some tools, what's wrong with that?
> That means we can ship those programs from within the kernel source tree
> instead of trying to rely on keeping a specific user/kernel api stable
> for forever.
>
> That would be a good thing, right?
Some in-tree users might start embedding byte array containing userspace programs
like /bin/sh when building kernels. How can we prove that such thing won't happen?
I consider that the byte array can contain arbitrary instructions (regardless of
some tools used for building the byte array).
>
>> Assuming that there will be multiple blobs, we need a way to identify these blobs.
>> How does fork_usermode_blob() provide information for identification?
>
> If the kernel itself was running these blobs, why would LSM care about
> it? These are coming from "within the building!" don't you trust the
> kernel already?
>
> I don't understand the issue here.
The byte array came from the kernel, but due to possibility of "root can poke into
kernel or any process memory.", that byte array can become as untrusted as byte
array coming from userspace. There is no concept like "the kernel itself _is_ running
these blobs". Only a fact "the byte array _was_ copied from the kernel address space
(rather than from some file on the filesystem)" exists. We need a mechanism (ideally,
without counting on LSMs) for avoid peeking/poking etc. into/from the byte array
which was copied from the kernel address space to user address space.
>> call_usermodehelper() can teach LSM modules via pre-existing file's pathname and
>> inode's security label at security_bprm_creds_for_exec()/security_bprm_check() etc.
>> But since fork_usermode_blob() accepts only "byte array" and "length of byte array"
>> arguments, I'm not sure whether LSM modules can obtain information needed for
>> inspection. How does fork_usermode_blob() tell that information?
>
> It would seem that the "security context" for those would be the same as
> anything created before userspace launches today, right? You handle
> that ok, and this should be just the same.
I don't think so. Today when call_usermodehelper() is called, LSMs switch their security
context (at least TOMOYO does it) for further syscalls from the usermode process started
by the kernel context. But when fork_usermode_blob() is called, how LSMs can switch their
security context for further syscalls from the usermode process started by the kernel
context?
>
> But again, as these programs are coming from "within the kernel", why
> would you want to disallow them? If you don't want to allow them, don't
> build them into your kernel? :)
I'm talking about not only "disallow unauthorized execve() request" but also "disallow
unauthorized syscalls after execve() request". Coming from the kernel is not important.
>> Thus, LSM modules (including pathname based security) want to control how that byte
>> array can behave. And how does fork_usermode_blob() tell necessary information?
>
> Think of these blobs just as any other kernel module would be today.
No, I can't. How can we guarantee that the byte array came from kernel remains intact
despite the possibility of "root can poke into kernel or any process memory" ?
> Right now I, as a kernel module, can read/write to any file in the
> system, and do all sorts of other fun things. You can't mediate that
> today from a LSM, and this is just one other example of this.
Some functions (e.g. kernel_sock_shutdown()) bypass permission checks by LSMs
comes from a sort of trustness that the byte array kept inside kernel address
space remains secure/intact.
>
> The "only" change is that now this code is running in userspace context,
> which for an overall security/system issue, should be better than
> running it in kernel context, right?
As soon as exposing that byte array outside of kernel address space, processes
running such byte array are considered insecure/tampered. We can't prove that
the byte array exposed to outside of kernel address space does only limited
set of instructions, and we have to perform permission checks by LSMs.
And LSMs need to receive the intent (or "security context" argument) from fork_usermode_blob()
for restricting further syscalls by the usermode process started via fork_usermode_blob().
>
> Perhaps we just add new LSM hooks for every place that we call this new
> function to run a blob? That will give you the needed "the kernel is
> about to run a blob that we think is a userspace USB IR filter driver",
> or whatever the blob does.
Yes, that would be the intent (or "security context" argument) fork_usermode_blob()
is missing. Though I don't know how such stringuish argument can be represented for
individual LSM modules...
^ permalink raw reply
* Re: [RFC][PATCH] net/bpfilter: Remove this broken and apparently unmantained
From: Greg KH @ 2020-06-25 9:57 UTC (permalink / raw)
To: Tetsuo Handa
Cc: Alexei Starovoitov, Eric W. Biederman, Linus Torvalds, Kees Cook,
Andrew Morton, Alexei Starovoitov, David Miller, Al Viro, bpf,
linux-fsdevel, Daniel Borkmann, Jakub Kicinski, Masahiro Yamada,
Gary Lin, Bruno Meneguele, linux-security-module, Casey Schaufler
In-Reply-To: <b83831ba-c330-7eb8-e6d5-5087de68a9b8@i-love.sakura.ne.jp>
On Thu, Jun 25, 2020 at 03:38:14PM +0900, Tetsuo Handa wrote:
> On 2020/06/25 10:35, Alexei Starovoitov wrote:
> >> What is unhappy for pathname based LSMs is that fork_usermode_blob() creates
> >> a file with empty filename. I can imagine that somebody would start abusing
> >> fork_usermode_blob() as an interface for starting programs like modprobe, hotplug,
> >> udevd and sshd. When such situation happened, how fork_usermode_blob() provides
> >> information for identifying the intent of such execve() requests?
> >>
> >> fork_usermode_blob() might also be an unhappy behavior for inode based LSMs (like
> >> SELinux and Smack) because it seems that fork_usermode_blob() can't have a chance
> >> to associate appropriate security labels based on the content of the byte array
> >> because files are created on-demand. Is fork_usermode_blob() friendly to inode
> >> based LSMs?
> >
> > blob is started by a kernel module. Regardless of path existence that kernel module
> > could have disabled any LSM and any kernel security mechanism.
> > People who write out of tree kernel modules found ways to bypass EXPORT_SYMBOL
> > with and without _GPL. Modules can do anything. It's only the number of hoops
> > they need to jump through to get what they want.
>
> So what? I know that. That's irrelevant to my questions.
>
> > Signed and in-tree kernel module is the only way to protect the integrity of the system.
> > That's why user blob is part of kernel module elf object and it's covered by the same
> > module signature verification logic.
>
> My questions are:
>
> (1) "Signed and in-tree kernel module" assertion is pointless.
> In future, some of in-tree kernel modules might start using fork_usermode_blob()
> instead of call_usermodehelper(), with instructions containing what your initial
> use case does not use. There is no guarantee that such thing can't happen.
I hope that this would happen for some tools, what's wrong with that?
That means we can ship those programs from within the kernel source tree
instead of trying to rely on keeping a specific user/kernel api stable
for forever.
That would be a good thing, right?
> Assuming that there will be multiple blobs, we need a way to identify these blobs.
> How does fork_usermode_blob() provide information for identification?
If the kernel itself was running these blobs, why would LSM care about
it? These are coming from "within the building!" don't you trust the
kernel already?
I don't understand the issue here.
> (2) Running some blob in usermode means that permission checks by LSM modules will
> be enforced. For example, socket's shutdown operation via shutdown() syscall from
> usermode blob will involve security_socket_shutdown() check.
>
> ----------
> int kernel_sock_shutdown(struct socket *sock, enum sock_shutdown_cmd how)
> {
> return sock->ops->shutdown(sock, how);
> }
> ----------
>
> ----------
> int __sys_shutdown(int fd, int how)
> {
> int err, fput_needed;
> struct socket *sock;
>
> sock = sockfd_lookup_light(fd, &err, &fput_needed);
> if (sock != NULL) {
> err = security_socket_shutdown(sock, how);
> if (!err)
> err = sock->ops->shutdown(sock, how);
> fput_light(sock->file, fput_needed);
> }
> return err;
> }
>
> SYSCALL_DEFINE2(shutdown, int, fd, int, how)
> {
> return __sys_shutdown(fd, how);
> }
> ----------
>
> I don't know what instructions your blob would contain. But even if the blobs
> containing your initial use case use only setsockopt()/getsockopt() syscalls,
> LSM modules have rights to inspect and reject these requests from usermode blobs
> via security_socket_setsockopt()/security_socket_getsockopt() hooks. In order to
> inspect these requests, LSM modules need information (so called "security context"),
> and fork_usermode_blob() has to be able to somehow teach that information to LSM
> modules. Pathname is one of information for pathname based LSM modules. Inode's
> security label is one of information for inode based LSM modules.
>
> call_usermodehelper() can teach LSM modules via pre-existing file's pathname and
> inode's security label at security_bprm_creds_for_exec()/security_bprm_check() etc.
> But since fork_usermode_blob() accepts only "byte array" and "length of byte array"
> arguments, I'm not sure whether LSM modules can obtain information needed for
> inspection. How does fork_usermode_blob() tell that information?
It would seem that the "security context" for those would be the same as
anything created before userspace launches today, right? You handle
that ok, and this should be just the same.
But again, as these programs are coming from "within the kernel", why
would you want to disallow them? If you don't want to allow them, don't
build them into your kernel? :)
> (3) Again, "root can poke into kernel or any process memory." assertion is pointless.
> Answering to your questions
>
> > hmm. do you really mean that it's possible for an LSM to restrict CAP_SYS_ADMIN effectively?
> Not every LSM module restricts CAP_* flags. But LSM modules can implement finer grained
> restrictions than plain CAP_* flags.
>
> > How elf binaries embedded in the kernel modules different from pid 1?
> No difference.
>
> > If anything can peek into their memory the system is compromised.
> Permission checks via LSM modules are there to prevent such behavior.
>
> > Say, there are no user blobs in kernel modules. How pid 1 memory is different
> > from all the JITed images? How is it different for all memory regions shared
> > between kernel and user processes?
> I don't know what "the JITed images" means. But I guess that the answer is
> "No difference".
>
> Then, I ask you back.
>
> Although the byte array (which contains code / data) might be initially loaded from
> the kernel space (which is protected), that byte array is no longer protected (e.g.
> SIGKILL, ptrace()) when executed because they are placed in the user address space.
>
> Why the usermode process started by fork_usermode_blob() cannot interfere (or be
> interfered by) the rest of the system (including normal usermode processes) ?
> And I guess that your answer is "the usermode process started by fork_usermode_blob()
> _can_ (and be interfered by) the rest of the system, for they are nothing but
> normal usermode processes."
>
> Thus, LSM modules (including pathname based security) want to control how that byte
> array can behave. And how does fork_usermode_blob() tell necessary information?
Think of these blobs just as any other kernel module would be today.
Right now I, as a kernel module, can read/write to any file in the
system, and do all sorts of other fun things. You can't mediate that
today from a LSM, and this is just one other example of this.
The "only" change is that now this code is running in userspace context,
which for an overall security/system issue, should be better than
running it in kernel context, right?
Perhaps we just add new LSM hooks for every place that we call this new
function to run a blob? That will give you the needed "the kernel is
about to run a blob that we think is a userspace USB IR filter driver",
or whatever the blob does.
Would that help out?
But, given that we don't even have any in-kernel users, all of this
feels like a lot of arguing over something that no one can currently
even have happen...
thanks,
greg k-h
^ permalink raw reply
* Re: [RFC][PATCH] net/bpfilter: Remove this broken and apparently unmantained
From: Tetsuo Handa @ 2020-06-25 6:38 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Eric W. Biederman, Linus Torvalds, Kees Cook, Andrew Morton,
Alexei Starovoitov, David Miller, Al Viro, bpf, linux-fsdevel,
Daniel Borkmann, Jakub Kicinski, Masahiro Yamada, Gary Lin,
Bruno Meneguele, linux-security-module, Casey Schaufler
In-Reply-To: <20200625013518.chuqehybelk2k27x@ast-mbp.dhcp.thefacebook.com>
On 2020/06/25 10:35, Alexei Starovoitov wrote:
>> What is unhappy for pathname based LSMs is that fork_usermode_blob() creates
>> a file with empty filename. I can imagine that somebody would start abusing
>> fork_usermode_blob() as an interface for starting programs like modprobe, hotplug,
>> udevd and sshd. When such situation happened, how fork_usermode_blob() provides
>> information for identifying the intent of such execve() requests?
>>
>> fork_usermode_blob() might also be an unhappy behavior for inode based LSMs (like
>> SELinux and Smack) because it seems that fork_usermode_blob() can't have a chance
>> to associate appropriate security labels based on the content of the byte array
>> because files are created on-demand. Is fork_usermode_blob() friendly to inode
>> based LSMs?
>
> blob is started by a kernel module. Regardless of path existence that kernel module
> could have disabled any LSM and any kernel security mechanism.
> People who write out of tree kernel modules found ways to bypass EXPORT_SYMBOL
> with and without _GPL. Modules can do anything. It's only the number of hoops
> they need to jump through to get what they want.
So what? I know that. That's irrelevant to my questions.
> Signed and in-tree kernel module is the only way to protect the integrity of the system.
> That's why user blob is part of kernel module elf object and it's covered by the same
> module signature verification logic.
My questions are:
(1) "Signed and in-tree kernel module" assertion is pointless.
In future, some of in-tree kernel modules might start using fork_usermode_blob()
instead of call_usermodehelper(), with instructions containing what your initial
use case does not use. There is no guarantee that such thing can't happen.
Assuming that there will be multiple blobs, we need a way to identify these blobs.
How does fork_usermode_blob() provide information for identification?
(2) Running some blob in usermode means that permission checks by LSM modules will
be enforced. For example, socket's shutdown operation via shutdown() syscall from
usermode blob will involve security_socket_shutdown() check.
----------
int kernel_sock_shutdown(struct socket *sock, enum sock_shutdown_cmd how)
{
return sock->ops->shutdown(sock, how);
}
----------
----------
int __sys_shutdown(int fd, int how)
{
int err, fput_needed;
struct socket *sock;
sock = sockfd_lookup_light(fd, &err, &fput_needed);
if (sock != NULL) {
err = security_socket_shutdown(sock, how);
if (!err)
err = sock->ops->shutdown(sock, how);
fput_light(sock->file, fput_needed);
}
return err;
}
SYSCALL_DEFINE2(shutdown, int, fd, int, how)
{
return __sys_shutdown(fd, how);
}
----------
I don't know what instructions your blob would contain. But even if the blobs
containing your initial use case use only setsockopt()/getsockopt() syscalls,
LSM modules have rights to inspect and reject these requests from usermode blobs
via security_socket_setsockopt()/security_socket_getsockopt() hooks. In order to
inspect these requests, LSM modules need information (so called "security context"),
and fork_usermode_blob() has to be able to somehow teach that information to LSM
modules. Pathname is one of information for pathname based LSM modules. Inode's
security label is one of information for inode based LSM modules.
call_usermodehelper() can teach LSM modules via pre-existing file's pathname and
inode's security label at security_bprm_creds_for_exec()/security_bprm_check() etc.
But since fork_usermode_blob() accepts only "byte array" and "length of byte array"
arguments, I'm not sure whether LSM modules can obtain information needed for
inspection. How does fork_usermode_blob() tell that information?
(3) Again, "root can poke into kernel or any process memory." assertion is pointless.
Answering to your questions
> hmm. do you really mean that it's possible for an LSM to restrict CAP_SYS_ADMIN effectively?
Not every LSM module restricts CAP_* flags. But LSM modules can implement finer grained
restrictions than plain CAP_* flags.
> How elf binaries embedded in the kernel modules different from pid 1?
No difference.
> If anything can peek into their memory the system is compromised.
Permission checks via LSM modules are there to prevent such behavior.
> Say, there are no user blobs in kernel modules. How pid 1 memory is different
> from all the JITed images? How is it different for all memory regions shared
> between kernel and user processes?
I don't know what "the JITed images" means. But I guess that the answer is
"No difference".
Then, I ask you back.
Although the byte array (which contains code / data) might be initially loaded from
the kernel space (which is protected), that byte array is no longer protected (e.g.
SIGKILL, ptrace()) when executed because they are placed in the user address space.
Why the usermode process started by fork_usermode_blob() cannot interfere (or be
interfered by) the rest of the system (including normal usermode processes) ?
And I guess that your answer is "the usermode process started by fork_usermode_blob()
_can_ (and be interfered by) the rest of the system, for they are nothing but
normal usermode processes."
Thus, LSM modules (including pathname based security) want to control how that byte
array can behave. And how does fork_usermode_blob() tell necessary information?
Your answers up to now did not convince LSM modules to ignore what the usermode process
started by fork_usermode_blob() can do. If you again don't answer my questions, I'll
ack to https://lkml.kernel.org/r/875zc4c86z.fsf_-_@x220.int.ebiederm.org .
^ permalink raw reply
* Re: [PATCH v5 1/2] acpi: Extend TPM2 ACPI table with missing log fields
From: Jarkko Sakkinen @ 2020-06-25 2:54 UTC (permalink / raw)
To: Stefan Berger
Cc: Stefan Berger, Jiandi An, linux-integrity, linux-kernel,
linux-acpi, linux-security-module, Rafael J . Wysocki
In-Reply-To: <20200625025251.GC270125@linux.intel.com>
On Thu, Jun 25, 2020 at 05:52:56AM +0300, Jarkko Sakkinen wrote:
> On Thu, Jun 25, 2020 at 05:34:38AM +0300, Jarkko Sakkinen wrote:
> > On Wed, Jun 24, 2020 at 08:38:25PM -0400, Stefan Berger wrote:
> > > On 6/24/20 8:00 PM, Jarkko Sakkinen wrote:
> > > > On Tue, Jun 23, 2020 at 08:06:35AM -0400, Stefan Berger wrote:
> > > > > From: Stefan Berger <stefanb@linux.ibm.com>
> > > > >
> > > > > Recent extensions of the TPM2 ACPI table added 3 more fields
> > > > > including 12 bytes of start method specific parameters and Log Area
> > > > > Minimum Length (u32) and Log Area Start Address (u64). So, we extend
> > > > > the existing structure with these fields to allow non-UEFI systems
> > > > > to access the TPM2's log.
> > > > >
> > > > > The specification that has the new fields is the following:
> > > > > TCG ACPI Specification
> > > > > Family "1.2" and "2.0"
> > > > > Version 1.2, Revision 8
> > > > >
> > > > > Adapt all existing table size calculations to use
> > > > > offsetof(struct acpi_table_tpm2, start_method_specific)
> > > > > [where start_method_specific is a newly added field]
> > > > > rather than sizeof(struct acpi_table_tpm2) so that the addition
> > > > > of the new fields does not affect current systems that may not
> > > > > have them.
> > > > >
> > > > I found at least one regression from this patch. Please remove my
> > > > reviewed-by comment form the next version.
> > > >
> > > > Should have:
> > > >
> > > > Link: https://trustedcomputinggroup.org/wp-content/uploads/TCG_ACPIGeneralSpecification_v1.20_r8.pdf
> > > >
> > > > Please, add this.
> > > >
> > > > > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> > > > > Cc: linux-acpi@vger.kernel.org
> > > > > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > > > ---
> > > > > drivers/char/tpm/tpm_crb.c | 13 ++++++++++---
> > > > > drivers/char/tpm/tpm_tis.c | 4 +++-
> > > > > include/acpi/actbl3.h | 5 +++--
> > > > > 3 files changed, 16 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> > > > > index a9dcf31eadd2..0565aa5482f9 100644
> > > > > --- a/drivers/char/tpm/tpm_crb.c
> > > > > +++ b/drivers/char/tpm/tpm_crb.c
> > > > > @@ -669,7 +669,9 @@ static int crb_acpi_add(struct acpi_device *device)
> > > > > status = acpi_get_table(ACPI_SIG_TPM2, 1,
> > > > > (struct acpi_table_header **) &buf);
> > > > > - if (ACPI_FAILURE(status) || buf->header.length < sizeof(*buf)) {
> > > > > + if (ACPI_FAILURE(status) || buf->header.length <
> > > > > + offsetof(struct acpi_table_tpm2,
> > > > > + start_method_specific)) {
> > > > > dev_err(dev, FW_BUG "failed to get TPM2 ACPI table\n");
> > > > > return -EINVAL;
> > > > > }
> > > > > @@ -684,14 +686,19 @@ static int crb_acpi_add(struct acpi_device *device)
> > > > > return -ENOMEM;
> > > > > if (sm == ACPI_TPM2_COMMAND_BUFFER_WITH_ARM_SMC) {
> > > > > - if (buf->header.length < (sizeof(*buf) + sizeof(*crb_smc))) {
> > > > > + if (buf->header.length <
> > > > > + (offsetof(struct acpi_table_tpm2,
> > > > > + start_method_specific) +
> > > > Should be
> > > >
> > > > offsetof(struct acpti_table_tpm2, log_area_minimum_length)
> > >
> > >
> > > The old code had sizeof(*buf) with buf being 'struct acpi_table_tpm2' and
> > > that was equivalent to offsetof(struct acpi_table_tpm2,
> > > start_method_specific) since 'start_method_specific' is the first new field
> > > that we are adding right here. Also see 3rd paragraph in the patch
> > > description. The replacement rule described there should apply to all
> > > sizeof() calculations on 'struct acpi_table_tpm2.'
> >
> > Aren't you ignoring sizeof(*crb_smc) then?
>
> Duh, it's there I see. Sorry, my mistake.
>
> Please put the new fields in a separate struct:
>
> struct acpi_tpm2_phy {
> u8 start_method_specific[12];
> u32 log_area_minimum_length;
> u64 log_area_start_address;
> };
>
> This way we don't have to obfuscate all the calculations and zero out
> the need for 1/2 in this patch set.
Also remark that, if you continue the current patch, that would need
tested-by from ARM whereas a new struct does not because the ARM code
is intact.
/Jarkko
^ permalink raw reply
* Re: [PATCH v5 1/2] acpi: Extend TPM2 ACPI table with missing log fields
From: Jarkko Sakkinen @ 2020-06-25 2:52 UTC (permalink / raw)
To: Stefan Berger
Cc: Stefan Berger, Jiandi An, linux-integrity, linux-kernel,
linux-acpi, linux-security-module, Rafael J . Wysocki
In-Reply-To: <20200625023431.GB270125@linux.intel.com>
On Thu, Jun 25, 2020 at 05:34:38AM +0300, Jarkko Sakkinen wrote:
> On Wed, Jun 24, 2020 at 08:38:25PM -0400, Stefan Berger wrote:
> > On 6/24/20 8:00 PM, Jarkko Sakkinen wrote:
> > > On Tue, Jun 23, 2020 at 08:06:35AM -0400, Stefan Berger wrote:
> > > > From: Stefan Berger <stefanb@linux.ibm.com>
> > > >
> > > > Recent extensions of the TPM2 ACPI table added 3 more fields
> > > > including 12 bytes of start method specific parameters and Log Area
> > > > Minimum Length (u32) and Log Area Start Address (u64). So, we extend
> > > > the existing structure with these fields to allow non-UEFI systems
> > > > to access the TPM2's log.
> > > >
> > > > The specification that has the new fields is the following:
> > > > TCG ACPI Specification
> > > > Family "1.2" and "2.0"
> > > > Version 1.2, Revision 8
> > > >
> > > > Adapt all existing table size calculations to use
> > > > offsetof(struct acpi_table_tpm2, start_method_specific)
> > > > [where start_method_specific is a newly added field]
> > > > rather than sizeof(struct acpi_table_tpm2) so that the addition
> > > > of the new fields does not affect current systems that may not
> > > > have them.
> > > >
> > > I found at least one regression from this patch. Please remove my
> > > reviewed-by comment form the next version.
> > >
> > > Should have:
> > >
> > > Link: https://trustedcomputinggroup.org/wp-content/uploads/TCG_ACPIGeneralSpecification_v1.20_r8.pdf
> > >
> > > Please, add this.
> > >
> > > > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> > > > Cc: linux-acpi@vger.kernel.org
> > > > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > > ---
> > > > drivers/char/tpm/tpm_crb.c | 13 ++++++++++---
> > > > drivers/char/tpm/tpm_tis.c | 4 +++-
> > > > include/acpi/actbl3.h | 5 +++--
> > > > 3 files changed, 16 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> > > > index a9dcf31eadd2..0565aa5482f9 100644
> > > > --- a/drivers/char/tpm/tpm_crb.c
> > > > +++ b/drivers/char/tpm/tpm_crb.c
> > > > @@ -669,7 +669,9 @@ static int crb_acpi_add(struct acpi_device *device)
> > > > status = acpi_get_table(ACPI_SIG_TPM2, 1,
> > > > (struct acpi_table_header **) &buf);
> > > > - if (ACPI_FAILURE(status) || buf->header.length < sizeof(*buf)) {
> > > > + if (ACPI_FAILURE(status) || buf->header.length <
> > > > + offsetof(struct acpi_table_tpm2,
> > > > + start_method_specific)) {
> > > > dev_err(dev, FW_BUG "failed to get TPM2 ACPI table\n");
> > > > return -EINVAL;
> > > > }
> > > > @@ -684,14 +686,19 @@ static int crb_acpi_add(struct acpi_device *device)
> > > > return -ENOMEM;
> > > > if (sm == ACPI_TPM2_COMMAND_BUFFER_WITH_ARM_SMC) {
> > > > - if (buf->header.length < (sizeof(*buf) + sizeof(*crb_smc))) {
> > > > + if (buf->header.length <
> > > > + (offsetof(struct acpi_table_tpm2,
> > > > + start_method_specific) +
> > > Should be
> > >
> > > offsetof(struct acpti_table_tpm2, log_area_minimum_length)
> >
> >
> > The old code had sizeof(*buf) with buf being 'struct acpi_table_tpm2' and
> > that was equivalent to offsetof(struct acpi_table_tpm2,
> > start_method_specific) since 'start_method_specific' is the first new field
> > that we are adding right here. Also see 3rd paragraph in the patch
> > description. The replacement rule described there should apply to all
> > sizeof() calculations on 'struct acpi_table_tpm2.'
>
> Aren't you ignoring sizeof(*crb_smc) then?
Duh, it's there I see. Sorry, my mistake.
Please put the new fields in a separate struct:
struct acpi_tpm2_phy {
u8 start_method_specific[12];
u32 log_area_minimum_length;
u64 log_area_start_address;
};
This way we don't have to obfuscate all the calculations and zero out
the need for 1/2 in this patch set.
/Jarkko
^ permalink raw reply
* Re: [PATCH v5 1/2] acpi: Extend TPM2 ACPI table with missing log fields
From: Jarkko Sakkinen @ 2020-06-25 2:34 UTC (permalink / raw)
To: Stefan Berger
Cc: Stefan Berger, Jiandi An, linux-integrity, linux-kernel,
linux-acpi, linux-security-module, Rafael J . Wysocki
In-Reply-To: <9d94c704-5774-ceeb-e4f3-010f74ffe37b@linux.ibm.com>
On Wed, Jun 24, 2020 at 08:38:25PM -0400, Stefan Berger wrote:
> On 6/24/20 8:00 PM, Jarkko Sakkinen wrote:
> > On Tue, Jun 23, 2020 at 08:06:35AM -0400, Stefan Berger wrote:
> > > From: Stefan Berger <stefanb@linux.ibm.com>
> > >
> > > Recent extensions of the TPM2 ACPI table added 3 more fields
> > > including 12 bytes of start method specific parameters and Log Area
> > > Minimum Length (u32) and Log Area Start Address (u64). So, we extend
> > > the existing structure with these fields to allow non-UEFI systems
> > > to access the TPM2's log.
> > >
> > > The specification that has the new fields is the following:
> > > TCG ACPI Specification
> > > Family "1.2" and "2.0"
> > > Version 1.2, Revision 8
> > >
> > > Adapt all existing table size calculations to use
> > > offsetof(struct acpi_table_tpm2, start_method_specific)
> > > [where start_method_specific is a newly added field]
> > > rather than sizeof(struct acpi_table_tpm2) so that the addition
> > > of the new fields does not affect current systems that may not
> > > have them.
> > >
> > I found at least one regression from this patch. Please remove my
> > reviewed-by comment form the next version.
> >
> > Should have:
> >
> > Link: https://trustedcomputinggroup.org/wp-content/uploads/TCG_ACPIGeneralSpecification_v1.20_r8.pdf
> >
> > Please, add this.
> >
> > > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> > > Cc: linux-acpi@vger.kernel.org
> > > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > ---
> > > drivers/char/tpm/tpm_crb.c | 13 ++++++++++---
> > > drivers/char/tpm/tpm_tis.c | 4 +++-
> > > include/acpi/actbl3.h | 5 +++--
> > > 3 files changed, 16 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> > > index a9dcf31eadd2..0565aa5482f9 100644
> > > --- a/drivers/char/tpm/tpm_crb.c
> > > +++ b/drivers/char/tpm/tpm_crb.c
> > > @@ -669,7 +669,9 @@ static int crb_acpi_add(struct acpi_device *device)
> > > status = acpi_get_table(ACPI_SIG_TPM2, 1,
> > > (struct acpi_table_header **) &buf);
> > > - if (ACPI_FAILURE(status) || buf->header.length < sizeof(*buf)) {
> > > + if (ACPI_FAILURE(status) || buf->header.length <
> > > + offsetof(struct acpi_table_tpm2,
> > > + start_method_specific)) {
> > > dev_err(dev, FW_BUG "failed to get TPM2 ACPI table\n");
> > > return -EINVAL;
> > > }
> > > @@ -684,14 +686,19 @@ static int crb_acpi_add(struct acpi_device *device)
> > > return -ENOMEM;
> > > if (sm == ACPI_TPM2_COMMAND_BUFFER_WITH_ARM_SMC) {
> > > - if (buf->header.length < (sizeof(*buf) + sizeof(*crb_smc))) {
> > > + if (buf->header.length <
> > > + (offsetof(struct acpi_table_tpm2,
> > > + start_method_specific) +
> > Should be
> >
> > offsetof(struct acpti_table_tpm2, log_area_minimum_length)
>
>
> The old code had sizeof(*buf) with buf being 'struct acpi_table_tpm2' and
> that was equivalent to offsetof(struct acpi_table_tpm2,
> start_method_specific) since 'start_method_specific' is the first new field
> that we are adding right here. Also see 3rd paragraph in the patch
> description. The replacement rule described there should apply to all
> sizeof() calculations on 'struct acpi_table_tpm2.'
Aren't you ignoring sizeof(*crb_smc) then?
>
> I'll address the other issues.
>
>
> Stefan
^ permalink raw reply
* Re: [RFC][PATCH] net/bpfilter: Remove this broken and apparently unmantained
From: Alexei Starovoitov @ 2020-06-25 1:35 UTC (permalink / raw)
To: Tetsuo Handa
Cc: Eric W. Biederman, Linus Torvalds, Kees Cook, Andrew Morton,
Alexei Starovoitov, David Miller, Al Viro, bpf, linux-fsdevel,
Daniel Borkmann, Jakub Kicinski, Masahiro Yamada, Gary Lin,
Bruno Meneguele, linux-security-module, Casey Schaufler
In-Reply-To: <2f55102e-5d11-5569-8248-13618d517e93@i-love.sakura.ne.jp>
On Thu, Jun 25, 2020 at 08:14:20AM +0900, Tetsuo Handa wrote:
> On 2020/06/24 23:26, Alexei Starovoitov wrote:
> > On Wed, Jun 24, 2020 at 5:17 AM Eric W. Biederman <ebiederm@xmission.com> wrote:
> >>
> >> Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:
> >>
> >>> On Tue, Jun 23, 2020 at 01:53:48PM -0500, Eric W. Biederman wrote:
> >>
> >>> There is no refcnt bug. It was a user error on tomoyo side.
> >>> fork_blob() works as expected.
> >>
> >> Nope. I have independently confirmed it myself.
> >
> > I guess you've tried Tetsuo's fork_blob("#!/bin/true") kernel module ?
> > yes. that fails. It never meant to be used for this.
> > With elf blob it works, but breaks if there are rejections
> > in things like security_bprm_creds_for_exec().
> > In my mind that path was 'must succeed or kernel module is toast'.
> > Like passing NULL into a function that doesn't check for it.
> > Working on a fix for that since Tetsuo cares.
> >
>
> What is unhappy for pathname based LSMs is that fork_usermode_blob() creates
> a file with empty filename. I can imagine that somebody would start abusing
> fork_usermode_blob() as an interface for starting programs like modprobe, hotplug,
> udevd and sshd. When such situation happened, how fork_usermode_blob() provides
> information for identifying the intent of such execve() requests?
>
> fork_usermode_blob() might also be an unhappy behavior for inode based LSMs (like
> SELinux and Smack) because it seems that fork_usermode_blob() can't have a chance
> to associate appropriate security labels based on the content of the byte array
> because files are created on-demand. Is fork_usermode_blob() friendly to inode
> based LSMs?
blob is started by a kernel module. Regardless of path existence that kernel module
could have disabled any LSM and any kernel security mechanism.
People who write out of tree kernel modules found ways to bypass EXPORT_SYMBOL
with and without _GPL. Modules can do anything. It's only the number of hoops
they need to jump through to get what they want.
Signed and in-tree kernel module is the only way to protect the integrity of the system.
That's why user blob is part of kernel module elf object and it's covered by the same
module signature verification logic.
^ permalink raw reply
* Re: [PATCH v5 1/2] acpi: Extend TPM2 ACPI table with missing log fields
From: Stefan Berger @ 2020-06-25 0:38 UTC (permalink / raw)
To: Jarkko Sakkinen, Stefan Berger, Jiandi An
Cc: linux-integrity, linux-kernel, linux-acpi, linux-security-module,
Rafael J . Wysocki
In-Reply-To: <20200625000021.GC21758@linux.intel.com>
On 6/24/20 8:00 PM, Jarkko Sakkinen wrote:
> On Tue, Jun 23, 2020 at 08:06:35AM -0400, Stefan Berger wrote:
>> From: Stefan Berger <stefanb@linux.ibm.com>
>>
>> Recent extensions of the TPM2 ACPI table added 3 more fields
>> including 12 bytes of start method specific parameters and Log Area
>> Minimum Length (u32) and Log Area Start Address (u64). So, we extend
>> the existing structure with these fields to allow non-UEFI systems
>> to access the TPM2's log.
>>
>> The specification that has the new fields is the following:
>> TCG ACPI Specification
>> Family "1.2" and "2.0"
>> Version 1.2, Revision 8
>>
>> Adapt all existing table size calculations to use
>> offsetof(struct acpi_table_tpm2, start_method_specific)
>> [where start_method_specific is a newly added field]
>> rather than sizeof(struct acpi_table_tpm2) so that the addition
>> of the new fields does not affect current systems that may not
>> have them.
>>
> I found at least one regression from this patch. Please remove my
> reviewed-by comment form the next version.
>
> Should have:
>
> Link: https://trustedcomputinggroup.org/wp-content/uploads/TCG_ACPIGeneralSpecification_v1.20_r8.pdf
>
> Please, add this.
>
>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>> Cc: linux-acpi@vger.kernel.org
>> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>> ---
>> drivers/char/tpm/tpm_crb.c | 13 ++++++++++---
>> drivers/char/tpm/tpm_tis.c | 4 +++-
>> include/acpi/actbl3.h | 5 +++--
>> 3 files changed, 16 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
>> index a9dcf31eadd2..0565aa5482f9 100644
>> --- a/drivers/char/tpm/tpm_crb.c
>> +++ b/drivers/char/tpm/tpm_crb.c
>> @@ -669,7 +669,9 @@ static int crb_acpi_add(struct acpi_device *device)
>>
>> status = acpi_get_table(ACPI_SIG_TPM2, 1,
>> (struct acpi_table_header **) &buf);
>> - if (ACPI_FAILURE(status) || buf->header.length < sizeof(*buf)) {
>> + if (ACPI_FAILURE(status) || buf->header.length <
>> + offsetof(struct acpi_table_tpm2,
>> + start_method_specific)) {
>> dev_err(dev, FW_BUG "failed to get TPM2 ACPI table\n");
>> return -EINVAL;
>> }
>> @@ -684,14 +686,19 @@ static int crb_acpi_add(struct acpi_device *device)
>> return -ENOMEM;
>>
>> if (sm == ACPI_TPM2_COMMAND_BUFFER_WITH_ARM_SMC) {
>> - if (buf->header.length < (sizeof(*buf) + sizeof(*crb_smc))) {
>> + if (buf->header.length <
>> + (offsetof(struct acpi_table_tpm2,
>> + start_method_specific) +
> Should be
>
> offsetof(struct acpti_table_tpm2, log_area_minimum_length)
The old code had sizeof(*buf) with buf being 'struct acpi_table_tpm2'
and that was equivalent to offsetof(struct acpi_table_tpm2,
start_method_specific) since 'start_method_specific' is the first new
field that we are adding right here. Also see 3rd paragraph in the patch
description. The replacement rule described there should apply to all
sizeof() calculations on 'struct acpi_table_tpm2.'
I'll address the other issues.
Stefan
^ permalink raw reply
* Re: [PATCH v5 1/2] acpi: Extend TPM2 ACPI table with missing log fields
From: Jarkko Sakkinen @ 2020-06-25 0:00 UTC (permalink / raw)
To: Stefan Berger, Jiandi An
Cc: linux-integrity, linux-kernel, linux-acpi, linux-security-module,
Stefan Berger, Rafael J . Wysocki
In-Reply-To: <20200623120636.1453470-2-stefanb@linux.vnet.ibm.com>
On Tue, Jun 23, 2020 at 08:06:35AM -0400, Stefan Berger wrote:
> From: Stefan Berger <stefanb@linux.ibm.com>
>
> Recent extensions of the TPM2 ACPI table added 3 more fields
> including 12 bytes of start method specific parameters and Log Area
> Minimum Length (u32) and Log Area Start Address (u64). So, we extend
> the existing structure with these fields to allow non-UEFI systems
> to access the TPM2's log.
>
> The specification that has the new fields is the following:
> TCG ACPI Specification
> Family "1.2" and "2.0"
> Version 1.2, Revision 8
>
> Adapt all existing table size calculations to use
> offsetof(struct acpi_table_tpm2, start_method_specific)
> [where start_method_specific is a newly added field]
> rather than sizeof(struct acpi_table_tpm2) so that the addition
> of the new fields does not affect current systems that may not
> have them.
>
I found at least one regression from this patch. Please remove my
reviewed-by comment form the next version.
Should have:
Link: https://trustedcomputinggroup.org/wp-content/uploads/TCG_ACPIGeneralSpecification_v1.20_r8.pdf
Please, add this.
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> Cc: linux-acpi@vger.kernel.org
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
> drivers/char/tpm/tpm_crb.c | 13 ++++++++++---
> drivers/char/tpm/tpm_tis.c | 4 +++-
> include/acpi/actbl3.h | 5 +++--
> 3 files changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index a9dcf31eadd2..0565aa5482f9 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -669,7 +669,9 @@ static int crb_acpi_add(struct acpi_device *device)
>
> status = acpi_get_table(ACPI_SIG_TPM2, 1,
> (struct acpi_table_header **) &buf);
> - if (ACPI_FAILURE(status) || buf->header.length < sizeof(*buf)) {
> + if (ACPI_FAILURE(status) || buf->header.length <
> + offsetof(struct acpi_table_tpm2,
> + start_method_specific)) {
> dev_err(dev, FW_BUG "failed to get TPM2 ACPI table\n");
> return -EINVAL;
> }
> @@ -684,14 +686,19 @@ static int crb_acpi_add(struct acpi_device *device)
> return -ENOMEM;
>
> if (sm == ACPI_TPM2_COMMAND_BUFFER_WITH_ARM_SMC) {
> - if (buf->header.length < (sizeof(*buf) + sizeof(*crb_smc))) {
> + if (buf->header.length <
> + (offsetof(struct acpi_table_tpm2,
> + start_method_specific) +
Should be
offsetof(struct acpti_table_tpm2, log_area_minimum_length)
> + sizeof(*crb_smc))) {
> dev_err(dev,
> FW_BUG "TPM2 ACPI table has wrong size %u for start method type %d\n",
> buf->header.length,
> ACPI_TPM2_COMMAND_BUFFER_WITH_ARM_SMC);
> return -EINVAL;
> }
> - crb_smc = ACPI_ADD_PTR(struct tpm2_crb_smc, buf, sizeof(*buf));
> + crb_smc = ACPI_ADD_PTR(struct tpm2_crb_smc, buf,
> + offsetof(struct acpi_table_tpm2,
> + start_method_specific));
> priv->smc_func_id = crb_smc->smc_func_id;
> }
>
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index e7df342a317d..a80f36860bac 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -111,7 +111,9 @@ static int check_acpi_tpm2(struct device *dev)
> */
> st =
> acpi_get_table(ACPI_SIG_TPM2, 1, (struct acpi_table_header **)&tbl);
> - if (ACPI_FAILURE(st) || tbl->header.length < sizeof(*tbl)) {
> + if (ACPI_FAILURE(st) || tbl->header.length <
> + offsetof(struct acpi_table_tpm2,
> + start_method_specific)) {
> dev_err(dev, FW_BUG "failed to get TPM2 ACPI table\n");
> return -EINVAL;
> }
> diff --git a/include/acpi/actbl3.h b/include/acpi/actbl3.h
> index b0b163b9efc6..8b55426bbcf6 100644
> --- a/include/acpi/actbl3.h
> +++ b/include/acpi/actbl3.h
> @@ -411,8 +411,9 @@ struct acpi_table_tpm2 {
> u16 reserved;
> u64 control_address;
> u32 start_method;
> -
> - /* Platform-specific data follows */
> + u8 start_method_specific[12];
> + u32 log_area_minimum_length; /* optional */
> + u64 log_area_start_address; /* optional */
'start_method_specific' is also optional. Please remove the optional
comments altogether as platform-specific data is by definition optional.
> };
>
> /* Values for start_method above */
> --
> 2.26.2
>
Please add this to the next version since we do not want to break ARM
side:
Cc: Jiandi An <anjiandi@codeaurora.org>
/Jarkko
^ permalink raw reply
* Re: [RFC][PATCH] net/bpfilter: Remove this broken and apparently unmantained
From: Tetsuo Handa @ 2020-06-24 23:14 UTC (permalink / raw)
To: Alexei Starovoitov, Eric W. Biederman
Cc: Linus Torvalds, Kees Cook, Andrew Morton, Alexei Starovoitov,
David Miller, Al Viro, bpf, linux-fsdevel, Daniel Borkmann,
Jakub Kicinski, Masahiro Yamada, Gary Lin, Bruno Meneguele,
linux-security-module, Casey Schaufler
In-Reply-To: <CAADnVQL8WrfV74v1ChvCKE=pQ_zo+A5EtEBB3CbD=P5ote8_MA@mail.gmail.com>
On 2020/06/24 23:26, Alexei Starovoitov wrote:
> On Wed, Jun 24, 2020 at 5:17 AM Eric W. Biederman <ebiederm@xmission.com> wrote:
>>
>> Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:
>>
>>> On Tue, Jun 23, 2020 at 01:53:48PM -0500, Eric W. Biederman wrote:
>>
>>> There is no refcnt bug. It was a user error on tomoyo side.
>>> fork_blob() works as expected.
>>
>> Nope. I have independently confirmed it myself.
>
> I guess you've tried Tetsuo's fork_blob("#!/bin/true") kernel module ?
> yes. that fails. It never meant to be used for this.
> With elf blob it works, but breaks if there are rejections
> in things like security_bprm_creds_for_exec().
> In my mind that path was 'must succeed or kernel module is toast'.
> Like passing NULL into a function that doesn't check for it.
> Working on a fix for that since Tetsuo cares.
>
What is unhappy for pathname based LSMs is that fork_usermode_blob() creates
a file with empty filename. I can imagine that somebody would start abusing
fork_usermode_blob() as an interface for starting programs like modprobe, hotplug,
udevd and sshd. When such situation happened, how fork_usermode_blob() provides
information for identifying the intent of such execve() requests?
fork_usermode_blob() might also be an unhappy behavior for inode based LSMs (like
SELinux and Smack) because it seems that fork_usermode_blob() can't have a chance
to associate appropriate security labels based on the content of the byte array
because files are created on-demand. Is fork_usermode_blob() friendly to inode
based LSMs?
^ permalink raw reply
* Re: [PATCH v3] ima_evm_utils: extended calc_bootaggr to PCRs 8 - 9
From: Bruno Meneguele @ 2020-06-24 21:50 UTC (permalink / raw)
To: Maurizio Drocco
Cc: Silviu.Vlasceanu, dmitry.kasatkin, jejb, jmorris, linux-integrity,
linux-kernel, linux-security-module, mdrocco, roberto.sassu,
serge, stefanb, zohar
In-Reply-To: <20200624213558.4265-1-maurizio.drocco@ibm.com>
[-- Attachment #1: Type: text/plain, Size: 1959 bytes --]
On Wed, Jun 24, 2020 at 05:35:58PM -0400, Maurizio Drocco wrote:
> From: Maurizio <maurizio.drocco@ibm.com>
>
> cal_bootaggr should include PCRs 8-9 in non-SHA1 digests.
>
> Signed-off-by: Maurizio Drocco <maurizio.drocco@ibm.com>
> ---
> Changelog:
> v3:
> - Fixed patch description
> v2:
> - Always include PCRs 8 & 9 to non-sha1 hashes
> v1:
> - Include non-zero PCRs 8 & 9 to boot aggregates
>
> src/evmctl.c | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/src/evmctl.c b/src/evmctl.c
> index 1d065ce..46b7092 100644
> --- a/src/evmctl.c
> +++ b/src/evmctl.c
> @@ -1930,6 +1930,16 @@ static void calc_bootaggr(struct tpm_bank_info *bank)
> }
> }
>
> + if (strcmp(bank->algo_name, "sha1") != 0) {
> + for (i = 8; i < 10; i++) {
> + err = EVP_DigestUpdate(pctx, bank->pcr[i], bank->digest_size);
> + if (!err) {
> + log_err("EVP_DigestUpdate() failed\n");
> + return;
> + }
> + }
> + }
> +
> err = EVP_DigestFinal(pctx, bank->digest, &mdlen);
> if (!err) {
> log_err("EVP_DigestFinal() failed\n");
> @@ -1972,8 +1982,9 @@ static int append_bootaggr(char *bootaggr, struct tpm_bank_info *tpm_banks)
> /*
> * The IMA measurement list boot_aggregate is the link between the preboot
> * event log and the IMA measurement list. Read and calculate all the
> - * possible per TPM bank boot_aggregate digests based on the existing
> - * PCRs 0 - 7 to validate against the IMA boot_aggregate record.
> + * possible per TPM bank boot_aggregate digests based on the existing PCRs
> + * 0 - 9 to validate against the IMA boot_aggregate record. If the digest
> + * algorithm is SHA1, only PCRs 0 - 7 are considered to avoid ambiguity.
> */
> static int cmd_ima_bootaggr(struct command *cmd)
> {
> --
> 2.17.1
>
Reviewed-by: Bruno Meneguele <bmeneg@redhat.com>
Thanks.
--
bmeneg
PGP Key: http://bmeneg.com/pubkey.txt
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* [PATCH v3] ima_evm_utils: extended calc_bootaggr to PCRs 8 - 9
From: Maurizio Drocco @ 2020-06-24 21:35 UTC (permalink / raw)
To: bmeneg
Cc: Silviu.Vlasceanu, dmitry.kasatkin, jejb, jmorris, linux-integrity,
linux-kernel, linux-security-module, maurizio.drocco, mdrocco,
roberto.sassu, serge, stefanb, zohar
In-Reply-To: <20200624213345.GB2639@glitch>
From: Maurizio <maurizio.drocco@ibm.com>
cal_bootaggr should include PCRs 8-9 in non-SHA1 digests.
Signed-off-by: Maurizio Drocco <maurizio.drocco@ibm.com>
---
Changelog:
v3:
- Fixed patch description
v2:
- Always include PCRs 8 & 9 to non-sha1 hashes
v1:
- Include non-zero PCRs 8 & 9 to boot aggregates
src/evmctl.c | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/src/evmctl.c b/src/evmctl.c
index 1d065ce..46b7092 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -1930,6 +1930,16 @@ static void calc_bootaggr(struct tpm_bank_info *bank)
}
}
+ if (strcmp(bank->algo_name, "sha1") != 0) {
+ for (i = 8; i < 10; i++) {
+ err = EVP_DigestUpdate(pctx, bank->pcr[i], bank->digest_size);
+ if (!err) {
+ log_err("EVP_DigestUpdate() failed\n");
+ return;
+ }
+ }
+ }
+
err = EVP_DigestFinal(pctx, bank->digest, &mdlen);
if (!err) {
log_err("EVP_DigestFinal() failed\n");
@@ -1972,8 +1982,9 @@ static int append_bootaggr(char *bootaggr, struct tpm_bank_info *tpm_banks)
/*
* The IMA measurement list boot_aggregate is the link between the preboot
* event log and the IMA measurement list. Read and calculate all the
- * possible per TPM bank boot_aggregate digests based on the existing
- * PCRs 0 - 7 to validate against the IMA boot_aggregate record.
+ * possible per TPM bank boot_aggregate digests based on the existing PCRs
+ * 0 - 9 to validate against the IMA boot_aggregate record. If the digest
+ * algorithm is SHA1, only PCRs 0 - 7 are considered to avoid ambiguity.
*/
static int cmd_ima_bootaggr(struct command *cmd)
{
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v2] ima_evm_utils: extended calc_bootaggr to PCRs 8 - 9
From: Bruno Meneguele @ 2020-06-24 21:33 UTC (permalink / raw)
To: Stefan Berger
Cc: Maurizio Drocco, zohar, Silviu.Vlasceanu, dmitry.kasatkin, jejb,
jmorris, linux-integrity, linux-kernel, linux-security-module,
mdrocco, roberto.sassu, serge
In-Reply-To: <92a0d170-8157-476b-8083-ae567b11f364@linux.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 2663 bytes --]
On Wed, Jun 24, 2020 at 05:17:52PM -0400, Stefan Berger wrote:
> On 6/23/20 2:13 PM, Bruno Meneguele wrote:
> > On Tue, Jun 23, 2020 at 02:01:22PM -0400, Maurizio Drocco wrote:
> > > From: Maurizio <maurizio.drocco@ibm.com>
> > >
> > > If PCRs 8 - 9 are set (i.e. not all-zeros), cal_bootaggr should include
> > > them into the digest.
>
>
> Wouldn't you have to check for not all-zeros in your code?
>
boot_aggregate in kernel, after the following patch be applied:
https://lkml.org/lkml/2020/6/23/833
is calculated regardless PCR 8 & 9 being zero or not.
Thus evmctl is only reflecting the same behavior.
I think it would be worth changing the commit log here.
>
> Stefan
>
>
> > >
> > > Signed-off-by: Maurizio Drocco <maurizio.drocco@ibm.com>
> > > ---
> > > Changelog:
> > > v2:
> > > - Always include PCRs 8 & 9 to non-sha1 hashes
> > > v1:
> > > - Include non-zero PCRs 8 & 9 to boot aggregates
> > >
> > > src/evmctl.c | 15 +++++++++++++--
> > > 1 file changed, 13 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/src/evmctl.c b/src/evmctl.c
> > > index 1d065ce..46b7092 100644
> > > --- a/src/evmctl.c
> > > +++ b/src/evmctl.c
> > > @@ -1930,6 +1930,16 @@ static void calc_bootaggr(struct tpm_bank_info *bank)
> > > }
> > > }
> > > + if (strcmp(bank->algo_name, "sha1") != 0) {
> > > + for (i = 8; i < 10; i++) {
> > > + err = EVP_DigestUpdate(pctx, bank->pcr[i], bank->digest_size);
> > > + if (!err) {
> > > + log_err("EVP_DigestUpdate() failed\n");
> > > + return;
> > > + }
> > > + }
> > > + }
> > > +
> > > err = EVP_DigestFinal(pctx, bank->digest, &mdlen);
> > > if (!err) {
> > > log_err("EVP_DigestFinal() failed\n");
> > > @@ -1972,8 +1982,9 @@ static int append_bootaggr(char *bootaggr, struct tpm_bank_info *tpm_banks)
> > > /*
> > > * The IMA measurement list boot_aggregate is the link between the preboot
> > > * event log and the IMA measurement list. Read and calculate all the
> > > - * possible per TPM bank boot_aggregate digests based on the existing
> > > - * PCRs 0 - 7 to validate against the IMA boot_aggregate record.
> > > + * possible per TPM bank boot_aggregate digests based on the existing PCRs
> > > + * 0 - 9 to validate against the IMA boot_aggregate record. If the digest
> > > + * algorithm is SHA1, only PCRs 0 - 7 are considered to avoid ambiguity.
> > > */
> > > static int cmd_ima_bootaggr(struct command *cmd)
> > > {
> > > --
> > > 2.17.1
> > >
> > Reviewed-by: Bruno Meneguele <bmeneg@redhat.com>
> >
>
--
bmeneg
PGP Key: http://bmeneg.com/pubkey.txt
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* [PATCH] ima_evm_utils: extended calc_bootaggr to PCRs 8 - 9
From: Maurizio Drocco @ 2020-06-24 21:33 UTC (permalink / raw)
To: stefanb
Cc: Silviu.Vlasceanu, bmeneg, dmitry.kasatkin, jejb, jmorris,
linux-integrity, linux-kernel, linux-security-module,
maurizio.drocco, mdrocco, roberto.sassu, serge, zohar
In-Reply-To: <92a0d170-8157-476b-8083-ae567b11f364@linux.ibm.com>
From: Maurizio <maurizio.drocco@ibm.com>
cal_bootaggr should include PCRs 8-9 in non-SHA1 digests.
Signed-off-by: Maurizio Drocco <maurizio.drocco@ibm.com>
---
Changelog:
v3:
- Fixed patch description
v2:
- Always include PCRs 8 & 9 to non-sha1 hashes
v1:
- Include non-zero PCRs 8 & 9 to boot aggregates
src/evmctl.c | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/src/evmctl.c b/src/evmctl.c
index 1d065ce..46b7092 100644
--- a/src/evmctl.c
+++ b/src/evmctl.c
@@ -1930,6 +1930,16 @@ static void calc_bootaggr(struct tpm_bank_info *bank)
}
}
+ if (strcmp(bank->algo_name, "sha1") != 0) {
+ for (i = 8; i < 10; i++) {
+ err = EVP_DigestUpdate(pctx, bank->pcr[i], bank->digest_size);
+ if (!err) {
+ log_err("EVP_DigestUpdate() failed\n");
+ return;
+ }
+ }
+ }
+
err = EVP_DigestFinal(pctx, bank->digest, &mdlen);
if (!err) {
log_err("EVP_DigestFinal() failed\n");
@@ -1972,8 +1982,9 @@ static int append_bootaggr(char *bootaggr, struct tpm_bank_info *tpm_banks)
/*
* The IMA measurement list boot_aggregate is the link between the preboot
* event log and the IMA measurement list. Read and calculate all the
- * possible per TPM bank boot_aggregate digests based on the existing
- * PCRs 0 - 7 to validate against the IMA boot_aggregate record.
+ * possible per TPM bank boot_aggregate digests based on the existing PCRs
+ * 0 - 9 to validate against the IMA boot_aggregate record. If the digest
+ * algorithm is SHA1, only PCRs 0 - 7 are considered to avoid ambiguity.
*/
static int cmd_ima_bootaggr(struct command *cmd)
{
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v2] ima_evm_utils: extended calc_bootaggr to PCRs 8 - 9
From: Stefan Berger @ 2020-06-24 21:17 UTC (permalink / raw)
To: Bruno Meneguele, Maurizio Drocco
Cc: zohar, Silviu.Vlasceanu, dmitry.kasatkin, jejb, jmorris,
linux-integrity, linux-kernel, linux-security-module, mdrocco,
roberto.sassu, serge
In-Reply-To: <20200623181357.GC4983@glitch>
On 6/23/20 2:13 PM, Bruno Meneguele wrote:
> On Tue, Jun 23, 2020 at 02:01:22PM -0400, Maurizio Drocco wrote:
>> From: Maurizio <maurizio.drocco@ibm.com>
>>
>> If PCRs 8 - 9 are set (i.e. not all-zeros), cal_bootaggr should include
>> them into the digest.
Wouldn't you have to check for not all-zeros in your code?
Stefan
>>
>> Signed-off-by: Maurizio Drocco <maurizio.drocco@ibm.com>
>> ---
>> Changelog:
>> v2:
>> - Always include PCRs 8 & 9 to non-sha1 hashes
>> v1:
>> - Include non-zero PCRs 8 & 9 to boot aggregates
>>
>> src/evmctl.c | 15 +++++++++++++--
>> 1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/evmctl.c b/src/evmctl.c
>> index 1d065ce..46b7092 100644
>> --- a/src/evmctl.c
>> +++ b/src/evmctl.c
>> @@ -1930,6 +1930,16 @@ static void calc_bootaggr(struct tpm_bank_info *bank)
>> }
>> }
>>
>> + if (strcmp(bank->algo_name, "sha1") != 0) {
>> + for (i = 8; i < 10; i++) {
>> + err = EVP_DigestUpdate(pctx, bank->pcr[i], bank->digest_size);
>> + if (!err) {
>> + log_err("EVP_DigestUpdate() failed\n");
>> + return;
>> + }
>> + }
>> + }
>> +
>> err = EVP_DigestFinal(pctx, bank->digest, &mdlen);
>> if (!err) {
>> log_err("EVP_DigestFinal() failed\n");
>> @@ -1972,8 +1982,9 @@ static int append_bootaggr(char *bootaggr, struct tpm_bank_info *tpm_banks)
>> /*
>> * The IMA measurement list boot_aggregate is the link between the preboot
>> * event log and the IMA measurement list. Read and calculate all the
>> - * possible per TPM bank boot_aggregate digests based on the existing
>> - * PCRs 0 - 7 to validate against the IMA boot_aggregate record.
>> + * possible per TPM bank boot_aggregate digests based on the existing PCRs
>> + * 0 - 9 to validate against the IMA boot_aggregate record. If the digest
>> + * algorithm is SHA1, only PCRs 0 - 7 are considered to avoid ambiguity.
>> */
>> static int cmd_ima_bootaggr(struct command *cmd)
>> {
>> --
>> 2.17.1
>>
> Reviewed-by: Bruno Meneguele <bmeneg@redhat.com>
>
^ permalink raw reply
* Re: [RFC][PATCH] net/bpfilter: Remove this broken and apparently unmantained
From: Casey Schaufler @ 2020-06-24 19:48 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Tetsuo Handa, linux-security-module, Eric W. Biederman,
Linus Torvalds, Kees Cook, Andrew Morton, Alexei Starovoitov,
David Miller, Al Viro, bpf, linux-fsdevel, Daniel Borkmann,
Jakub Kicinski, Masahiro Yamada, Gary Lin, Bruno Meneguele
In-Reply-To: <20200624175408.kwc562ofnfhmy674@ast-mbp.dhcp.thefacebook.com>
On 6/24/2020 10:54 AM, Alexei Starovoitov wrote:
> On Wed, Jun 24, 2020 at 08:41:37AM -0700, Casey Schaufler wrote:
>> On 6/24/2020 12:05 AM, Tetsuo Handa wrote:
>>> Forwarding to LSM-ML again. Any comments?
>> Hey, BPF folks - you *really* need to do better about keeping the LSM
>> community in the loop when you're discussing LSM issues.
>>
>>> On 2020/06/24 15:39, Alexei Starovoitov wrote:
>>>> On Wed, Jun 24, 2020 at 01:58:33PM +0900, Tetsuo Handa wrote:
>>>>> On 2020/06/24 13:00, Alexei Starovoitov wrote:
>>>>>>> However, regarding usermode_blob, although the byte array (which contains code / data)
>>>>>>> might be initially loaded from the kernel space (which is protected), that byte array
>>>>>>> is no longer protected (e.g. SIGKILL, strace()) when executed because they are placed
>>>>>>> in the user address space. Thus, LSM modules (including pathname based security) want
>>>>>>> to control how that byte array can behave.
>>>>>> It's privileged memory regardless. root can poke into kernel or any process memory.
>>>>> LSM is there to restrict processes running as "root".
>>>> hmm. do you really mean that it's possible for an LSM to restrict CAP_SYS_ADMIN effectively?
>> I think that SELinux works hard to do just that. SELinux implements it's own
>> privilege model that is tangential to the capabilities model.
> of course. no argument here.
>
>> More directly, it is simple to create a security module to provide finer privilege
>> granularity than capabilities. I have one lurking in a source tree, and I would
>> be surprised if it's the only one waiting for the next round of LSM stacking.
> no one is arguing with that either.
>
>>>> LSM can certainly provide extra level of foolproof-ness against accidental
>>>> mistakes, but it's not a security boundary.
>> Gasp! Them's fight'n words. How do you justify such an outrageous claim?
> .. for root user processes.
> What's outrageous about that?
> Did you capture the context or just replying to few sentences out of the context?
As I mentioned above, you need to include the LSM list in these discussions.
If you don't want "out of context" comments. I replied to what's presented.
And regardless of the context, saying that an LSM can't provide a security
boundary for "root user processes" is just wrong. Obviously there's been more
to the conversation than is included here.
^ permalink raw reply
* Re: linux-next: umh: fix processed error when UMH_WAIT_PROC is used seems to break linux bridge on s390x (bisected)
From: Christian Borntraeger @ 2020-06-24 18:37 UTC (permalink / raw)
To: Luis Chamberlain
Cc: Christoph Hellwig, ast, axboe, bfields, bridge, chainsaw,
christian.brauner, chuck.lever, davem, dhowells, gregkh,
jarkko.sakkinen, jmorris, josh, keescook, keyrings, kuba,
lars.ellenberg, linux-fsdevel, linux-kernel, linux-nfs,
linux-security-module, nikolay, philipp.reisner, ravenexp, roopa,
serge, slyfox, viro, yangtiezhu, netdev, markward, linux-s390
In-Reply-To: <4d8fbcea-a892-3453-091f-d57c03f9aa90@de.ibm.com>
On 24.06.20 20:32, Christian Borntraeger wrote:
[...]>
> So the translations look correct. But your change is actually a sematic change
> if(ret) will only trigger if there is an error
> if (KWIFEXITED(ret)) will always trigger when the process ends. So we will always overwrite -ECHILD
> and we did not do it before.
>
So the right fix is
diff --git a/kernel/umh.c b/kernel/umh.c
index f81e8698e36e..a3a3196e84d1 100644
--- a/kernel/umh.c
+++ b/kernel/umh.c
@@ -154,7 +154,7 @@ static void call_usermodehelper_exec_sync(struct subprocess_info *sub_info)
* the real error code is already in sub_info->retval or
* sub_info->retval is 0 anyway, so don't mess with it then.
*/
- if (KWIFEXITED(ret))
+ if (KWEXITSTATUS(ret))
sub_info->retval = KWEXITSTATUS(ret);
}
I think.
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox