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

From: Kees Cook <keescook@chromium.org>
To: Scott Branden <scott.branden@broadcom.com>
Cc: James Morris <jmorris@namei.org>,
	Luis Chamberlain <mcgrof@kernel.org>,
	Mimi Zohar <zohar@linux.ibm.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	Jessica Yu <jeyu@kernel.org>,
	Dmitry Kasatkin <dmitry.kasatkin@gmail.com>,
	"Serge E. Hallyn" <serge@hallyn.com>,
	Casey Schaufler <casey@schaufler-ca.com>,
	"Eric W. Biederman" <ebiederm@xmission.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Matthew Garrett <matthewgarrett@google.com>,
	David Howells <dhowells@redhat.com>,
	Mauro Carvalho Chehab <mchehab+huawei@kernel.org>,
	Randy Dunlap <rdunlap@infradead.org>,
	"Joel Fernandes (Google)" <joel@joelfernandes.org>,
	KP Singh <kpsingh@google.com>, Dave Olsthoorn <dave@bewaar.me>,
	Hans de Goede <hdegoede@redhat.com>,
	Peter Jones <pjones@redhat.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Stephen Boyd <stephen.boyd@linaro.org>,
	Paul Moore <paul@paul-moore.com>,
	linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	linux-integrity@vger.kernel.org,
	linux-security-module@vger.kernel.org,
	Christoph Hellwig <hch@lst.de>
Subject: Re: [PATCH 2/4] fs: Remove FIRMWARE_PREALLOC_BUFFER from kernel_read_file() enums
Date: Tue, 7 Jul 2020 14:55:35 -0700	[thread overview]
Message-ID: <202007071447.D96AA42ECE@keescook> (raw)
In-Reply-To: <0a5e2c2e-507c-9114-5328-5943f63d707e@broadcom.com>

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

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