On Tue, Jul 07, 2020 at 09:42:02AM -0700, Scott Branden wrote:

On 2020-07-07 1:19 a.m., Kees Cook wrote:

quoted

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/ (local)
https://lore.kernel.org/lkml/202007061950.F6B3D9E6A@keescook/ (local)

I will reply in the thread above.

quoted

-	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/20200706232309.12010-2-scott.branden@broadcom.com/ (local)

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