Re: [RFC PATCH v4 7/8] ima: based on policy prevent loading firmware (pre-allocated buffer)
On Wed, Jun 06, 2018 at 08:20:17AM +0200, Ard Biesheuvel wrote:
> On 6 June 2018 at 00:37, Kees Cook wrote:
> > On Fri, Jun 1, 2018 at 12:25 PM, Luis R. Rodriguez
> > wrote:
> >> On Fri, Jun 01, 2018 at 09:15:45PM +0200, Luis R. Rodriguez wrote:
> >>> On Tue, May 29, 2018 at 02:01:59PM -0400, Mimi Zohar wrote:
> >>> > Some systems are memory constrained but they need to load very large
> >>> > firmwares. The firmware subsystem allows drivers to request this
> >>> > firmware be loaded from the filesystem, but this requires that the
> >>> > entire firmware be loaded into kernel memory first before it's provided
> >>> > to the driver. This can lead to a situation where we map the firmware
> >>> > twice, once to load the firmware into kernel memory and once to copy the
> >>> > firmware into the final resting place.
> >>> >
> >>> > To resolve this problem, commit a098ecd2fa7d ("firmware: support loading
> >>> > into a pre-allocated buffer") introduced request_firmware_into_buf() API
> >>> > that allows drivers to request firmware be loaded directly into a
> >>> > pre-allocated buffer. The QCOM_MDT_LOADER calls dma_alloc_coherent() to
> >>> > allocate this buffer. According to Documentation/DMA-API.txt,
> >>> >
> >>> > Consistent memory is memory for which a write by either the
> >>> > device or the processor can immediately be read by the processor
> >>> > or device without having to worry about caching effects. (You
> >>> > may however need to make sure to flush the processor's write
> >>> > buffers before telling devices to read that memory.)
> >>> >
> >>> > Devices using pre-allocated DMA memory run the risk of the firmware
> >>> > being accessible by the device prior to the kernel's firmware signature
> >>> > verification has completed.
> >>>
> >>> Indeed. And since its DMA memory we have *no idea* what can happen in
> >>> terms of consumption of this firmware from hardware, when it would start
> >>> consuming it in particular.
> >>>
> >>> If the device has its own hardware firmware verification mechanism this is
> >>> completely obscure to us, but it may however suffice certain security
> >>> policies.
> >>>
> >>> The problem here lies in the conflicting security policies of the kernel
> >>> wanting
> >>> to not give away firmware until its complete and the current inability to
> >>> enable
> >>> us to have platforms suggest they trust hardware won't do something
> >>> stupid.
> >>> This becomes an issue since the semantics of the firmware API preallocated
> >>> buffer do not require currently allow the kernel to inform LSMs of the
> >>> fact
> >>> that a buffer is DMA memory or not, and a way for certain platforms then
> >>> to say that such use is fine for specific devices.
> >>>
> >>> Given a pointer can we determine if a piece of memory is DMA or not?
> >>
> >> FWIW
> >>
> >> Vlastimil suggests page_zone() or virt_to_page() may be able to.
> >
> > I don't see a PAGEFLAG for DMA, but I do see ZONE_DMA for
> > page_zone()... So maybe something like
> >
> > struct page *page;
> >
> > page = virt_to_page(address);
> > if (!page)
> >fail closed...
> > if (page_zone(page) == ZONE_DMA)
> > handle dma case...
> > else
> > non-dma
> >
> > But I've CCed Laura and Rik, who I always lean on when I have these
> > kinds of page questions...
> >
>
> That is not going to help. In general, DMA can access any memory in
> the system (unless a IOMMU is actively preventing that).
>
> The streaming DMA API allows you to map()/unmap() arbitrary pieces of
> memory for DMA, regardless of how they were allocated. (Some drivers
> were even doing DMA from the stack at some point, but this broke
> vmapped stacks so most of these cases have been fixed) Uploading
> firmware to a device does not require a coherent (as opposed to
> streaming) mapping for DMA, and so it is perfectly reasonable for a
> driver to use the streaming API to map the firmware image (wherever it
> is in memory) and map it.
This is useful thanks!
But let's keep in mind that this isn't about whether or not this should be
done. This is about informing se
Re: [PATCH v4 5/8] ima: based on policy require signed firmware (sysfs fallback)
On Fri, Jun 01, 2018 at 06:39:55PM -0400, Mimi Zohar wrote:
> On Fri, 2018-06-01 at 20:21 +0200, Luis R. Rodriguez wrote:
> > On Tue, May 29, 2018 at 02:01:57PM -0400, Mimi Zohar wrote:
> > > Luis, is the security_kernel_post_read_file LSM hook in
> > > firmware_loading_store() still needed after this patch? Should it be
> > > calling security_kernel_load_data() instead?
> >
> > That's up to Kees to decide as he added that hook, and knows
> > what LSMs may be doing with it. From my perspective it is confusing
> > to have that hook there so I think it could be removed now.
> >
> > Kees?
>
> Commit 6593d92 ("firmware_class: perform new LSM checks") references
> two methods of loading firmware - filesystem-found firmware and
> demand-loaded blobs. I assume this call in firmware_loading_store()
> is the demand-loaded blobs. Does that method still exist? Is it
> still being used?
Yeah its the stupid sysfs interface. So likely loadpin needs porting
as you IMA as you did.
Luis
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [RFC PATCH v4 7/8] ima: based on policy prevent loading firmware (pre-allocated buffer)
On Fri, Jun 01, 2018 at 09:15:45PM +0200, Luis R. Rodriguez wrote:
> On Tue, May 29, 2018 at 02:01:59PM -0400, Mimi Zohar wrote:
> > Some systems are memory constrained but they need to load very large
> > firmwares. The firmware subsystem allows drivers to request this
> > firmware be loaded from the filesystem, but this requires that the
> > entire firmware be loaded into kernel memory first before it's provided
> > to the driver. This can lead to a situation where we map the firmware
> > twice, once to load the firmware into kernel memory and once to copy the
> > firmware into the final resting place.
> >
> > To resolve this problem, commit a098ecd2fa7d ("firmware: support loading
> > into a pre-allocated buffer") introduced request_firmware_into_buf() API
> > that allows drivers to request firmware be loaded directly into a
> > pre-allocated buffer. The QCOM_MDT_LOADER calls dma_alloc_coherent() to
> > allocate this buffer. According to Documentation/DMA-API.txt,
> >
> > Consistent memory is memory for which a write by either the
> > device or the processor can immediately be read by the processor
> > or device without having to worry about caching effects. (You
> > may however need to make sure to flush the processor's write
> > buffers before telling devices to read that memory.)
> >
> > Devices using pre-allocated DMA memory run the risk of the firmware
> > being accessible by the device prior to the kernel's firmware signature
> > verification has completed.
>
> Indeed. And since its DMA memory we have *no idea* what can happen in
> terms of consumption of this firmware from hardware, when it would start
> consuming it in particular.
>
> If the device has its own hardware firmware verification mechanism this is
> completely obscure to us, but it may however suffice certain security
> policies.
>
> The problem here lies in the conflicting security policies of the kernel
> wanting
> to not give away firmware until its complete and the current inability to
> enable
> us to have platforms suggest they trust hardware won't do something stupid.
> This becomes an issue since the semantics of the firmware API preallocated
> buffer do not require currently allow the kernel to inform LSMs of the fact
> that a buffer is DMA memory or not, and a way for certain platforms then
> to say that such use is fine for specific devices.
>
> Given a pointer can we determine if a piece of memory is DMA or not?
FWIW
Vlastimil suggests page_zone() or virt_to_page() may be able to.
Luis
> Seems
> hacky to use such inferences if we had them anyway... but worth asking...
>
> I would suggest we augment the prealloc buffer firmware API to pass a
> flags argument which helps describe the preallocated buffer, and for now
> allow us to enable callers to describe if the buffer is kernel memory or
> DMA memory, and then have the LSMs decide based on this information as well.
> The qualcomm driver would change to use the DMA flag, and IMA would in turn
> deny such uses. Once and if platforms want to trust the DMA flag they can
> later add infrastructure for specifying this somehow.
>
> > Loading firmware already calls the security_kernel_read_file LSM hook.
> > With an IMA policy requiring signed firmware, this patch prevents
> > loading firmware into a pre-allocated buffer.
> >
> > Signed-off-by: Mimi Zohar
> > Cc: Luis R. Rodriguez
> > Cc: David Howells
> > Cc: Kees Cook
> > Cc: Serge E. Hallyn
> > Cc: Stephen Boyd
> > ---
> > security/integrity/ima/ima_main.c | 26 +-
> > 1 file changed, 17 insertions(+), 9 deletions(-)
> >
> > diff --git a/security/integrity/ima/ima_main.c
> > b/security/integrity/ima/ima_main.c
> > index 4a87f78098c8..3dae605a1604 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -419,6 +419,15 @@ void ima_post_path_mknod(struct dentry *dentry)
> > iint->flags |= IMA_NEW_FILE;
> > }
> >
> > +static int read_idmap[READING_MAX_ID] = {
> > + [READING_FIRMWARE] = FIRMWARE_CHECK,
> > + [READING_FIRMWARE_PREALLOC_BUFFER] = FIRMWARE_CHECK,
> > + [READING_MODULE] = MODULE_CHECK,
> > + [READING_KEXEC_IMAGE] = KEXEC_KERNEL_CHECK,
> > + [READING_KEXEC_INITRAMFS] = KEXEC_INITRAMFS_CHECK,
> > + [READING_POLICY] = POLICY_CHECK
> > +};
> > +
> > /**
> > * ima_read_file - pre-measure/appraise hook decision based on policy
> > * @file: pointer to the file to be measured/appraised/audit
> >
Re: [RFC PATCH v4 7/8] ima: based on policy prevent loading firmware (pre-allocated buffer)
On Tue, May 29, 2018 at 02:01:59PM -0400, Mimi Zohar wrote:
> Some systems are memory constrained but they need to load very large
> firmwares. The firmware subsystem allows drivers to request this
> firmware be loaded from the filesystem, but this requires that the
> entire firmware be loaded into kernel memory first before it's provided
> to the driver. This can lead to a situation where we map the firmware
> twice, once to load the firmware into kernel memory and once to copy the
> firmware into the final resting place.
>
> To resolve this problem, commit a098ecd2fa7d ("firmware: support loading
> into a pre-allocated buffer") introduced request_firmware_into_buf() API
> that allows drivers to request firmware be loaded directly into a
> pre-allocated buffer. The QCOM_MDT_LOADER calls dma_alloc_coherent() to
> allocate this buffer. According to Documentation/DMA-API.txt,
>
> Consistent memory is memory for which a write by either the
> device or the processor can immediately be read by the processor
> or device without having to worry about caching effects. (You
> may however need to make sure to flush the processor's write
> buffers before telling devices to read that memory.)
>
> Devices using pre-allocated DMA memory run the risk of the firmware
> being accessible by the device prior to the kernel's firmware signature
> verification has completed.
Indeed. And since its DMA memory we have *no idea* what can happen in
terms of consumption of this firmware from hardware, when it would start
consuming it in particular.
If the device has its own hardware firmware verification mechanism this is
completely obscure to us, but it may however suffice certain security policies.
The problem here lies in the conflicting security policies of the kernel wanting
to not give away firmware until its complete and the current inability to enable
us to have platforms suggest they trust hardware won't do something stupid.
This becomes an issue since the semantics of the firmware API preallocated
buffer do not require currently allow the kernel to inform LSMs of the fact
that a buffer is DMA memory or not, and a way for certain platforms then
to say that such use is fine for specific devices.
Given a pointer can we determine if a piece of memory is DMA or not? Seems
hacky to use such inferences if we had them anyway... but worth asking...
I would suggest we augment the prealloc buffer firmware API to pass a
flags argument which helps describe the preallocated buffer, and for now
allow us to enable callers to describe if the buffer is kernel memory or
DMA memory, and then have the LSMs decide based on this information as well.
The qualcomm driver would change to use the DMA flag, and IMA would in turn
deny such uses. Once and if platforms want to trust the DMA flag they can
later add infrastructure for specifying this somehow.
> Loading firmware already calls the security_kernel_read_file LSM hook.
> With an IMA policy requiring signed firmware, this patch prevents
> loading firmware into a pre-allocated buffer.
>
> Signed-off-by: Mimi Zohar
> Cc: Luis R. Rodriguez
> Cc: David Howells
> Cc: Kees Cook
> Cc: Serge E. Hallyn
> Cc: Stephen Boyd
> ---
> security/integrity/ima/ima_main.c | 26 +-
> 1 file changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/security/integrity/ima/ima_main.c
> b/security/integrity/ima/ima_main.c
> index 4a87f78098c8..3dae605a1604 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -419,6 +419,15 @@ void ima_post_path_mknod(struct dentry *dentry)
> iint->flags |= IMA_NEW_FILE;
> }
>
> +static int read_idmap[READING_MAX_ID] = {
> + [READING_FIRMWARE] = FIRMWARE_CHECK,
> + [READING_FIRMWARE_PREALLOC_BUFFER] = FIRMWARE_CHECK,
> + [READING_MODULE] = MODULE_CHECK,
> + [READING_KEXEC_IMAGE] = KEXEC_KERNEL_CHECK,
> + [READING_KEXEC_INITRAMFS] = KEXEC_INITRAMFS_CHECK,
> + [READING_POLICY] = POLICY_CHECK
> +};
> +
> /**
> * ima_read_file - pre-measure/appraise hook decision based on policy
> * @file: pointer to the file to be measured/appraised/audit
> @@ -442,18 +451,17 @@ int ima_read_file(struct file *file, enum
> kernel_read_file_id read_id)
> }
> return 0; /* We rely on module signature checking */
> }
> +
> + if (read_id == READING_FIRMWARE_PREALLOC_BUFFER) {
> + if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
> + (ima_appraise & IMA_APPRAISE_ENFORCE)) {
> + pr_err("Prevent device from accessing firmware prior to
> verifying the firmware signature.\n");
> +
Re: [PATCH v4 5/8] ima: based on policy require signed firmware (sysfs fallback)
On Tue, May 29, 2018 at 02:01:57PM -0400, Mimi Zohar wrote:
> Luis, is the security_kernel_post_read_file LSM hook in
> firmware_loading_store() still needed after this patch? Should it be
> calling security_kernel_load_data() instead?
That's up to Kees to decide as he added that hook, and knows
what LSMs may be doing with it. From my perspective it is confusing
to have that hook there so I think it could be removed now.
Kees?
Luis
>
> ---
>
> With an IMA policy requiring signed firmware, this patch prevents
> the sysfs fallback method of loading firmware.
>
> Signed-off-by: Mimi Zohar
> Cc: Luis R. Rodriguez
> Cc: David Howells
> Cc: Matthew Garrett
> ---
> security/integrity/ima/ima_main.c | 10 +-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/security/integrity/ima/ima_main.c
> b/security/integrity/ima/ima_main.c
> index a565d46084c2..4a87f78098c8 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -475,8 +475,10 @@ 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))
> + (ima_appraise & IMA_APPRAISE_ENFORCE)) {
> + pr_err("Prevent firmware loading_store.\n");
> return -EACCES; /* INTEGRITY_UNKNOWN */
> + }
> return 0;
> }
>
> @@ -520,6 +522,12 @@ int ima_load_data(enum kernel_load_data_id id)
> pr_err("impossible to appraise a kernel image without a
> file descriptor; try using kexec_file_load syscall.\n");
> return -EACCES; /* INTEGRITY_UNKNOWN */
> }
> + break;
> + case LOADING_FIRMWARE:
> + if (ima_appraise & IMA_APPRAISE_FIRMWARE) {
> + pr_err("Prevent firmware sysfs fallback loading.\n");
> + return -EACCES; /* INTEGRITY_UNKNOWN */
> + }
> default:
> break;
> }
> --
> 2.7.5
>
>
--
Do not panic
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v4 4/8] firmware: add call to LSM hook before firmware sysfs fallback
On Tue, May 29, 2018 at 02:01:56PM -0400, Mimi Zohar wrote:
> Add an LSM hook prior to allowing firmware sysfs fallback loading.
Acked-by: Luis R. Rodriguez
> Signed-off-by: Mimi Zohar
> Cc: Luis R. Rodriguez
> Cc: David Howells
> Cc: Kees Cook
>
> Changelog v4:
> - call new LSM security_kernel_arg hook
>
> Changelog v2:
> - call security_kernel_read_blob()
> - rename the READING_FIRMWARE_FALLBACK kernel_read_file_id enumeration to
> READING_FIRMWARE_FALLBACK_SYSFS.
> ---
> drivers/base/firmware_loader/fallback.c | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/base/firmware_loader/fallback.c
> b/drivers/base/firmware_loader/fallback.c
> index 358354148dec..2443bda81631 100644
> --- a/drivers/base/firmware_loader/fallback.c
> +++ b/drivers/base/firmware_loader/fallback.c
> @@ -651,6 +651,8 @@ static bool fw_force_sysfs_fallback(unsigned int
> opt_flags)
>
> static bool fw_run_sysfs_fallback(unsigned int opt_flags)
> {
> + int ret;
> +
> if (fw_fallback_config.ignore_sysfs_fallback) {
> pr_info_once("Ignoring firmware sysfs fallback due to sysctl
> knob\n");
> return false;
> @@ -659,6 +661,11 @@ static bool fw_run_sysfs_fallback(unsigned int opt_flags)
> if ((opt_flags & FW_OPT_NOFALLBACK))
> return false;
>
> + /* Also permit LSMs and IMA to fail firmware sysfs fallback */
> + ret = security_kernel_load_data(LOADING_FIRMWARE);
> + if (ret < 0)
> + return ret;
> +
> return fw_force_sysfs_fallback(opt_flags);
> }
>
> --
> 2.7.5
>
>
--
Do not panic
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v3.1] firmware: clean up filesystem load exit path
On Thu, Feb 04, 2016 at 01:15:02PM -0800, Kees Cook wrote: > This makes the error and success paths more readable while trying to > load firmware from the filesystem. > > Signed-off-by: Kees Cook > Cc: Josh Boyer > Cc: David Howells > Cc: Luis R. Rodriguez > Cc: Mimi Zohar Thanks, Acked-by: Luis R. Rodriguez > --- > Suggested as an alternative to "[PATCH v3 06/22] firmware: fold successful fw > read early" Mimi, feel free to pick up as a replacement in your series. Luis ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v3 06/22] firmware: fold successful fw read early
On Thu, Feb 04, 2016 at 09:36:50AM -0800, Kees Cook wrote:
> On Wed, Feb 3, 2016 at 11:06 AM, Mimi Zohar wrote:
> > From: David Howells
> >
> > We'll be folding in some more checks on fw_read_file_contents(),
> > this will make the success case easier to follow.
> >
> > Reviewed-by: Josh Boyer
> > Signed-off-by: David Howells
> > Signed-off-by: Luis R. Rodriguez
> > Signed-off-by: Mimi Zohar
> > ---
> > drivers/base/firmware_class.c | 16 +++-
> > 1 file changed, 7 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> > index fb64814..c658cec 100644
> > --- a/drivers/base/firmware_class.c
> > +++ b/drivers/base/firmware_class.c
> > @@ -361,20 +361,18 @@ static int fw_get_filesystem_firmware(struct device
> > *device,
> > continue;
> > rc = fw_read_file_contents(file, buf);
> > fput(file);
> > - if (rc)
> > + if (rc == 0) {
> > + dev_dbg(device, "direct-loading %s\n",
> > + buf->fw_id);
> > + fw_finish_direct_load(device, buf);
> > + goto out;
> > + } else
> > dev_warn(device, "loading %s failed with error
> > %d\n",
> > path, rc);
> > - else
> > - break;
> > }
> > +out:
> > __putname(path);
> >
> > - if (!rc) {
> > - dev_dbg(device, "direct-loading %s\n",
> > - buf->fw_id);
> > - fw_finish_direct_load(device, buf);
> > - }
> > -
> > return rc;
> > }
>
> Looking at this code, why does this use "break":
>
> len = snprintf(path, PATH_MAX, "%s/%s",
>fw_path[i], buf->fw_id);
> if (len >= PATH_MAX) {
> rc = -ENAMETOOLONG;
> break;
> }
>
> Shouldn't that emit a warning, set rc, and continue?
Yes but this is a separate piece of code, and that's a functional change but I
welcome the change for sure. This could be done separately.
> Regardless, I think this is more readable. Adding an "out" target at
> the end of a for loop seems weird, given "break" existing. :)
Good call, goto mixed with while loops can be get iffy. I'm fine with this
change as a replacement. To be fair we should also note both patches (the one
submitted and yours below) also make a small functional change so that the loop
continues over all other possible directories in the fw_path in case of failure
with the first directory used as a base. If we wanted to be pedantic and careful
we could split the functional change to not enable to continue in case of
failure
onto the next directory, and instead require that as a separate patch. I'll
leave
it to Mimi to decide.
>
> rc = fw_read_file_contents(file, buf);
> fput(file);
> - if (rc)
> + if (rc) {
> dev_warn(device, "loading %s failed with error %d\n",
> path, rc);
> + continue;
> + }
> + dev_dbg(device, "direct-loading %s\n", buf->fw_id);
> + fw_finish_direct_load(device, buf);
> - else
> - break;
> + break;
> }
> __putname(path);
> -
> - if (!rc) {
> - dev_dbg(device, "direct-loading %s\n",
> - buf->fw_id);
> - fw_finish_direct_load(device, buf);
> - }
> -
> return rc;
> }
Can you provided a Signed-offy-by? If so consider my Acked-by.
Luis
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v3 14/22] security: define kernel_read_file hook
On Wed, Feb 03, 2016 at 02:06:22PM -0500, Mimi Zohar wrote: > The kernel_read_file security hook is called prior to reading the file > into memory. > > Signed-off-by: Mimi Zohar Acked-by: Luis R. Rodriguez Luis ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v3 15/22] vfs: define kernel_copy_file_from_fd()
On Wed, Feb 03, 2016 at 02:06:23PM -0500, Mimi Zohar wrote: > This patch defines kernel_read_file_from_fd(), a wrapper for the VFS > common kernel_read_file(). > > Changelog: > - Separated from the kernel modules patch > > Signed-off-by: Mimi Zohar Acked-by: Luis R. Rodriguez Luis ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v3 13/22] firmware: replace call to fw_read_file_contents() with kernel version
On Wed, Feb 03, 2016 at 02:06:21PM -0500, Mimi Zohar wrote: > Replace the fw_read_file_contents with kernel_file_read_from_path(). > > Although none of the upstreamed LSMs define a kernel_fw_from_file hook, > IMA is called by the security function to prevent unsigned firmware from > being loaded and to measure/appraise signed firmware, based on policy. > > Instead of reading the firmware twice, once for measuring/appraising the > firmware and again for reading the firmware contents into memory, the > kernel_post_read_file() security hook calculates the file hash based on > the in memory file buffer. The firmware is read once. > > This patch removes the LSM kernel_fw_from_file() hook and security call. > > Changelog v3: > - remove kernel_fw_from_file hook > - use kernel_file_read_from_path() - requested by Luis > v2: > - reordered and squashed firmware patches > - fix MAX firmware size (Kees Cook) > > Signed-off-by: Mimi Zohar Acked-by: Luis R. Rodriguez Luis ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v3 18/22] kexec: replace call to copy_file_from_fd() with kernel version
On Wed, Feb 03, 2016 at 02:06:26PM -0500, Mimi Zohar wrote: > Replace copy_file_from_fd() with kernel_read_file_from_fd(). > > Two new identifiers named READING_KEXEC_IMAGE and READING_KEXEC_INITRAMFS > are defined for measuring, appraising or auditing the kexec image and > initramfs. > > Changelog v3: > - return -EBADF, not -ENOEXEC > - identifier change > - moved copy_file_from_fd() to a separate patch > - defer support for IMA > v1: > - re-order and squash the kexec patches > v0: ima: measure and appraise kexec image and initramfs (squashed) > - rename ima_read_hooks enumeration to ima_policy_id > - use kstat file size type loff_t, not size_t > - add union name "hooks" to fix sparse warning > - Calculate the file hash from the in memory buffer > (suggested by Dave Young) > - Rename ima_read_and_process_file() to ima_hash_and_process_file() > - replace individual case statements with range: > KEXEC_CHECK ... IMA_MAX_READ_CHECK - 1 > - Instead of ima_read_and_process_file() allocating memory, the caller > allocates and frees the memory. > - Moved the kexec measurement/appraisal call to copy_file_from_fd(). The > same call now measures and appraises both the kexec image and initramfs. > > Signed-off-by: Mimi Zohar Acked-by: Luis R. Rodriguez Luis ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v3 16/22] module: replace copy_module_from_fd with kernel version
On Wed, Feb 03, 2016 at 02:06:24PM -0500, Mimi Zohar wrote: > Replace copy_module_from_fd() with kernel_read_file_from_fd(). > > Although none of the upstreamed LSMs define a kernel_module_from_file > hook, IMA is called, based on policy, to prevent unsigned kernel modules > from being loaded by the original kernel module syscall and to > measure/appraise signed kernel modules. > > The security function security_kernel_module_from_file() was called prior > to reading a kernel module. Preventing unsigned kernel modules from being > loaded by the original kernel module syscall remains on the pre-read > kernel_read_file() security hook. Instead of reading the kernel module > twice, once for measuring/appraising and again for loading the kernel > module, the signature validation is moved to the kernel_post_read_file() > security hook. > > This patch removes the security_kernel_module_from_file() hook and security > call. > > Signed-off-by: Mimi Zohar Acked-by: Luis R. Rodriguez Luis ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v3 12/22] vfs: define kernel_read_file_from_path
On Wed, Feb 03, 2016 at 02:06:20PM -0500, Mimi Zohar wrote: > This patch defines kernel_read_file_from_path(), a wrapper for the VFS > common kernel_read_file(). > > Changelog: > - Separated from the IMA patch > > Signed-off-by: Mimi Zohar Acked-by: Luis R. Rodriguez Luis ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v3 08/22] vfs: define kernel_read_file_id enumeration
On Wed, Feb 03, 2016 at 02:06:16PM -0500, Mimi Zohar wrote: > To differentiate between the kernel_read_file() callers, this patch > defines a new enumeration named kernel_read_file_id and includes the > caller identifier as an argument. > > Subsequent patches define READING_KEXEC_IMAGE, READING_KEXEC_INITRAMFS, > READING_FIRMWARE, READING_MODULE, and READING_POLICY. > > Changelog v3: > - Replace the IMA specific enumeration with a generic one. > > Signed-off-by: Mimi Zohar Acked-by: Luis R. Rodriguez Luis ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [RFC PATCH v2 06/11] kexec: replace call to copy_file_from_fd() with kernel version
On Mon, Jan 25, 2016 at 06:48:12PM -0500, Mimi Zohar wrote: > On Mon, 2016-01-25 at 21:34 +0100, Luis R. Rodriguez wrote: > > On Mon, Jan 25, 2016 at 10:04:18AM -0500, Mimi Zohar wrote: > > > On Mon, 2016-01-25 at 14:37 +0800, Dave Young wrote: > > > > Hi, Mimi > > > > > > > > Besides of code issues, I have several thing to be understand: > > > > > > > > What is the effect to kexec behavior with this patchset? > > > > - without IMA enabled (kconfig or kernel cmdline) it will be same as > > > > before? > > > > > > Yes, without IMA configured or an IMA policy, it is the same as before. > > > > That's a bit unfair to your work here, this actually paves the way > > for not only IMA but also other LSMs to vet for the kexec/initramfs > > given LSM hooks are used now on a common kernel read set of functions. > > Right, I responded to his questions about IMA and not the bigger picture. Indeed, its just the bigger picture helps validate your work further. > > > > - with IMA enabled for kernel bzImage, kexec_file_load will check > > > > both ima > > > > signature and original pe file signature, those two mechanisms are > > > > somehow duplicated. I'm not sure if we need both for bzImage. > > > > > > IMA provides a uniform method of measuring and appraising all files on > > > the system, based on policy. The IMA policy could prevent the original > > > kexec syscall. > > Sorry for jumping back and forth between security hooks. Similarly, for > the kernel module hook, it prevents the original kernel module syscall > as well. This is indeed an important feature. I know people who don't use IMA won't care, but people who do should, likewise its also important for the prospects in design of LSMs if they wanted to consider the prospects of this evaluation for any of the file types we are now clearly defining. > > On systems without MODULE_SIG_FORCE, the IMA policy > > > would require an IMA signature as well. (The current patch would > > > require both, even when MODULE_SIG_FORCE is enabled.) > > > > > > Right, so what this approach has revealed really is that architecturally > > MODULE_SIG_FORCE should have been an LSM but its not, its also hard to make > > it > > an LSM. Now with LSM stacking this might make more sense but that requires > > work and who knows when and if that will happen. > > I kind of lost you here. A new mini LSM would require file signatures > of an existing type or would it be a new method for verifying file > signatures? No the way I see it is this is a feature for signature verification with all security mechanisms built-in to the kernel. Clearly there is a difference for how signatures are defined between file types: modules have signatures built-in to the file, meanwhile for other files this might be detached (that will be the case for firmware, sysdata). This has yet to be considered for kexec / initramfs but it shouldn't' be too hard now to consider that. Its also why firmware signatures is using a separate kconfig option. > > In the meantime we'll live > > with the fact that enabling MODULE_SIG_FORCE means you want to stack on > > top of the LSMs you have enabled, the MODULE_SIG_FORCE functionality being > > all kernel related and perhaps easier to manage / set. > > As I see it, with MODULE_SIG_FORCE, IMA-appraisal could relax its own > policy knowing that only signed kernel modules are loaded. Without > MODULE_SIG_FORCE enabled, then IMA-appraisal needs to do the enforcing, > of course based on policy. Sure, up to the LSMs, which begs the question of how an LSM would codify such stacking assumptions. IIRC stacking will just let the code stack, but I am not sure if we have a clean way to know: hey the kernel vetted this file's signature itself, just fyi. Other than #idef'ery or relying on the .config. What I just described is the case for say IMA and kernel module firmware signing (note both have separate kconfigs), the same question still applies to ways LSM stacking can provide a set of security requirements each could have addressed. All in a clean way. Luis ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [RFC PATCH v2 06/11] kexec: replace call to copy_file_from_fd() with kernel version
On Mon, Jan 25, 2016 at 10:04:18AM -0500, Mimi Zohar wrote: > On Mon, 2016-01-25 at 14:37 +0800, Dave Young wrote: > > Hi, Mimi > > > > Besides of code issues, I have several thing to be understand: > > > > What is the effect to kexec behavior with this patchset? > > - without IMA enabled (kconfig or kernel cmdline) it will be same as > > before? > > Yes, without IMA configured or an IMA policy, it is the same as before. That's a bit unfair to your work here, this actually paves the way for not only IMA but also other LSMs to vet for the kexec/initramfs given LSM hooks are used now on a common kernel read set of functions. > > > - with IMA enabled for kernel bzImage, kexec_file_load will check both ima > > signature and original pe file signature, those two mechanisms are > > somehow duplicated. I'm not sure if we need both for bzImage. > > IMA provides a uniform method of measuring and appraising all files on > the system, based on policy. The IMA policy could prevent the original > kexec syscall. On systems without MODULE_SIG_FORCE, the IMA policy > would require an IMA signature as well. (The current patch would > require both, even when MODULE_SIG_FORCE is enabled.) Right, so what this approach has revealed really is that architecturally MODULE_SIG_FORCE should have been an LSM but its not, its also hard to make it an LSM. Now with LSM stacking this might make more sense but that requires work and who knows when and if that will happen. In the meantime we'll live with the fact that enabling MODULE_SIG_FORCE means you want to stack on top of the LSMs you have enabled, the MODULE_SIG_FORCE functionality being all kernel related and perhaps easier to manage / set. Luis ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [RFC PATCH v2 09/11] ima: load policy using path
On Mon, Jan 18, 2016 at 10:11:24AM -0500, Mimi Zohar wrote:
> From: Dmitry Kasatkin
>
> echo /etc/ima/ima_policy > /sys/kernel/security/ima/policy
> fs/exec.c | 21
> diff --git a/fs/exec.c b/fs/exec.c
> index 3524e5f..5731b40 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -903,6 +903,27 @@ out:
> return ret;
> }
>
> +int kernel_read_file_from_path(char *path, void **buf, loff_t *size,
> +loff_t max_size, int policy_id)
> +{
> + struct file *file;
> + int ret;
> +
> + if (!path || !*path)
> + return -EINVAL;
> +
> + file = filp_open(path, O_RDONLY, 0);
> + if (IS_ERR(file)) {
> + ret = PTR_ERR(file);
> + pr_err("Unable to open file: %s (%d)", path, ret);
> + return ret;
> + }
> +
> + ret = kernel_read_file(file, buf, size, max_size, policy_id);
> + fput(file);
> + return ret;
> +}
> +
I like this strategy, and can see it being further generalized in
areas of the kernel where we may want self policy / file loading
appraisal. Those policies are now defined through LSMs, in IMA
here you do your own vetting but -- it seems we might better
want instead to enable easily *any* LSM to do its vetting. In this
case I suppose that's still possible given kernel_read_file() is
used and that in turn would have a pre LSM hook and post-LSM hook.
Is that right? If so then it should just be a matter of all other
areas of the kernel to use kernel_read_file_from_path() or
kernel_read_file() to take advantage of similar strategies. Or
is there more work? The other obvious alternative for the heavy
handed users would be to just use the future sysdata helpers, that
in turn would use kernel_read_file_from_path() and still if a
distribution wants can also in the future get kernel signature
verification.
Luis
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [RFC PATCH v2 00/11] vfss: support for a common kernel file loader
On Mon, Jan 18, 2016 at 10:11:15AM -0500, Mimi Zohar wrote: > For a while it was looked down upon to directly read files from Linux. > These days there exists a few mechanisms in the kernel that do just this > though to load a file into a local buffer. There are minor but important > checks differences on each, we should take all the best practices from > each of them, generalize them and make all places in the kernel that > read a file use it.[1] > > One difference is the method for opening the file. In some cases we > have a file, while in other cases we have a pathname or a file descriptor. > > Another difference is the security hook calls, or lack of them. In > some versions there is a post file read hook, while in others there > is a pre file read hook. > > This patch set is the first attempt at resolving these differences. It > does not attempt to merge the different methods of opening a file, but > defines a single common kernel file read function with two wrappers. > Although this patch set defines two new security hooks for pre and post > file read, it does not attempt to merge the existing security hooks. > That is left as future work. > > Changelog v2: > - Combined the "ima: measuring/appraising files read by the kernel" patches > with this patch set to simplify review. > - Split the "ima: measure and appraise kexec image and initramfs" patch to > separate IMA from the kexec changes. > > The latest version of these patches can be found in the next-kernel-read-v2 > branch of: > git://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git > > [1] Taken from Luis Rodriguez's wiki - > http://kernelnewbies.org/KernelProjects/common-kernel-loader Did 0-day bot get a chance to test this tree? If not can it be added ? Luis ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [RFC PATCH v2 08/11] module: replace copy_module_from_fd with kernel version
On Thu, Jan 21, 2016 at 5:12 AM, Mimi Zohar wrote:
> On Thu, 2016-01-21 at 01:03 +0100, Luis R. Rodriguez wrote:
>> On Mon, Jan 18, 2016 at 10:11:23AM -0500, Mimi Zohar wrote:
>> > This patch replaces the module copy_module_from_fd() call with the VFS
>> > common kernel_read_file_from_fd() function. Instead of reading the
>> > kernel module twice, once for measuring/appraising and then loading
>> > the kernel module, the file is read once.
>> >
>> > This patch defines a new security hook named security_kernel_read_file(),
>> > which is called before reading the file. For now, call the module
>> > security hook from security_kernel_read_file until the LSMs have been
>> > converted to use the kernel_read_file hook.
>> >
>> > This patch retains the kernel_module_from_file hook, but removes the
>> > security_kernel_module_from_file() function.
>>
>> I think it would help if your cover letter and this patch described
>> a bit that some LSMs either prefer to read / check / appraise files
>> prior to loading and some other prefer to do that later. You could
>> explain the LSM hook preferences and what they do. Then here you
>> can explain how this one prefers a hook early, but acknowledge that
>> the other one still exists.
>
> Before this patch set, IMA measured/appraised/audited a file before
> allowing it to be accessed, causing the file in some cases to be read
> twice. This patch set changes that. Files are read into memory and
> then measured/appraised/audited.
Sounds like this could help also with performance, has any preliminary
benchmarking been done to see the effect ?
> It's been a while since this hook was added. As I recall, Kees added
> the pre module hook to limit loading kernel modules to only those
> filesystems that were mounted read-only. I would have to look at each
> of the LSMs to see how they're using the hooks.
Sure.
>> So:
>>
>> kernel_read_file() {
>> ...
>> security_kernel_read_file();
>> ...
>> security_kernel_post_read_file();
>> ...
>> }
>>
>> > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>> > index 4e6e2af..9915310 100644
>> > --- a/include/linux/lsm_hooks.h
>> > +++ b/include/linux/lsm_hooks.h
>> > @@ -1465,6 +1471,7 @@ union security_list_options {
>> > int (*kernel_fw_from_file)(struct file *file, char *buf, size_t size);
>> > int (*kernel_module_request)(char *kmod_name);
>> > int (*kernel_module_from_file)(struct file *file);
>> > + int (*kernel_read_file)(struct file *file, int policy_id);
>> > int (*kernel_post_read_file)(struct file *file, char *buf, loff_t size,
>> > int policy_id);
>> > int (*task_fix_setuid)(struct cred *new, const struct cred *old,
>>
>> Is the goal to eventually kill the other LSM hooks and just keep the
>> file one? If so where is that done in this series? It was not clear.
>
> As mentioned in the cover letter, consolidating the LSM hooks is not
> covered in this patch set.
Sorry I missed that after I started reviewing.
> I was under the impression that not only
> were we defining a common kernel read file function, but that we were
> also consolidating the pre and post security hooks as well.
Sure.
> By defining
> the pre and post security hooks in this patch set, it permits each of
> the LSMs to migrate to the new hooks independently of each other. Lets
> ask the LSM maintainers what they think.
I see -- yeah making this a 2 step thing makes sense, so long as the
maintainers can later expect / understand what would be done in a
second patch set. Breaking this down in two patch sets makes sense. It
should also mean there might be fun benchmarks on gains provided there
were considerable IO savings by not opening files twice.
Luis
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [RFC PATCH v2 07/11] firmware: replace call to fw_read_file_contents() with kernel version
On Thu, Jan 21, 2016 at 4:05 AM, Mimi Zohar wrote:
> On Wed, 2016-01-20 at 15:56 -0800, Luis R. Rodriguez wrote:
>> On Wed, Jan 20, 2016 at 3:39 PM, Luis R. Rodriguez wrote:
>
>> >> @@ -350,13 +321,18 @@ static int fw_get_filesystem_firmware(struct device
>> >> *device,
>> >> file = filp_open(path, O_RDONLY, 0);
>> >> if (IS_ERR(file))
>> >> continue;
>> >> - rc = fw_read_file_contents(file, buf);
>> >> +
>> >> + buf->size = 0;
>> >> + rc = kernel_read_file(file, &buf->data, &size, INT_MAX,
>> >> + FIRMWARE_CHECK);
>> >
>> > The way kernel firmware signing was implemented was that we'd first read
>> > the
>> > foo.sig (or whatever extension we use).
>
> Was there a reason for using a detached signature and not using the same
> method as kernel modules?
Yes, the firmware might have different license. Its also easier for
users to use standard mechanisms to verify.
>> The same kernel_read_file() would be
>> > used if this gets applied so this would still works well with that. Of
>> > course
>> > folks using IMA and their own security policy would just disable the kernel
>> > fw signing facility.
>
> Right, support for not measuring/appraising the firmware and sig would
> be supported in the policy.
OK!
>> >> diff --git a/include/linux/ima.h b/include/linux/ima.h
>> >> index ae91938..0a7f039 100644
>> >> --- a/include/linux/ima.h
>> >> +++ b/include/linux/ima.h
>> >> @@ -16,6 +16,7 @@ struct linux_binprm;
>> >> enum ima_policy_id {
>> >> KEXEC_CHECK = 1,
>> >> INITRAMFS_CHECK,
>> >> + FIRMWARE_CHECK,
>> >> IMA_MAX_READ_CHECK
>> >> };
>> >
>> > The only thing that is worth questioning here in light of kernel fw
>> > signing is
>> > what int policy_id do we use? Would we be OK to add FIRMWARE_SIG_CHECK
>> > later
>> > While at it, perhaps kernel_read_file() last argument should be enum
>> > ima_policy_id then. If the policy_id is going to be used elsewhere outside
>> > of
>> > IMA then perhaps ima.h is not the best place for it ? Would fs.h for type
>> > of
>> > file be OK ? Then we'd have a list of known file types the kernel reads.
>
> I would definitely prefer the enumeration be defined at the VFS layer.
> For example,
>
> enum kernel_read_file_id {
> READING_KEXEC_IMAGE,
> READING_KEXEC_INITRAMFS,
> READING_FIRMWARE,
> READING_FIRMWARE_SIG,
> READING_MAX_ID
> };
Looks good to me. Please add a kdoc section to force us to have to
document each one.
> Agreed, the last field of kernel_read_file() and the wrappers should be
> the enumeration.
Great.
>> Actually your patch #9 "ima: load policy using path" defines
>> kernel_read_file_from_path and since the firmware code uses a path
>> this code would be much cleaner if instead you used that. It'd mean
>> more code sharing and would make firmware code cleaner. Could you
>> re-order that to go first and then later this as its first user?
>> Perhaps add the helper for the firmware patch.
>
> Thanks, I missed that. I'll include this change in the next version.
Great.
Luis
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [RFC PATCH v2 09/11] ima: load policy using path
On Mon, Jan 18, 2016 at 10:11:24AM -0500, Mimi Zohar wrote:
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -903,6 +903,27 @@ out:
> return ret;
> }
>
> +int kernel_read_file_from_path(char *path, void **buf, loff_t *size,
> +loff_t max_size, int policy_id)
> +{
> + struct file *file;
> + int ret;
> +
> + if (!path || !*path)
> + return -EINVAL;
> +
> + file = filp_open(path, O_RDONLY, 0);
> + if (IS_ERR(file)) {
> + ret = PTR_ERR(file);
> + pr_err("Unable to open file: %s (%d)", path, ret);
> + return ret;
> + }
> +
> + ret = kernel_read_file(file, buf, size, max_size, policy_id);
> + fput(file);
> + return ret;
> +}
> +
EXPORT_SYMBOL_GPL() needed.
Luis
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [RFC PATCH v2 08/11] module: replace copy_module_from_fd with kernel version
On Mon, Jan 18, 2016 at 10:11:23AM -0500, Mimi Zohar wrote:
> This patch replaces the module copy_module_from_fd() call with the VFS
> common kernel_read_file_from_fd() function. Instead of reading the
> kernel module twice, once for measuring/appraising and then loading
> the kernel module, the file is read once.
>
> This patch defines a new security hook named security_kernel_read_file(),
> which is called before reading the file. For now, call the module
> security hook from security_kernel_read_file until the LSMs have been
> converted to use the kernel_read_file hook.
>
> This patch retains the kernel_module_from_file hook, but removes the
> security_kernel_module_from_file() function.
I think it would help if your cover letter and this patch described
a bit that some LSMs either prefer to read / check / appraise files
prior to loading and some other prefer to do that later. You could
explain the LSM hook preferences and what they do. Then here you
can explain how this one prefers a hook early, but acknowledge that
the other one still exists.
So:
kernel_read_file() {
...
security_kernel_read_file();
...
security_kernel_post_read_file();
...
}
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 4e6e2af..9915310 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1465,6 +1471,7 @@ union security_list_options {
> int (*kernel_fw_from_file)(struct file *file, char *buf, size_t size);
> int (*kernel_module_request)(char *kmod_name);
> int (*kernel_module_from_file)(struct file *file);
> + int (*kernel_read_file)(struct file *file, int policy_id);
> int (*kernel_post_read_file)(struct file *file, char *buf, loff_t size,
>int policy_id);
> int (*task_fix_setuid)(struct cred *new, const struct cred *old,
Is the goal to eventually kill the other LSM hooks and just keep the
file one? If so where is that done in this series? It was not clear.
Luis
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [RFC PATCH v2 07/11] firmware: replace call to fw_read_file_contents() with kernel version
On Wed, Jan 20, 2016 at 3:39 PM, Luis R. Rodriguez wrote:
> On Mon, Jan 18, 2016 at 10:11:22AM -0500, Mimi Zohar wrote:
>> Replace fw_read_file_contents() for reading a file with the common VFS
>> kernel_read_file() function. A benefit of calling kernel_read_file()
>> to read the firmware is the firmware is read only once, instead of once
>> for measuring/appraising the firmware and again for reading the file
>> contents into memory.
>>
>> This patch retains the kernel_fw_from_file() hook, which is called from
>> security_kernel_post_read_file(), but removes the
>> sercurity_kernel_fw_from_file() function.
>>
>> Changelog:
>> - reordered and squashed firmware patches
>> - fix MAX firmware size (Kees Cook)
>>
>> Signed-off-by: Mimi Zohar
>> ---
>> drivers/base/firmware_class.c | 48
>> ++-
>> include/linux/ima.h | 7 +
>> include/linux/security.h | 8 +-
>> security/integrity/ima/ima.h | 1 -
>> security/integrity/ima/ima_appraise.c | 8 --
>> security/integrity/ima/ima_main.c | 18 +
>> security/integrity/ima/ima_policy.c | 26 +--
>> security/integrity/integrity.h| 11 +++-
>> security/security.c | 28 ++--
>> 9 files changed, 54 insertions(+), 101 deletions(-)
>>
>> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
>> index 8524450..cc175f1 100644
>> --- a/drivers/base/firmware_class.c
>> +++ b/drivers/base/firmware_class.c
>> @@ -29,6 +29,7 @@
>> #include
>> #include
>> #include
>> +#include
>>
>> #include
>>
>> @@ -291,40 +292,10 @@ static const char * const fw_path[] = {
>> module_param_string(path, fw_path_para, sizeof(fw_path_para), 0644);
>> MODULE_PARM_DESC(path, "customized firmware image search path with a higher
>> priority than default path");
>>
>> -static int fw_read_file_contents(struct file *file, struct firmware_buf
>> *fw_buf)
>> -{
>> - int size;
>> - char *buf;
>> - int rc;
>> -
>> - if (!S_ISREG(file_inode(file)->i_mode))
>> - return -EINVAL;
>> - size = i_size_read(file_inode(file));
>> - if (size <= 0)
>> - return -EINVAL;
>> - buf = vmalloc(size);
>> - if (!buf)
>> - return -ENOMEM;
>> - rc = kernel_read(file, 0, buf, size);
>> - if (rc != size) {
>> - if (rc > 0)
>> - rc = -EIO;
>> - goto fail;
>> - }
>> - rc = security_kernel_fw_from_file(file, buf, size);
>> - if (rc)
>> - goto fail;
>> - fw_buf->data = buf;
>> - fw_buf->size = size;
>> - return 0;
>> -fail:
>> - vfree(buf);
>> - return rc;
>> -}
>> -
>> static int fw_get_filesystem_firmware(struct device *device,
>> struct firmware_buf *buf)
>> {
>> + loff_t size;
>> int i, len;
>> int rc = -ENOENT;
>> char *path;
>> @@ -350,13 +321,18 @@ static int fw_get_filesystem_firmware(struct device
>> *device,
>> file = filp_open(path, O_RDONLY, 0);
>> if (IS_ERR(file))
>> continue;
>> - rc = fw_read_file_contents(file, buf);
>> +
>> + buf->size = 0;
>> + rc = kernel_read_file(file, &buf->data, &size, INT_MAX,
>> + FIRMWARE_CHECK);
>
> The way kernel firmware signing was implemented was that we'd first read the
> foo.sig (or whatever extension we use). The same kernel_read_file() would be
> used if this gets applied so this would still works well with that. Of course
> folks using IMA and their own security policy would just disable the kernel
> fw signing facility.
>
>> diff --git a/include/linux/ima.h b/include/linux/ima.h
>> index ae91938..0a7f039 100644
>> --- a/include/linux/ima.h
>> +++ b/include/linux/ima.h
>> @@ -16,6 +16,7 @@ struct linux_binprm;
>> enum ima_policy_id {
>> KEXEC_CHECK = 1,
>> INITRAMFS_CHECK,
>> + FIRMWARE_CHECK,
>> IMA_MAX_READ_CHECK
>> };
>
> The only thing that is worth questioning here in light of kernel fw signing is
> what int policy_id do we use? Would we be OK to add FIRMWARE_SIG_CHECK later
> While at it, perhaps kernel_r
Re: [RFC PATCH v2 07/11] firmware: replace call to fw_read_file_contents() with kernel version
On Mon, Jan 18, 2016 at 10:11:22AM -0500, Mimi Zohar wrote:
> Replace fw_read_file_contents() for reading a file with the common VFS
> kernel_read_file() function. A benefit of calling kernel_read_file()
> to read the firmware is the firmware is read only once, instead of once
> for measuring/appraising the firmware and again for reading the file
> contents into memory.
>
> This patch retains the kernel_fw_from_file() hook, which is called from
> security_kernel_post_read_file(), but removes the
> sercurity_kernel_fw_from_file() function.
>
> Changelog:
> - reordered and squashed firmware patches
> - fix MAX firmware size (Kees Cook)
>
> Signed-off-by: Mimi Zohar
> ---
> drivers/base/firmware_class.c | 48
> ++-
> include/linux/ima.h | 7 +
> include/linux/security.h | 8 +-
> security/integrity/ima/ima.h | 1 -
> security/integrity/ima/ima_appraise.c | 8 --
> security/integrity/ima/ima_main.c | 18 +
> security/integrity/ima/ima_policy.c | 26 +--
> security/integrity/integrity.h| 11 +++-
> security/security.c | 28 ++--
> 9 files changed, 54 insertions(+), 101 deletions(-)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 8524450..cc175f1 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -29,6 +29,7 @@
> #include
> #include
> #include
> +#include
>
> #include
>
> @@ -291,40 +292,10 @@ static const char * const fw_path[] = {
> module_param_string(path, fw_path_para, sizeof(fw_path_para), 0644);
> MODULE_PARM_DESC(path, "customized firmware image search path with a higher
> priority than default path");
>
> -static int fw_read_file_contents(struct file *file, struct firmware_buf
> *fw_buf)
> -{
> - int size;
> - char *buf;
> - int rc;
> -
> - if (!S_ISREG(file_inode(file)->i_mode))
> - return -EINVAL;
> - size = i_size_read(file_inode(file));
> - if (size <= 0)
> - return -EINVAL;
> - buf = vmalloc(size);
> - if (!buf)
> - return -ENOMEM;
> - rc = kernel_read(file, 0, buf, size);
> - if (rc != size) {
> - if (rc > 0)
> - rc = -EIO;
> - goto fail;
> - }
> - rc = security_kernel_fw_from_file(file, buf, size);
> - if (rc)
> - goto fail;
> - fw_buf->data = buf;
> - fw_buf->size = size;
> - return 0;
> -fail:
> - vfree(buf);
> - return rc;
> -}
> -
> static int fw_get_filesystem_firmware(struct device *device,
> struct firmware_buf *buf)
> {
> + loff_t size;
> int i, len;
> int rc = -ENOENT;
> char *path;
> @@ -350,13 +321,18 @@ static int fw_get_filesystem_firmware(struct device
> *device,
> file = filp_open(path, O_RDONLY, 0);
> if (IS_ERR(file))
> continue;
> - rc = fw_read_file_contents(file, buf);
> +
> + buf->size = 0;
> + rc = kernel_read_file(file, &buf->data, &size, INT_MAX,
> + FIRMWARE_CHECK);
The way kernel firmware signing was implemented was that we'd first read the
foo.sig (or whatever extension we use). The same kernel_read_file() would be
used if this gets applied so this would still works well with that. Of course
folks using IMA and their own security policy would just disable the kernel
fw signing facility.
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index ae91938..0a7f039 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -16,6 +16,7 @@ struct linux_binprm;
> enum ima_policy_id {
> KEXEC_CHECK = 1,
> INITRAMFS_CHECK,
> + FIRMWARE_CHECK,
> IMA_MAX_READ_CHECK
> };
The only thing that is worth questioning here in light of kernel fw signing is
what int policy_id do we use? Would we be OK to add FIRMWARE_SIG_CHECK later
While at it, perhaps kernel_read_file() last argument should be enum
ima_policy_id then. If the policy_id is going to be used elsewhere outside of
IMA then perhaps ima.h is not the best place for it ? Would fs.h for type of
file be OK ? Then we'd have a list of known file types the kernel reads.
Luis
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [RFC PATCH v2 06/11] kexec: replace call to copy_file_from_fd() with kernel version
On Mon, Jan 18, 2016 at 10:11:21AM -0500, Mimi Zohar wrote:
> diff --git a/fs/exec.c b/fs/exec.c
> index 211b81c..a5ae51e 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -884,6 +884,21 @@ out:
> }
> EXPORT_SYMBOL_GPL(kernel_read_file);
>
> +int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t
> max_size,
> + int policy_id)
> +{
> + struct fd f = fdget(fd);
> + int ret = -ENOEXEC;
> +
> + if (!f.file)
> + goto out;
> +
> + ret = kernel_read_file(f.file, buf, size, max_size, policy_id);
> +out:
> + fdput(f);
> + return ret;
> +}
> +
Don't you need to EXPORT_SYMBOL_GPL() here as well?
> diff --git a/security/integrity/ima/ima_appraise.c
> b/security/integrity/ima/ima_appraise.c
> index 4edf47f..3adf937 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -78,6 +78,8 @@ enum integrity_status ima_get_cache_status(struct
> integrity_iint_cache *iint,
> return iint->ima_module_status;
> case FIRMWARE_CHECK:
> return iint->ima_firmware_status;
> + case KEXEC_CHECK ... IMA_MAX_READ_CHECK - 1:
> + return iint->ima_read_status;
I didn't get the memo that we're OK to use compiler specific extensions
like this. I'm sure if you are using it its not the first case, just
want to be sure we are aware of possible issues if some compiler doesn't
support this.
If we don't have a precedence can we just avoid its use?
Cc'd Julia in case this might be of interest for Coccinelle to grok.
> case FILE_CHECK:
> default:
> return iint->ima_file_status;
> @@ -100,6 +102,9 @@ static void ima_set_cache_status(struct
> integrity_iint_cache *iint,
> case FIRMWARE_CHECK:
> iint->ima_firmware_status = status;
> break;
> + case KEXEC_CHECK ... IMA_MAX_READ_CHECK - 1:
> + iint->ima_read_status = status;
> + break;
> case FILE_CHECK:
> default:
> iint->ima_file_status = status;
Likewise.
> @@ -122,6 +127,8 @@ static void ima_cache_flags(struct integrity_iint_cache
> *iint, int func)
> case FIRMWARE_CHECK:
> iint->flags |= (IMA_FIRMWARE_APPRAISED | IMA_APPRAISED);
> break;
> + case KEXEC_CHECK ... IMA_MAX_READ_CHECK - 1:
> + break;
> case FILE_CHECK:
> default:
> iint->flags |= (IMA_FILE_APPRAISED | IMA_APPRAISED);
Likewise.
> diff --git a/security/integrity/ima/ima_policy.c
> b/security/integrity/ima/ima_policy.c
> index 595e038..4711083 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -306,6 +306,8 @@ static int get_subaction(struct ima_rule_entry *rule, int
> func)
> return IMA_MODULE_APPRAISE;
> case FIRMWARE_CHECK:
> return IMA_FIRMWARE_APPRAISE;
> + case KEXEC_CHECK ... IMA_MAX_READ_CHECK - 1:
> + return IMA_READ_APPRAISE;
> case FILE_CHECK:
> default:
> return IMA_FILE_APPRAISE;
Likewise.
> @@ -948,10 +956,19 @@ int ima_policy_show(struct seq_file *m, void *v)
> seq_printf(m, pt(Opt_func), ft(func_post));
> break;
> default:
> - snprintf(tbuf, sizeof(tbuf), "%d",
> - entry->hooks.func);
> - seq_printf(m, pt(Opt_func), tbuf);
> - break;
> + switch (entry->hooks.policy_id) {
> + case KEXEC_CHECK:
> + seq_printf(m, pt(Opt_func), ft(func_kexec));
> + break;
> + case INITRAMFS_CHECK:
> + seq_printf(m, pt(Opt_func), ft(func_initramfs));
> + break;
> + default:
> + snprintf(tbuf, sizeof(tbuf), "%d",
> + entry->hooks.func);
> + seq_printf(m, pt(Opt_func), tbuf);
> + break;
> + }
> }
> seq_puts(m, " ");
> }
Two switches wrapped tend to lead to messy and hard to read code,
is using a function here better?
Luis
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [RFC PATCH v2 02/11] vfs: define a generic function to read a file from the kernel
On Mon, Jan 18, 2016 at 10:11:17AM -0500, Mimi Zohar wrote:
> diff --git a/fs/exec.c b/fs/exec.c
> index b06623a..6d623c2 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -831,6 +832,58 @@ int kernel_read(struct file *file, loff_t offset,
>
> EXPORT_SYMBOL(kernel_read);
>
> +int kernel_read_file(struct file *file, void **buf, loff_t *size,
> + loff_t max_size)
> +{
> + loff_t i_size, pos;
> + ssize_t bytes = 0;
> + int ret;
> +
> + if (!S_ISREG(file_inode(file)->i_mode))
> + return -EINVAL;
> +
> + i_size = i_size_read(file_inode(file));
> + if (max_size > 0 && i_size > max_size)
> + return -EFBIG;
loff_t is a __kernel_loff_t, which in turn is a long long, and that's
signed. We don't catch a negative value here, for max_size, we could
return -EINVAL if its < 0.
> + if (i_size == 0)
> + return -EINVAL;
Likewise for i_size. The setter of the size will depend on how the
code calling this routine setup the struct file passed.
So how about adding a i_size <= 0 check here as well here?
At least fw_read_file_contents() has historically done this,
so if this generic read is going to skip that I'd like to
see why. We're unifying so I rather be more pedantic.
Provided this is addressed feel free to peg:
Reviewed-by: Luis R. Rodriguez
Luis
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Next kexec release
Hey folks, I see recent discussions about desire for a new recent release of kexec, one of such reasons is kexec support on EFI systems. SUSE is also interested in this, and it'd be great if we can all synch up on supporting the same recent release. Thanks! Luis ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
