FIRMWARE_PREALLOC_BUFFER is a "how", not a "what", and confuses the LSMs
that are interested in filtering between types of things. The "how"
should be an internal detail made uninteresting to the LSMs.
Fixes: a098ecd2fa7d ("firmware: support loading into a pre-allocated buffer")
Fixes: fd90bc559bfb ("ima: based on policy verify firmware signatures (pre-allocated buffer)")
Fixes: 4f0496d8ffa3 ("ima: based on policy warn about loading firmware (pre-allocated buffer)")
Signed-off-by: Kees Cook <[email protected]>
---
drivers/base/firmware_loader/main.c | 5 ++---
fs/exec.c | 7 ++++---
include/linux/fs.h | 2 +-
security/integrity/ima/ima_main.c | 6 ++----
4 files changed, 9 insertions(+), 11 deletions(-)
diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index ca871b13524e..c2f57cedcd6f 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -465,14 +465,12 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
int i, len;
int rc = -ENOENT;
char *path;
- enum kernel_read_file_id id = READING_FIRMWARE;
size_t msize = INT_MAX;
void *buffer = NULL;
/* Already populated data member means we're loading into a buffer */
if (!decompress && fw_priv->data) {
buffer = fw_priv->data;
- id = READING_FIRMWARE_PREALLOC_BUFFER;
msize = fw_priv->allocated_size;
}
@@ -496,7 +494,8 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
/* load firmware files from the mount namespace of init */
rc = kernel_read_file_from_path_initns(path, &buffer,
- &size, msize, id);
+ &size, msize,
+ READING_FIRMWARE);
if (rc) {
if (rc != -ENOENT)
dev_warn(device, "loading %s failed with error %d\n",
diff --git a/fs/exec.c b/fs/exec.c
index e6e8a9a70327..2bf549757ce7 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -927,6 +927,7 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
{
loff_t i_size, pos;
ssize_t bytes = 0;
+ void *allocated = NULL;
int ret;
if (!S_ISREG(file_inode(file)->i_mode) || max_size < 0)
@@ -950,8 +951,8 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
goto out;
}
- if (id != READING_FIRMWARE_PREALLOC_BUFFER)
- *buf = vmalloc(i_size);
+ if (!*buf)
+ *buf = allocated = vmalloc(i_size);
if (!*buf) {
ret = -ENOMEM;
goto out;
@@ -980,7 +981,7 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
out_free:
if (ret < 0) {
- if (id != READING_FIRMWARE_PREALLOC_BUFFER) {
+ if (allocated) {
vfree(*buf);
*buf = NULL;
}
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 3f881a892ea7..95fc775ed937 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2993,10 +2993,10 @@ static inline void i_readcount_inc(struct inode *inode)
#endif
extern int do_pipe_flags(int *, int);
+/* This is a list of *what* is being read, not *how*. */
#define __kernel_read_file_id(id) \
id(UNKNOWN, unknown) \
id(FIRMWARE, firmware) \
- id(FIRMWARE_PREALLOC_BUFFER, firmware) \
id(FIRMWARE_EFI_EMBEDDED, firmware) \
id(MODULE, kernel-module) \
id(KEXEC_IMAGE, kexec-image) \
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index c1583d98c5e5..f80ee4ce4669 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -611,19 +611,17 @@ void ima_post_path_mknod(struct dentry *dentry)
int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
{
/*
- * READING_FIRMWARE_PREALLOC_BUFFER
- *
* Do devices using pre-allocated memory run the risk of the
* firmware being accessible to the device prior to the completion
* of IMA's signature verification any more than when using two
- * buffers?
+ * buffers? It may be desirable to include the buffer address
+ * in this API and walk all the dma_map_single() mappings to check.
*/
return 0;
}
const 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,
--
2.25.1
On 2020-07-07 1:19 a.m., Kees Cook wrote:
> FIRMWARE_PREALLOC_BUFFER is a "how", not a "what", and confuses the LSMs
> that are interested in filtering between types of things. The "how"
> should be an internal detail made uninteresting to the LSMs.
>
> Fixes: a098ecd2fa7d ("firmware: support loading into a pre-allocated buffer")
> Fixes: fd90bc559bfb ("ima: based on policy verify firmware signatures (pre-allocated buffer)")
> Fixes: 4f0496d8ffa3 ("ima: based on policy warn about loading firmware (pre-allocated buffer)")
> Signed-off-by: Kees Cook <[email protected]>
> ---
> drivers/base/firmware_loader/main.c | 5 ++---
> fs/exec.c | 7 ++++---
> include/linux/fs.h | 2 +-
> security/integrity/ima/ima_main.c | 6 ++----
> 4 files changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
> index ca871b13524e..c2f57cedcd6f 100644
> --- a/drivers/base/firmware_loader/main.c
> +++ b/drivers/base/firmware_loader/main.c
> @@ -465,14 +465,12 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
> int i, len;
> int rc = -ENOENT;
> char *path;
> - enum kernel_read_file_id id = READING_FIRMWARE;
> size_t msize = INT_MAX;
> void *buffer = NULL;
>
> /* Already populated data member means we're loading into a buffer */
> if (!decompress && fw_priv->data) {
> buffer = fw_priv->data;
> - id = READING_FIRMWARE_PREALLOC_BUFFER;
> msize = fw_priv->allocated_size;
> }
>
> @@ -496,7 +494,8 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
>
> /* load firmware files from the mount namespace of init */
> rc = kernel_read_file_from_path_initns(path, &buffer,
> - &size, msize, id);
> + &size, msize,
> + READING_FIRMWARE);
> if (rc) {
> if (rc != -ENOENT)
> dev_warn(device, "loading %s failed with error %d\n",
> diff --git a/fs/exec.c b/fs/exec.c
> index e6e8a9a70327..2bf549757ce7 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -927,6 +927,7 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
> {
> loff_t i_size, pos;
> ssize_t bytes = 0;
> + void *allocated = NULL;
> int ret;
>
> if (!S_ISREG(file_inode(file)->i_mode) || max_size < 0)
> @@ -950,8 +951,8 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
> goto out;
> }
>
> - if (id != READING_FIRMWARE_PREALLOC_BUFFER)
> - *buf = vmalloc(i_size);
> + if (!*buf)
> + *buf = allocated = vmalloc(i_size);
> if (!*buf) {
> ret = -ENOMEM;
> goto out;
> @@ -980,7 +981,7 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
>
> out_free:
> if (ret < 0) {
> - if (id != READING_FIRMWARE_PREALLOC_BUFFER) {
> + if (allocated) {
> vfree(*buf);
> *buf = NULL;
> }
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 3f881a892ea7..95fc775ed937 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2993,10 +2993,10 @@ static inline void i_readcount_inc(struct inode *inode)
> #endif
> extern int do_pipe_flags(int *, int);
>
> +/* This is a list of *what* is being read, not *how*. */
> #define __kernel_read_file_id(id) \
> id(UNKNOWN, unknown) \
> id(FIRMWARE, firmware) \
With this change, I'm trying to figure out how the partial firmware read
is going to work on top of this reachitecture.
Is it going to be ok to add READING_PARTIAL_FIRMWARE here as that is a
"what"?
> - id(FIRMWARE_PREALLOC_BUFFER, firmware) \
My patch series gets rejected any time I make a change to the
kernel_read_file* region in linux/fs.h.
The requirement is for this api to move to another header file outside
of linux/fs.h
It seems the same should apply to your change.
Could you please add the following patch to the start of you patch
series to move the kernel_read_file* to its own include file?
https://patchwork.kernel.org/patch/11647063/
> id(FIRMWARE_EFI_EMBEDDED, firmware) \
> id(MODULE, kernel-module) \
> id(KEXEC_IMAGE, kexec-image) \
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index c1583d98c5e5..f80ee4ce4669 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -611,19 +611,17 @@ void ima_post_path_mknod(struct dentry *dentry)
> int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
> {
> /*
> - * READING_FIRMWARE_PREALLOC_BUFFER
> - *
> * Do devices using pre-allocated memory run the risk of the
> * firmware being accessible to the device prior to the completion
> * of IMA's signature verification any more than when using two
> - * buffers?
> + * buffers? It may be desirable to include the buffer address
> + * in this API and walk all the dma_map_single() mappings to check.
> */
> return 0;
> }
>
> const 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,
On Tue, Jul 07, 2020 at 09:42:02AM -0700, Scott Branden wrote:
> On 2020-07-07 1:19 a.m., Kees Cook wrote:
> > FIRMWARE_PREALLOC_BUFFER is a "how", not a "what", and confuses the LSMs
> > that are interested in filtering between types of things. The "how"
> > should be an internal detail made uninteresting to the LSMs.
> >
> > Fixes: a098ecd2fa7d ("firmware: support loading into a pre-allocated buffer")
> > Fixes: fd90bc559bfb ("ima: based on policy verify firmware signatures (pre-allocated buffer)")
> > Fixes: 4f0496d8ffa3 ("ima: based on policy warn about loading firmware (pre-allocated buffer)")
> > [...]
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 3f881a892ea7..95fc775ed937 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -2993,10 +2993,10 @@ static inline void i_readcount_inc(struct inode *inode)
> > #endif
> > extern int do_pipe_flags(int *, int);
> > +/* This is a list of *what* is being read, not *how*. */
> > #define __kernel_read_file_id(id) \
> > id(UNKNOWN, unknown) \
> > id(FIRMWARE, firmware) \
> With this change, I'm trying to figure out how the partial firmware read is
> going to work on top of this reachitecture.
> Is it going to be ok to add READING_PARTIAL_FIRMWARE here as that is a
> "what"?
No, that's why I said you need to do the implementation within the API
and not expect each LSM to implement their own (as I mentioned both
times):
https://lore.kernel.org/lkml/202005221551.5CA1372@keescook/
https://lore.kernel.org/lkml/202007061950.F6B3D9E6A@keescook/
I will reply in the thread above.
> > - id(FIRMWARE_PREALLOC_BUFFER, firmware) \
> My patch series gets rejected any time I make a change to the
> kernel_read_file* region in linux/fs.h.
> The requirement is for this api to move to another header file outside of
> linux/fs.h
> It seems the same should apply to your change.
Well I'm hardly making the same level of changes, but yeah, sure, if
that helps move things along, I can include that here.
> Could you please add the following patch to the start of you patch series to
> move the kernel_read_file* to its own include file?
> https://patchwork.kernel.org/patch/11647063/
https://lore.kernel.org/lkml/[email protected]/
You've included it in include/linux/security.h and that should be pretty
comprehensive, it shouldn't be needed in so many .c files.
--
Kees Cook
Greeting,
FYI, we noticed the following commit (built with gcc-9):
commit: 676800b78feedea6751fad36ce1ab41947e5689b ("[PATCH 2/4] fs: Remove FIRMWARE_PREALLOC_BUFFER from kernel_read_file() enums")
url: https://github.com/0day-ci/linux/commits/Kees-Cook/Fix-misused-kernel_read_file-enums/20200707-162205
base: https://git.kernel.org/cgit/linux/kernel/git/gregkh/driver-core.git 0a2fae2aea4a21b59d4a920b9765aaa696270b16
in testcase: rcuperf
with following parameters:
runtime: 300s
perf_type: rcu
on test machine: qemu-system-i386 -enable-kvm -cpu SandyBridge -smp 2 -m 16G
caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
+-----------------------------------------------------------------------------------------------+------------+------------+
| | 618ef49cdf | 676800b78f |
+-----------------------------------------------------------------------------------------------+------------+------------+
| boot_successes | 26 | 8 |
| boot_failures | 0 | 24 |
| BUG:unable_to_handle_page_fault_for_address | 0 | 24 |
| WARNING:at_mm/vmalloc.c:#__vunmap | 0 | 21 |
| Oops:#[##] | 0 | 24 |
| Kernel_panic-not_syncing:Fatal_exception | 0 | 2 |
| EIP:__vunmap | 0 | 18 |
| Kernel_panic-not_syncing:stack-protector:Kernel_stack_is_corrupted_in:__ia32_sys_finit_module | 0 | 13 |
| WARNING:at_kernel/kthread.c:#kthread_probe_data/0x | 0 | 3 |
| EIP:kthread_probe_data | 0 | 3 |
| EIP:no_context | 0 | 10 |
| EIP:__run_timers | 0 | 10 |
| Kernel_panic-not_syncing:Fatal_exception_in_interrupt | 0 | 9 |
| BUG:kernel_NULL_pointer_dereference,address | 0 | 3 |
| EIP:_raw_spin_lock_irqsave | 0 | 3 |
| EIP:vma_interval_tree_insert_after | 0 | 1 |
| WARNING:at_kernel/rcu/tree.c:#rcu_sched_clock_irq | 0 | 1 |
| EIP:rcu_sched_clock_irq | 0 | 1 |
+-----------------------------------------------------------------------------------------------+------------+------------+
If you fix the issue, kindly add following tag
Reported-by: kernel test robot <[email protected]>
[ 12.579581] BUG: unable to handle page fault for address: 88060345
[ 12.588865] WARNING: CPU: 0 PID: 1 at mm/vmalloc.c:2277 __vunmap+0x1eb/0x200
[ 12.590307] #PF: supervisor read access in kernel mode
[ 12.591988] Modules linked in:
[ 12.593387] #PF: error_code(0x0000) - not-present page
[ 12.594216] CPU: 0 PID: 1 Comm: systemd Not tainted 5.8.0-rc3-00009-g676800b78feed #1
[ 12.595529] *pdpt = 0000000000000000 *pde = f000ff53f000ff53
[ 12.597396] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014
[ 12.598793] BUG: unable to handle page fault for address: 0504042e
[ 12.611746] EIP: __vunmap+0x1eb/0x200
[ 12.613257] #PF: supervisor read access in kernel mode
[ 12.614206] Code: cc 8d b6 00 00 00 00 31 c9 31 d2 b8 ff ff ff ff e8 9a d0 ff ff eb af 8d b4 26 00 00 00 00 90 50 68 54 e0 b9 dc e8 9a 34 e4 ff <0f> 0b 59 5b e9 7b ff ff ff 8d b4 26 00 00 00 00 8d 74 26 00 90 66
[ 12.615471] #PF: error_code(0x0000) - not-present page
[ 12.638948] EAX: 00000028 EBX: ddd21ee8 ECX: 00000001 EDX: 00000000
[ 12.640082] *pdpt = 0000000000000000 *pde = f000ff53f000ff53
[ 12.640093] BUG: unable to handle page fault for address: 0504042e
[ 12.640096] #PF: supervisor read access in kernel mode
[ 12.641575] ESI: 00000ee8 EDI: ddd21ee8 EBP: ddd21dfc ESP: ddd21ddc
[ 12.642836] #PF: error_code(0x0000) - not-present page
[ 12.644338] DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068 EFLAGS: 00010296
[ 12.645459] *pdpt = 0000000000000000 *pde = f000ff53f000ff53
[ 12.646912] CR0: 80050033 CR2: b7b34650 CR3: 36246000 CR4: 000406f0
[ 12.648162] BUG: unable to handle page fault for address: 0504042e
[ 12.648163] #PF: supervisor read access in kernel mode
[ 12.648164] #PF: error_code(0x0000) - not-present page
[ 12.648165] *pdpt = 0000000000000000 *pde = f000ff53f000ff53
[ 12.648168] BUG: unable to handle page fault for address: 0504042e
[ 12.648169] #PF: supervisor read access in kernel mode
[ 12.648170] #PF: error_code(0x0000) - not-present page
[ 12.648170] *pdpt = 0000000000000000 *pde = f000ff53f000ff53
[ 12.648173] BUG: unable to handle page fault for address: 0504042e
[ 12.648175] #PF: supervisor read access in kernel mode
[ 12.649736] Call Trace:
[ 12.651177] #PF: error_code(0x0000) - not-present page
[ 12.651179] *pdpt = 0000000000000000 *pde = f000ff53f000ff53
[ 12.652680] __vfree+0x22/0x60
[ 12.653978] BUG: unable to handle page fault for address: 0504042e
[ 12.655324] vfree+0x2a/0x60
[ 12.656444] #PF: supervisor read access in kernel mode
[ 12.656445] #PF: error_code(0x0000) - not-present page
[ 12.656446] *pdpt = 0000000000000000 *pde = f000ff53f000ff53
[ 12.656452] BUG: unable to handle page fault for address: 0504042e
[ 12.656452] #PF: supervisor read access in kernel mode
[ 12.656453] #PF: error_code(0x0000) - not-present page
[ 12.656454] *pdpt = 0000000000000000 *pde = f000ff53f000ff53
[ 12.656456] BUG: unable to handle page fault for address: 0504042e
[ 12.656457] #PF: supervisor read access in kernel mode
[ 12.656457] #PF: error_code(0x0000) - not-present page
[ 12.656458] *pdpt = 0000000000000000 *pde = f000ff53f000ff53
[ 12.656460] BUG: unable to handle page fault for address: 0504042e
[ 12.656460] #PF: supervisor read access in kernel mode
[ 12.656461] #PF: error_code(0x0000) - not-present page
[ 12.656461] *pdpt = 0000000000000000 *pde = f000ff53f000ff53
[ 12.656464] BUG: unable to handle page fault for address: 0504042e
[ 12.656465] #PF: supervisor read access in kernel mode
[ 12.656467] #PF: error_code(0x0000) - not-present page
[ 12.657815] load_module+0x3aa/0xf90
[ 12.659197] *pdpt = 0000000000000000 *pde = f000ff53f000ff53
[ 12.660466] ? vfs_read+0x142/0x180
[ 12.661533] BUG: unable to handle page fault for address: 0504042e
[ 12.662964] ? kernel_read_file+0x1b2/0x230
[ 12.664174] #PF: supervisor read access in kernel mode
[ 12.664176] #PF: error_code(0x0000) - not-present page
[ 12.664177] *pdpt = 0000000000000000 *pde = f000ff53f000ff53
[ 12.664182] BUG: unable to handle page fault for address: 0504042e
[ 12.664183] #PF: supervisor read access in kernel mode
[ 12.664183] #PF: error_code(0x0000) - not-present page
[ 12.664184] *pdpt = 0000000000000000 *pde = f000ff53f000ff53
[ 12.664186] BUG: unable to handle page fault for address: 0504042e
[ 12.664187] #PF: supervisor read access in kernel mode
[ 12.664187] #PF: error_code(0x0000) - not-present page
[ 12.664188] *pdpt = 0000000000000000 *pde = f000ff53f000ff53
[ 12.664190] BUG: unable to handle page fault for address: 0504042e
[ 12.664191] #PF: supervisor read access in kernel mode
[ 12.664191] #PF: error_code(0x0000) - not-present page
[ 12.664192] *pdpt = 0000000000000000 *pde = f000ff53f000ff53
[ 12.664195] BUG: unable to handle page fault for address: 0504042e
[ 12.665429] ? kernel_read_file_from_fd+0x36/0x70
[ 12.666061] #PF: supervisor read access in kernel mode
[ 12.667419] __ia32_sys_finit_module+0x8f/0xe0
[ 12.668671] #PF: error_code(0x0000) - not-present page
[ 12.668672] *pdpt = 0000000000000000 *pde = f000ff53f000ff53
[ 12.668681] BUG: unable to handle page fault for address: 0504042e
[ 12.668682] #PF: supervisor read access in kernel mode
[ 12.668683] #PF: error_code(0x0000) - not-present page
[ 12.668683] *pdpt = 0000000000000000 *pde = f000ff53f000ff53
[ 12.668686] BUG: unable to handle page fault for address: 0504042e
[ 12.668687] #PF: supervisor read access in kernel mode
[ 12.668688] #PF: error_code(0x0000) - not-present page
[ 12.668688] *pdpt = 0000000000000000 *pde = f000ff53f000ff53
[ 12.668691] BUG: unable to handle page fault for address: 0504042e
[ 12.668691] #PF: supervisor read access in kernel mode
[ 12.668692] #PF: error_code(0x0000) - not-present page
[ 12.668692] *pdpt = 0000000000000000 *pde = f000ff53f000ff53
[ 12.668695] BUG: unable to handle page fault for address: 0504042e
[ 12.669508] ---[ end trace fc996799a48790a6 ]---
To reproduce:
# build kernel
cd linux
cp config-5.8.0-rc3-00009-g676800b78feed .config
make HOSTCC=gcc-9 CC=gcc-9 ARCH=i386 olddefconfig prepare modules_prepare bzImage
git clone https://github.com/intel/lkp-tests.git
cd lkp-tests
bin/lkp qemu -k <bzImage> job-script # job-script is attached in this email
Thanks,
lkp
Hi Kees,
Thanks for looking at my patch series to see how it relates.
I see what you're trying to accomplish in various areas of cleanup.
I'll comment as I go through your individual emails.
1 comment below.
On 2020-07-07 2:55 p.m., Kees Cook wrote:
> On Tue, Jul 07, 2020 at 09:42:02AM -0700, Scott Branden wrote:
>> On 2020-07-07 1:19 a.m., Kees Cook wrote:
>>> FIRMWARE_PREALLOC_BUFFER is a "how", not a "what", and confuses the LSMs
>>> that are interested in filtering between types of things. The "how"
>>> should be an internal detail made uninteresting to the LSMs.
>>>
>>> Fixes: a098ecd2fa7d ("firmware: support loading into a pre-allocated buffer")
>>> Fixes: fd90bc559bfb ("ima: based on policy verify firmware signatures (pre-allocated buffer)")
>>> Fixes: 4f0496d8ffa3 ("ima: based on policy warn about loading firmware (pre-allocated buffer)")
>>> [...]
>>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>>> index 3f881a892ea7..95fc775ed937 100644
>>> --- a/include/linux/fs.h
>>> +++ b/include/linux/fs.h
>>> @@ -2993,10 +2993,10 @@ static inline void i_readcount_inc(struct inode *inode)
>>> #endif
>>> extern int do_pipe_flags(int *, int);
>>> +/* This is a list of *what* is being read, not *how*. */
>>> #define __kernel_read_file_id(id) \
>>> id(UNKNOWN, unknown) \
>>> id(FIRMWARE, firmware) \
>> With this change, I'm trying to figure out how the partial firmware read is
>> going to work on top of this reachitecture.
>> Is it going to be ok to add READING_PARTIAL_FIRMWARE here as that is a
>> "what"?
> No, that's why I said you need to do the implementation within the API
> and not expect each LSM to implement their own (as I mentioned both
> times):
>
> https://lore.kernel.org/lkml/202005221551.5CA1372@keescook/
> https://lore.kernel.org/lkml/202007061950.F6B3D9E6A@keescook/
>
> I will reply in the thread above.
>
>>> - id(FIRMWARE_PREALLOC_BUFFER, firmware) \
>> My patch series gets rejected any time I make a change to the
>> kernel_read_file* region in linux/fs.h.
>> The requirement is for this api to move to another header file outside of
>> linux/fs.h
>> It seems the same should apply to your change.
> Well I'm hardly making the same level of changes, but yeah, sure, if
> that helps move things along, I can include that here.
>
>> Could you please add the following patch to the start of you patch series to
>> move the kernel_read_file* to its own include file?
>> https://patchwork.kernel.org/patch/11647063/
> https://lore.kernel.org/lkml/[email protected]/
>
> You've included it in include/linux/security.h and that should be pretty
> comprehensive, it shouldn't be needed in so many .c files.
Some people want the header files included in each c file they are used.
Others want header files not included if they are included in another
header file.
I chose the first approach: every file that uses the api includes the
header file.
I didn't know there was a standard approach to only put it in security.h
>
On Tue, Jul 07, 2020 at 08:06:23PM -0700, Scott Branden wrote:
> Some people want the header files included in each c file they are used.
> Others want header files not included if they are included in another header
> file.
Ah, yes. That's fine then, leave them in the .c files.
--
Kees Cook
Hi Kees,
This patch fails during booting of my system - see below.
On 2020-07-07 1:19 a.m., Kees Cook wrote:
> FIRMWARE_PREALLOC_BUFFER is a "how", not a "what", and confuses the LSMs
> that are interested in filtering between types of things. The "how"
> should be an internal detail made uninteresting to the LSMs.
>
> Fixes: a098ecd2fa7d ("firmware: support loading into a pre-allocated buffer")
> Fixes: fd90bc559bfb ("ima: based on policy verify firmware signatures (pre-allocated buffer)")
> Fixes: 4f0496d8ffa3 ("ima: based on policy warn about loading firmware (pre-allocated buffer)")
> Signed-off-by: Kees Cook <[email protected]>
> ---
> drivers/base/firmware_loader/main.c | 5 ++---
> fs/exec.c | 7 ++++---
> include/linux/fs.h | 2 +-
> security/integrity/ima/ima_main.c | 6 ++----
> 4 files changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
> index ca871b13524e..c2f57cedcd6f 100644
> --- a/drivers/base/firmware_loader/main.c
> +++ b/drivers/base/firmware_loader/main.c
> @@ -465,14 +465,12 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
> int i, len;
> int rc = -ENOENT;
> char *path;
> - enum kernel_read_file_id id = READING_FIRMWARE;
> size_t msize = INT_MAX;
> void *buffer = NULL;
>
> /* Already populated data member means we're loading into a buffer */
> if (!decompress && fw_priv->data) {
> buffer = fw_priv->data;
> - id = READING_FIRMWARE_PREALLOC_BUFFER;
> msize = fw_priv->allocated_size;
> }
>
> @@ -496,7 +494,8 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
>
> /* load firmware files from the mount namespace of init */
> rc = kernel_read_file_from_path_initns(path, &buffer,
> - &size, msize, id);
> + &size, msize,
> + READING_FIRMWARE);
> if (rc) {
> if (rc != -ENOENT)
> dev_warn(device, "loading %s failed with error %d\n",
> diff --git a/fs/exec.c b/fs/exec.c
> index e6e8a9a70327..2bf549757ce7 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -927,6 +927,7 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
> {
> loff_t i_size, pos;
> ssize_t bytes = 0;
> + void *allocated = NULL;
> int ret;
>
> if (!S_ISREG(file_inode(file)->i_mode) || max_size < 0)
> @@ -950,8 +951,8 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
> goto out;
> }
>
> - if (id != READING_FIRMWARE_PREALLOC_BUFFER)
> - *buf = vmalloc(i_size);
> + if (!*buf)
The assumption that *buf is always NULL when id !=
READING_FIRMWARE_PREALLOC_BUFFER doesn't appear to be correct.
I get unhandled page faults due to this change on boot.
> + *buf = allocated = vmalloc(i_size);
> if (!*buf) {
> ret = -ENOMEM;
> goto out;
> @@ -980,7 +981,7 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
>
> out_free:
> if (ret < 0) {
> - if (id != READING_FIRMWARE_PREALLOC_BUFFER) {
> + if (allocated) {
> vfree(*buf);
> *buf = NULL;
> }
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 3f881a892ea7..95fc775ed937 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2993,10 +2993,10 @@ static inline void i_readcount_inc(struct inode *inode)
> #endif
> extern int do_pipe_flags(int *, int);
>
> +/* This is a list of *what* is being read, not *how*. */
> #define __kernel_read_file_id(id) \
> id(UNKNOWN, unknown) \
> id(FIRMWARE, firmware) \
> - id(FIRMWARE_PREALLOC_BUFFER, firmware) \
> id(FIRMWARE_EFI_EMBEDDED, firmware) \
> id(MODULE, kernel-module) \
> id(KEXEC_IMAGE, kexec-image) \
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index c1583d98c5e5..f80ee4ce4669 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -611,19 +611,17 @@ void ima_post_path_mknod(struct dentry *dentry)
> int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
> {
> /*
> - * READING_FIRMWARE_PREALLOC_BUFFER
> - *
> * Do devices using pre-allocated memory run the risk of the
> * firmware being accessible to the device prior to the completion
> * of IMA's signature verification any more than when using two
> - * buffers?
> + * buffers? It may be desirable to include the buffer address
> + * in this API and walk all the dma_map_single() mappings to check.
> */
> return 0;
> }
>
> const 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,
On Fri, Jul 10, 2020 at 02:00:32PM -0700, Scott Branden wrote:
> > @@ -950,8 +951,8 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
> > goto out;
> > }
> > - if (id != READING_FIRMWARE_PREALLOC_BUFFER)
> > - *buf = vmalloc(i_size);
> > + if (!*buf)
> The assumption that *buf is always NULL when id !=
> READING_FIRMWARE_PREALLOC_BUFFER doesn't appear to be correct.
> I get unhandled page faults due to this change on boot.
Did it give you a stack backtrace?
On 2020-07-10 3:04 p.m., Matthew Wilcox wrote:
> On Fri, Jul 10, 2020 at 02:00:32PM -0700, Scott Branden wrote:
>>> @@ -950,8 +951,8 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
>>> goto out;
>>> }
>>> - if (id != READING_FIRMWARE_PREALLOC_BUFFER)
>>> - *buf = vmalloc(i_size);
>>> + if (!*buf)
>> The assumption that *buf is always NULL when id !=
>> READING_FIRMWARE_PREALLOC_BUFFER doesn't appear to be correct.
>> I get unhandled page faults due to this change on boot.
> Did it give you a stack backtrace?
Yes, but there's no requirement that *buf need to be NULL when calling
this function.
To fix my particular crash I added the following locally:
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3989,7 +3989,7 @@ SYSCALL_DEFINE3(finit_module, int, fd, const char
__user *, uargs, int, flags)
{
struct load_info info = { };
loff_t size;
- void *hdr;
+ void *hdr = NULL;
int err;
err = may_init_module();
>
On Fri, Jul 10, 2020 at 03:10:25PM -0700, Scott Branden wrote:
>
>
> On 2020-07-10 3:04 p.m., Matthew Wilcox wrote:
> > On Fri, Jul 10, 2020 at 02:00:32PM -0700, Scott Branden wrote:
> > > > @@ -950,8 +951,8 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
> > > > goto out;
> > > > }
> > > > - if (id != READING_FIRMWARE_PREALLOC_BUFFER)
> > > > - *buf = vmalloc(i_size);
> > > > + if (!*buf)
> > > The assumption that *buf is always NULL when id !=
> > > READING_FIRMWARE_PREALLOC_BUFFER doesn't appear to be correct.
> > > I get unhandled page faults due to this change on boot.
> > Did it give you a stack backtrace?
> Yes, but there's no requirement that *buf need to be NULL when calling this
> function.
> To fix my particular crash I added the following locally:
>
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3989,7 +3989,7 @@ SYSCALL_DEFINE3(finit_module, int, fd, const char
> __user *, uargs, int, flags)
> ?{
> ???? struct load_info info = { };
> ???? loff_t size;
> -??? void *hdr;
> +??? void *hdr = NULL;
> ???? int err;
>
> ???? err = may_init_module();
> >
>
Thanks for the diagnosis and fix! I haven't had time to cycle back
around to this series yet. Hopefully soon. :)
--
Kees Cook
Hi Kees,
On 2020-07-10 3:44 p.m., Kees Cook wrote:
> On Fri, Jul 10, 2020 at 03:10:25PM -0700, Scott Branden wrote:
>>
>> On 2020-07-10 3:04 p.m., Matthew Wilcox wrote:
>>> On Fri, Jul 10, 2020 at 02:00:32PM -0700, Scott Branden wrote:
>>>>> @@ -950,8 +951,8 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
>>>>> goto out;
>>>>> }
>>>>> - if (id != READING_FIRMWARE_PREALLOC_BUFFER)
>>>>> - *buf = vmalloc(i_size);
>>>>> + if (!*buf)
>>>> The assumption that *buf is always NULL when id !=
>>>> READING_FIRMWARE_PREALLOC_BUFFER doesn't appear to be correct.
>>>> I get unhandled page faults due to this change on boot.
>>> Did it give you a stack backtrace?
>> Yes, but there's no requirement that *buf need to be NULL when calling this
>> function.
>> To fix my particular crash I added the following locally:
>>
>> --- a/kernel/module.c
>> +++ b/kernel/module.c
>> @@ -3989,7 +3989,7 @@ SYSCALL_DEFINE3(finit_module, int, fd, const char
>> __user *, uargs, int, flags)
>> {
>> struct load_info info = { };
>> loff_t size;
>> - void *hdr;
>> + void *hdr = NULL;
>> int err;
>>
>> err = may_init_module();
> Thanks for the diagnosis and fix! I haven't had time to cycle back
> around to this series yet. Hopefully soon. :)
I don't consider this a complete fix as there may be other callers which
do not initialize
the *buf param to NULL before calling kernel_read_file.
But, it does boot my system. Also, I was able to make modifications for my
pread changes that pass (and the IMA works with IMA patch in my series
is dropped completely with your changes in place).
So your changes work for me other than the hack needed above.
>
Hi Kees
On 2020-07-10 3:44 p.m., Kees Cook wrote:
> On Fri, Jul 10, 2020 at 03:10:25PM -0700, Scott Branden wrote:
>>
>> On 2020-07-10 3:04 p.m., Matthew Wilcox wrote:
>>> On Fri, Jul 10, 2020 at 02:00:32PM -0700, Scott Branden wrote:
>>>>> @@ -950,8 +951,8 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
>>>>> goto out;
>>>>> }
>>>>> - if (id != READING_FIRMWARE_PREALLOC_BUFFER)
>>>>> - *buf = vmalloc(i_size);
>>>>> + if (!*buf)
>>>> The assumption that *buf is always NULL when id !=
>>>> READING_FIRMWARE_PREALLOC_BUFFER doesn't appear to be correct.
>>>> I get unhandled page faults due to this change on boot.
>>> Did it give you a stack backtrace?
>> Yes, but there's no requirement that *buf need to be NULL when calling this
>> function.
>> To fix my particular crash I added the following locally:
>>
>> --- a/kernel/module.c
>> +++ b/kernel/module.c
>> @@ -3989,7 +3989,7 @@ SYSCALL_DEFINE3(finit_module, int, fd, const char
>> __user *, uargs, int, flags)
>> {
>> struct load_info info = { };
>> loff_t size;
>> - void *hdr;
>> + void *hdr = NULL;
>> int err;
>>
>> err = may_init_module();
> Thanks for the diagnosis and fix! I haven't had time to cycle back
> around to this series yet. Hopefully soon. :)
>
In order to assist in your patchset I have combined it with my patch
series here:
https://github.com/sbranden/linux/tree/kernel_read_file_for_kees
Please let me know if this matches your expectations for my patches or
if there is something else I need to change.
Thanks,
Scott
On Thu, Jul 16, 2020 at 01:35:17PM -0700, Scott Branden wrote:
> On 2020-07-10 3:44 p.m., Kees Cook wrote:
> > On Fri, Jul 10, 2020 at 03:10:25PM -0700, Scott Branden wrote:
> > >
> > > On 2020-07-10 3:04 p.m., Matthew Wilcox wrote:
> > > > On Fri, Jul 10, 2020 at 02:00:32PM -0700, Scott Branden wrote:
> > > > > > @@ -950,8 +951,8 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
> > > > > > goto out;
> > > > > > }
> > > > > > - if (id != READING_FIRMWARE_PREALLOC_BUFFER)
> > > > > > - *buf = vmalloc(i_size);
> > > > > > + if (!*buf)
> > > > > The assumption that *buf is always NULL when id !=
> > > > > READING_FIRMWARE_PREALLOC_BUFFER doesn't appear to be correct.
> > > > > I get unhandled page faults due to this change on boot.
> > > > Did it give you a stack backtrace?
> > > Yes, but there's no requirement that *buf need to be NULL when calling this
> > > function.
> > > To fix my particular crash I added the following locally:
> > >
> > > --- a/kernel/module.c
> > > +++ b/kernel/module.c
> > > @@ -3989,7 +3989,7 @@ SYSCALL_DEFINE3(finit_module, int, fd, const char
> > > __user *, uargs, int, flags)
> > > ?{
> > > ???? struct load_info info = { };
> > > ???? loff_t size;
> > > -??? void *hdr;
> > > +??? void *hdr = NULL;
> > > ???? int err;
> > >
> > > ???? err = may_init_module();
> > Thanks for the diagnosis and fix! I haven't had time to cycle back
> > around to this series yet. Hopefully soon. :)
> >
> In order to assist in your patchset I have combined it with my patch series
> here:
> https://github.com/sbranden/linux/tree/kernel_read_file_for_kees
>
> Please let me know if this matches your expectations for my patches or if
> there is something else I need to change.
Thanks! I was working on the next revision of this last night, and I'm
trying to get through today's email to finish it. I'll take a look!
--
Kees Cook