Message-ID: <20150827212907.GF8051@wotan.suse.de>
Date:	Thu, 27 Aug 2015 23:29:07 +0200
From:	"Luis R. Rodriguez" <mcgrof@...e.com>
To:	David Woodhouse <dwmw2@...radead.org>
Cc:	David Howells <dhowells@...hat.com>,
	Mimi Zohar <zohar@...ux.vnet.ibm.com>,
	Andy Lutomirski <luto@...capital.net>,
	Kees Cook <keescook@...omium.org>,
	"Roberts, William C" <william.c.roberts@...el.com>,
	"linux-security-module@...r.kernel.org" 
	<linux-security-module@...r.kernel.org>,
	linux-kernel@...r.kernel.org, linux-wireless@...r.kernel.org,
	"james.l.morris@...cle.com" <james.l.morris@...cle.com>,
	"serge@...lyn.com" <serge@...lyn.com>,
	Vitaly Kuznetsov <vkuznets@...hat.com>,
	Paul Moore <paul@...l-moore.com>,
	Eric Paris <eparis@...isplace.org>, selinux@...ho.nsa.gov,
	Stephen Smalley <sds@...ho.nsa.gov>,
	"Schaufler, Casey" <casey.schaufler@...el.com>,
	"Luis R. Rodriguez" <mcgrof@...not-panic.com>,
	Dmitry Kasatkin <dmitry.kasatkin@...il.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Peter Jones <pjones@...hat.com>, Takashi Iwai <tiwai@...e.de>,
	Ming Lei <ming.lei@...onical.com>, Joey Lee <jlee@...e.de>,
	"Vojtěch Pavlík" <vojtech@...e.com>,
	Kyle McMartin <kyle@...nel.org>,
	Seth Forshee <seth.forshee@...onical.com>,
	Matthew Garrett <mjg59@...f.ucam.org>,
	Johannes Berg <johannes@...solutions.net>
Subject: Re: Linux Firmware Signing

On Thu, Aug 27, 2015 at 10:57:23AM -0000, David Woodhouse wrote:
> > Luis R. Rodriguez <mcgrof@...e.com> wrote:
> >
> >> "PKCS#7: Add an optional authenticated attribute to hold firmware name"
> >> https://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/commit/?h=fwsign-pkcs7&id=1448377a369993f864915743cfb34772e730213good
> >>
> >>         1.3.6.1.4.1.2312.16     Linux kernel
> >>         1.3.6.1.4.1.2312.16.2   - PKCS#7/CMS SignerInfo attribute types
> >>         1.3.6.1.4.1.2312.16.2.1   - firmwareName
> >>
> >> I take it you are referring to this?
> >
> > Yes.
> >
> >> If we follow this model we'd then need something like:
> >>
> >>         1.3.6.1.4.1.2312.16.2.2   - seLinuxPolicyName
> >>
> >> That should mean each OID that has different file names would need to be
> >> explicit about and have a similar entry on the registry. I find that
> >> pretty redundant and would like to avoid that if possible.
> >
> > firmwareName is easy for people to understand - it's the name the kernel
> > asks for and the filename of the blob.  seLinuxPolicyName is, I think, a
> > lot more tricky since a lot of people don't use SELinux, and most that do
> > don't understand it (most people that use it aren't even really aware of
> > it).
> >
> > If you can use the firmwareName as the SELinux/LSM key, I would suggest
> > doing so - even if you dress it up as a path
> > (/lib/firmware/<firmwareName>).
> 
> In conversation with Mimi last week she was very keen on the model where
> we load modules & firmware in such a fashion that the kernel has access to
> the original inode -- by passing in a fd,

Sure, so let's be specific to ensure what Mimi needs is there. I though there
was work needed on modules but that seems covered and work then seems only
needed for kexec and SELinux policy files (and a review of other possible file
consumers in the kernel) for what you describe. I also went ahead and studied
areas where we can share code now as I was looking at this code now, and also
would like to recap on the idea of possibly just sharing the same LSM hook
for all "read this special file from the fs in the kernel" cases. Details below.

Fortnately the LSM hooks uses struct file and with this you can get the inode
with this:

	struct inode *inode = file_inode(file);

For modules we have this LSM hook:

int (*kernel_module_from_file)(struct file *file);

This can be used for finit_module(). Its used as follows, the fd comes from
finit_module() syscall.

SYSCALL_DEFINE3(finit_module, int, fd, const char __user *, uargs, int, flags)
{
	...
	err = copy_module_from_fd(fd, &info);                                   
	if (err)                                                                
		return err; 
	...
}

static int copy_module_from_fd(int fd, struct load_info *info)
{
	struct fd f = fdget(fd);
	...
	err = security_kernel_module_from_file(f.file);                         
	if (err)                                                                
		goto out;
}

For firmware we have this LSM hook:

int (*kernel_fw_from_file)(struct file *file, char *buf, size_t size);  

> or in the firmware case by doing the fs lookup directly.

Right so now that firmware usermode helper is behind us (systemd ripped it) we
do the fs lookup directly ourselves. One of my side goals with the extensible
firmware API was also to allow for us to take a leap and let drivers
skip completely the usermode helper so we can then phase that code to the
only required remainign user: the dell-rbu driver. Anyway, once we have the
path built up we use it as follows.

static int fw_read_file(const char *path, void **_buf, size_t *_size)           
{
	struct file *file;
	...
	file = filp_open(path, O_RDONLY, 0);                                    
	if (IS_ERR(file))                                                       
		return PTR_ERR(file);  
	...
	rc = security_kernel_fw_from_file(file, buf, size);                     
	if (rc)                                                                 
		goto err_buf;  
	...
}

I was under the impression that work was needed to add an LSM hook which would
grant the LSM access to the file specific data for modules but that's already
there with finit_module()! So Mimi needs is already there for modules as well
now.

We have no LSM hook for kexec, even though the kernel does have access to the
fd, so if you wanted the struct file for an LSM it should be possible as the
syscall for kexec is:

SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,                
                unsigned long, cmdline_len, const char __user *, cmdline_ptr,   
                unsigned long, flags) 
{
	...
}

I noted earlier however that kexec is currently an x86 thing only though, and
Howells clarified that this is because we want kernel image signing as an
option (its a Kconfig option), and only PE supports a built-in signing method.
Its unclear to me who extends ELF and if its worthwhile to consider adding
support there for a signing method. Howells noted that there are rumours other
archs would support kexec, its unclear what they would use.

Even though kexec remains x86 specific an LSM for it should easily be possible
to add, but more on this below...

> So surely you have all the SELinux labelling you need?

SELinux uses: security_load_policy(data, len), refer to selinuxfs sel_load_ops.
Since its write operation on its file_operation is sel_write_load() and that
is as follows:

static ssize_t sel_write_load(struct file *file, const char __user *buf,        
			      size_t count, loff_t *ppos) 
{
	...
}

We should be able to add yet-another LSM hook here to let the kernel / LSM have
access to the inode, is that LSM hook desirable ? But folks, before you answer
note that there's a growing trend here! Its point 1 Kees had made earlier.  I
was hesitant to go into details as I think fw signing needs to be baked first
but.. since we're reviewing all these details now it seems logical to go down
the rabbit hole further.

Everywhere where we fetch a file from within the kernel either directly (say
firmware load, 802.11 regulatory request) or from userspace request (SELinux
policy load node) we end up having to sprinkle a new LSM hook. In fact for
modules and kexec there were syscalls added too. There might be a possiblity
for sharing some of these requests / code so some review is in order for it.

Here's my review if we wanted to try sharing things, in consideration and
review of:

  * SELinux policy files
  * modules
  * firmware / system data (consider replacing CRDA)
  * kexec

----

  * SELinux policy files:

sel_write_load() is very specific, its part of the selinuxfs and it just
uses copy_from_user() to dump the data from the file onto a vmalloc'd
piece of memory. We don't exactly read arbitrary files from the fs then.
If we *really* wanted to generalize things further we probably could
but I'm not going to lead any discussion about design over selinuxfs,
I'll let the folks behind it think about that themselves.

  * modules
  * firmware / system data

modules + firmware: there seems to be some code sharing we could possibly do
for both fw_read_file() and copy_module_from_fd(), note we are going to use
different keys for vetting each of these. It may be possible to share the
LSM hook here. All parties would just need to agree.

  * kexec

kexec works by reading files and setting up pointers for the different
segments it needs for bootup, it does this for both the kernel and initrd
if present. It however uses its own copy_file_from_fd() routine and no
surprise here, there's code that can be shared as well. We'd be using
a separate signature for kexec, so that'd be vetted on its own already.
It may be possible to share the same LSM hook here, again all parties
would just need to agree.

----

So conclusion:

After fw signing gets baked (or I'll do that as I work with the system data
helpers) there is possible work here to consolidate firmware's fw_read_file(),
module's fw_read_file(), and kexec's copy_file_from_fd() into a core kernel
tiny helper that gets it done right for all. If we really wanted to we could
also just use the same LSM hook for all, this hook would surely have the
struct file as Mimi wants as well. Unless I misunderstood things, at the
Linux security summit it seemed folks thought this was reasonable and
desirable. One of the gains then would be that the kernel can grow for
different use cases and files can be fetched as needed but we wouldn't have to
add yet-another-LSM hook for each new purpose, we'd just be sharing the same
fetch / LSM hook. Please discuss and let me know if this still stands, I'll
work towards any agreed upon direction with the fw signing code.

And again, there may other parts of the kernel that do similar work, just
as we found out about SELinux policy files. Those need to be identified
and studied separatley. I guess we can use grammar to hunt these down.

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists