Re: [PATCH 2/4] fs: Remove FIRMWARE_PREALLOC_BUFFER from kernel_read_file() enums

[Scott Branden]

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/
> 202007061950.F6B3D9E6A@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/20200706232309.12010-2-scott.branden@xxxxxxxxxxxx/
>
> 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
>