From: Hans de Goede <hdegoede@redhat.com> To: "Luis R. Rodriguez" <mcgrof@kernel.org>, Mimi Zohar <zohar@linux.vnet.ibm.com>, Kees Cook <keescook@chromium.org> Cc: Darren Hart <dvhart@infradead.org>, Andy Shevchenko <andy@infradead.org>, Ard Biesheuvel <ard.biesheuvel@linaro.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>, "H . Peter Anvin" <hpa@zytor.com>, platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org, Peter Jones <pjones@redhat.com>, Dave Olsthoorn <dave@bewaar.me>, Will Deacon <will.deacon@arm.com>, Andy Lutomirski <luto@kernel.org>, Matt Fleming <matt@codeblueprint.co.uk>, David Howells <dhowells@redhat.com>, Josh Triplett <josh@joshtriplett.org>, dmitry.torokhov@gmail.com, mfuzzey@parkeon.com, Kalle Valo <kvalo@codeaurora.org>, Arend Van Spriel <arend.vanspriel@broadcom.com>, Linus Torvalds <torvalds@linux-foundation.org>, nbroeking@me.com, bjorn.andersson@linaro.org, Torsten Duwe <duwe@suse.de>, x86@kernel.org, linux-efi@vger.kernel.org Subject: Re: [PATCH v3 2/5] efi: Add embedded peripheral firmware support Date: Tue, 24 Apr 2018 17:09:52 +0200 [thread overview] Message-ID: <71e6a45a-398d-b7a4-dab0-8b9936683226@redhat.com> (raw) In-Reply-To: <20180423211143.GZ14440@wotan.suse.de> Hi, On 23-04-18 23:11, Luis R. Rodriguez wrote: > Hans, please see use of READING_FIRMWARE_PREALLOC_BUFFER, we'll need a new ID > and security for this type of request so IMA can reject it if the policy is > configured for it. Hmm, interesting, actually it seems like the whole existence of READING_FIRMWARE_PREALLOC_BUFFER is a mistake, the IMA framework really does not care if we are loading the firmware into memory allocated by the firmware-loader code, or into memory allocated by the device-driver requesting the firmware. As such the current IMA code (from v4.17-rc2) actually does not handle READING_FIRMWARE_PREALLOC_BUFFER at all, here are bits of code from: security/integrity/ima/ima_main.c: static int read_idmap[READING_MAX_ID] = { [READING_FIRMWARE] = FIRMWARE_CHECK, [READING_MODULE] = MODULE_CHECK, [READING_KEXEC_IMAGE] = KEXEC_KERNEL_CHECK, [READING_KEXEC_INITRAMFS] = KEXEC_INITRAMFS_CHECK, [READING_POLICY] = POLICY_CHECK }; int ima_post_read_file(struct file *file, void *buf, loff_t size, ... if (!file && read_id == READING_FIRMWARE) { if ((ima_appraise & IMA_APPRAISE_FIRMWARE) && (ima_appraise & IMA_APPRAISE_ENFORCE)) return -EACCES; /* INTEGRITY_UNKNOWN */ return 0; } Which show that the IMA code is not handling READING_FIRMWARE_PREALLOC_BUFFER as it should (I believe it should handle it the same as READING_FIRMWARE). Now we could fix that, but the only user of READING_FIRMWARE_PREALLOC_BUFFER is the code which originally introduced it: https://patchwork.kernel.org/patch/9162011/ So I believe it might be better to instead replace it with just READING_FIRMWARE and find another way to tell kernel_read_file() that there is a pre-allocated buffer, perhaps the easiest way there is that *buf must be NULL when the caller wants kernel_read_file() to vmalloc the mem. This would of course require auditing all callers that the buf which the pass in is initialized to NULL. Either way adding a third READING_FIRMWARE_FOO to the kernel_read_file_id enum seems like a bad idea, from the IMA pov firmware is firmware. What this whole exercise has shown me though is that I need to call security_kernel_post_read_file() when loading EFI embedded firmware. I will add a call to security_kernel_post_read_file() for v4 of the patch-set. > Please Cc Kees in future patches. Will do. Regards, Hans
WARNING: multiple messages have this Message-ID (diff)
From: Hans de Goede <hdegoede@redhat.com> To: "Luis R. Rodriguez" <mcgrof@kernel.org>, Mimi Zohar <zohar@linux.vnet.ibm.com>, Kees Cook <keescook@chromium.org> Cc: Darren Hart <dvhart@infradead.org>, Andy Shevchenko <andy@infradead.org>, Ard Biesheuvel <ard.biesheuvel@linaro.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>, "H . Peter Anvin" <hpa@zytor.com>, platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org, Peter Jones <pjones@redhat.com>, Dave Olsthoorn <dave@bewaar.me>, Will Deacon <will.deacon@arm.com>, Andy Lutomirski <luto@kernel.org>, Matt Fleming <matt@codeblueprint.co.uk>, David Howells <dhowells@redhat.com>, Josh Triplett <josh@joshtriplett.org>, dmitry.torokhov@gmail.com, mfuzzey@parkeon.com, Kalle Valo <kvalo@codeaurora.org>, Arend Van Spriel <arend.vanspriel@broadcom.com>, Linus Torvalds <torvalds@lin> Subject: Re: [PATCH v3 2/5] efi: Add embedded peripheral firmware support Date: Tue, 24 Apr 2018 17:09:52 +0200 [thread overview] Message-ID: <71e6a45a-398d-b7a4-dab0-8b9936683226@redhat.com> (raw) In-Reply-To: <20180423211143.GZ14440@wotan.suse.de> Hi, On 23-04-18 23:11, Luis R. Rodriguez wrote: > Hans, please see use of READING_FIRMWARE_PREALLOC_BUFFER, we'll need a new ID > and security for this type of request so IMA can reject it if the policy is > configured for it. Hmm, interesting, actually it seems like the whole existence of READING_FIRMWARE_PREALLOC_BUFFER is a mistake, the IMA framework really does not care if we are loading the firmware into memory allocated by the firmware-loader code, or into memory allocated by the device-driver requesting the firmware. As such the current IMA code (from v4.17-rc2) actually does not handle READING_FIRMWARE_PREALLOC_BUFFER at all, here are bits of code from: security/integrity/ima/ima_main.c: static int read_idmap[READING_MAX_ID] = { [READING_FIRMWARE] = FIRMWARE_CHECK, [READING_MODULE] = MODULE_CHECK, [READING_KEXEC_IMAGE] = KEXEC_KERNEL_CHECK, [READING_KEXEC_INITRAMFS] = KEXEC_INITRAMFS_CHECK, [READING_POLICY] = POLICY_CHECK }; int ima_post_read_file(struct file *file, void *buf, loff_t size, ... if (!file && read_id == READING_FIRMWARE) { if ((ima_appraise & IMA_APPRAISE_FIRMWARE) && (ima_appraise & IMA_APPRAISE_ENFORCE)) return -EACCES; /* INTEGRITY_UNKNOWN */ return 0; } Which show that the IMA code is not handling READING_FIRMWARE_PREALLOC_BUFFER as it should (I believe it should handle it the same as READING_FIRMWARE). Now we could fix that, but the only user of READING_FIRMWARE_PREALLOC_BUFFER is the code which originally introduced it: https://patchwork.kernel.org/patch/9162011/ So I believe it might be better to instead replace it with just READING_FIRMWARE and find another way to tell kernel_read_file() that there is a pre-allocated buffer, perhaps the easiest way there is that *buf must be NULL when the caller wants kernel_read_file() to vmalloc the mem. This would of course require auditing all callers that the buf which the pass in is initialized to NULL. Either way adding a third READING_FIRMWARE_FOO to the kernel_read_file_id enum seems like a bad idea, from the IMA pov firmware is firmware. What this whole exercise has shown me though is that I need to call security_kernel_post_read_file() when loading EFI embedded firmware. I will add a call to security_kernel_post_read_file() for v4 of the patch-set. > Please Cc Kees in future patches. Will do. Regards, Hans
next prev parent reply other threads:[~2018-04-24 15:09 UTC|newest] Thread overview: 101+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-04-08 17:40 [PATCH v3 0/5] efi/firmware/platform-x86: Add EFI embedded fw support Hans de Goede 2018-04-08 17:40 ` [PATCH v3 1/5] efi: Export boot-services code and data as debugfs-blobs Hans de Goede 2018-04-16 8:23 ` Ard Biesheuvel 2018-04-16 8:23 ` Ard Biesheuvel 2018-04-24 13:11 ` Hans de Goede 2018-04-24 13:11 ` Hans de Goede 2018-04-23 11:55 ` Greg Kroah-Hartman 2018-04-23 11:55 ` Greg Kroah-Hartman 2018-04-08 17:40 ` [PATCH v3 2/5] efi: Add embedded peripheral firmware support Hans de Goede 2018-04-16 8:28 ` Ard Biesheuvel 2018-04-16 8:28 ` Ard Biesheuvel 2018-04-24 13:17 ` Hans de Goede 2018-04-24 13:17 ` Hans de Goede 2018-04-17 0:17 ` Luis R. Rodriguez 2018-04-17 0:17 ` Luis R. Rodriguez 2018-04-17 8:58 ` Hans de Goede 2018-04-17 8:58 ` Hans de Goede 2018-04-17 9:19 ` Hans de Goede 2018-04-17 9:19 ` Hans de Goede 2018-04-23 21:11 ` Luis R. Rodriguez 2018-04-23 21:11 ` Luis R. Rodriguez 2018-04-24 15:09 ` Hans de Goede [this message] 2018-04-24 15:09 ` Hans de Goede 2018-04-24 16:07 ` Mimi Zohar 2018-04-24 16:07 ` Mimi Zohar 2018-04-24 18:33 ` Hans de Goede 2018-04-24 18:33 ` Hans de Goede 2018-04-24 23:42 ` Luis R. Rodriguez 2018-04-24 23:42 ` Luis R. Rodriguez 2018-04-24 23:42 ` Luis R. Rodriguez 2018-04-25 5:00 ` Mimi Zohar 2018-04-25 5:00 ` Mimi Zohar 2018-04-25 5:00 ` Mimi Zohar 2018-04-25 17:55 ` Luis R. Rodriguez 2018-04-25 17:55 ` Luis R. Rodriguez 2018-04-25 17:55 ` Luis R. Rodriguez 2018-05-04 0:21 ` Luis R. Rodriguez 2018-05-04 0:21 ` Luis R. Rodriguez 2018-05-04 0:21 ` Luis R. Rodriguez 2018-05-04 15:26 ` Martijn Coenen 2018-05-04 15:26 ` Martijn Coenen 2018-05-04 15:26 ` Martijn Coenen 2018-05-04 19:44 ` Martijn Coenen 2018-05-04 19:44 ` Martijn Coenen 2018-05-04 19:44 ` Martijn Coenen 2018-05-08 15:38 ` Luis R. Rodriguez 2018-05-08 15:38 ` Luis R. Rodriguez 2018-05-08 15:38 ` Luis R. Rodriguez 2018-05-08 16:10 ` Luis R. Rodriguez 2018-05-08 16:10 ` Luis R. Rodriguez 2018-05-08 16:10 ` Luis R. Rodriguez 2018-06-07 16:49 ` Bjorn Andersson 2018-06-07 16:49 ` Bjorn Andersson 2018-06-07 16:49 ` Bjorn Andersson 2018-06-07 18:22 ` Luis R. Rodriguez 2018-06-07 18:22 ` Luis R. Rodriguez 2018-06-07 18:22 ` Luis R. Rodriguez 2018-06-01 19:23 ` Luis R. Rodriguez 2018-06-01 19:23 ` Luis R. Rodriguez 2018-06-01 19:23 ` Luis R. Rodriguez 2018-06-06 20:32 ` Do Qualcomm drivers use DMA buffers for request_firmware_into_buf()? Luis R. Rodriguez 2018-06-06 20:32 ` Luis R. Rodriguez 2018-06-06 20:32 ` Luis R. Rodriguez 2018-06-06 20:41 ` Ard Biesheuvel 2018-06-06 22:29 ` Luis R. Rodriguez 2018-06-06 22:41 ` Ard Biesheuvel 2018-06-06 22:55 ` Luis R. Rodriguez 2018-06-07 16:18 ` Bjorn Andersson 2018-06-07 16:18 ` Bjorn Andersson 2018-06-07 16:18 ` Bjorn Andersson 2018-06-07 16:18 ` Bjorn Andersson 2018-06-07 16:23 ` Ard Biesheuvel 2018-06-07 16:33 ` Greg Kroah-Hartman 2018-06-07 16:43 ` Ard Biesheuvel 2018-06-07 16:49 ` Greg Kroah-Hartman 2018-06-07 16:56 ` Ard Biesheuvel 2018-06-07 18:21 ` Bjorn Andersson 2018-06-07 18:42 ` Ard Biesheuvel 2018-06-26 0:08 ` Bjorn Andersson 2018-06-27 18:00 ` Luis R. Rodriguez 2018-06-27 22:21 ` Ard Biesheuvel 2018-06-27 23:33 ` Luis R. Rodriguez 2018-06-27 23:42 ` Ard Biesheuvel 2018-06-27 23:50 ` Luis R. Rodriguez 2018-06-08 6:41 ` Vlastimil Babka 2018-06-08 6:41 ` Vlastimil Babka 2018-06-08 6:41 ` Vlastimil Babka 2018-06-07 18:06 ` Bjorn Andersson 2018-06-18 23:49 ` Luis R. Rodriguez 2018-06-07 16:33 ` [PATCH v3 2/5] efi: Add embedded peripheral firmware support Bjorn Andersson 2018-06-07 16:33 ` Bjorn Andersson 2018-06-07 16:33 ` Bjorn Andersson 2018-04-08 17:40 ` [PATCH v3 3/5] platform/x86: Rename silead_dmi to touchscreen_dmi Hans de Goede 2018-04-09 8:07 ` Andy Shevchenko 2018-04-09 8:07 ` Andy Shevchenko 2018-04-20 0:20 ` Darren Hart 2018-04-20 0:20 ` Darren Hart 2018-04-08 17:40 ` [PATCH v3 4/5] platform/x86: touchscreen_dmi: Add EFI embedded firmware info support Hans de Goede 2018-04-08 17:40 ` [PATCH v3 5/5] platform/x86: touchscreen_dmi: Add info for the Chuwi Vi8 Plus tablet Hans de Goede 2018-04-09 8:10 ` Andy Shevchenko 2018-04-09 8:10 ` Andy Shevchenko
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=71e6a45a-398d-b7a4-dab0-8b9936683226@redhat.com \ --to=hdegoede@redhat.com \ --cc=andy@infradead.org \ --cc=ard.biesheuvel@linaro.org \ --cc=arend.vanspriel@broadcom.com \ --cc=bjorn.andersson@linaro.org \ --cc=dave@bewaar.me \ --cc=dhowells@redhat.com \ --cc=dmitry.torokhov@gmail.com \ --cc=duwe@suse.de \ --cc=dvhart@infradead.org \ --cc=gregkh@linuxfoundation.org \ --cc=hpa@zytor.com \ --cc=josh@joshtriplett.org \ --cc=keescook@chromium.org \ --cc=kvalo@codeaurora.org \ --cc=linux-efi@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=luto@kernel.org \ --cc=matt@codeblueprint.co.uk \ --cc=mcgrof@kernel.org \ --cc=mfuzzey@parkeon.com \ --cc=mingo@redhat.com \ --cc=nbroeking@me.com \ --cc=pjones@redhat.com \ --cc=platform-driver-x86@vger.kernel.org \ --cc=tglx@linutronix.de \ --cc=torvalds@linux-foundation.org \ --cc=will.deacon@arm.com \ --cc=x86@kernel.org \ --cc=zohar@linux.vnet.ibm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.