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 <zohar@xxxxxxxxxxxxxxxxxx>
> > Cc: Luis R. Rodriguez <mcgrof@xxxxxxxx>
> > Cc: David Howells <dhowells@xxxxxxxxxx>
> > Cc: Kees Cook <keescook@xxxxxxxxxxxx>
> > Cc: Serge E. Hallyn <serge@xxxxxxxxxx>
> > Cc: Stephen Boyd <sboyd@xxxxxxxxxx>
> > ---
> > 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");
> > + return -EACCES;
> > + }
> > + }
> > return 0;
> > }
> >
> > -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_post_read_file - in memory collect/appraise/audit measurement
> > * @file: pointer to the file to be measured/appraised/audit
> > --
> > 2.7.5
> >
> >
>
> --
> Do not panic
>
--
Do not panic
