Re: [PATCH 2/2] misc: dice: Add driver to forward secrets to userspace

[David Brazdil <dbrazdil@google.com>]

On Tue, Dec 07, 2021 at 06:16:17PM +0100, Greg Kroah-Hartman wrote:
> On Tue, Dec 07, 2021 at 04:44:18PM +0000, David Brazdil wrote:
> > Hi Greg,
> > 
> > On Tue, Dec 07, 2021 at 02:08:17PM +0100, Greg Kroah-Hartman wrote:
> > > On Tue, Dec 07, 2021 at 12:36:17PM +0000, David Brazdil wrote:
> > > > Open Profile for DICE is a protocol for deriving unique secrets at boot,
> > > > used by some Android devices. The firmware/bootloader hands over secrets
> > > > in a reserved memory region, this driver takes ownership of the memory
> > > > region and exposes it to userspace via a character device that
> > > > lets userspace mmap the memory region into its process.
> > > > 
> > > > The character device can only be opened once at any given time.
> > > 
> > > Why?  That should not matter.  And your code (correctly), does not check
> > > for that.  So why say that here?
> > 
> > It does check - open() returns -EBUSY if cmpxchg of the state from READY
> > to BUSY fails. I agree this is a bit unconventional but it makes things
> > easier to reason about. With multiple open FDs the driver would have to
> > wait for all of them to get released before wiping, so one user could
> > block the wiping requested by others by holding the FD indefinitely.
> > And wiping despite other open FDs seems wrong, too. Is there a better
> > way of doing this?
> 
> Yes, totally ignore it from the kernel point of view.  You don't know
> what userspace just did with that FD the kernel gave it, it could have
> sent it across a pipe, run dup() on it, or any sort of other things.
> Just rely on open/release to know when the device is opened, and then
> when that instance is released.  If userspace wants to do looney things,
> and oddities happen, that's userspace's problem, not yours :)
> 
Fair point.

> > > > +#include <linux/cdev.h>
> > > > +#include <linux/dice.h>
> > > > +#include <linux/io.h>
> > > > +#include <linux/mm.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/of_reserved_mem.h>
> > > > +#include <linux/platform_device.h>
> > > > +
> > > > +#define DICE_MKDEV		MKDEV(MAJOR(dice_devt), 0)
> > > > +#define DICE_MINOR_COUNT	1
> > > 
> > > Please just use the misc_device api, no need to try to claim a major
> > > number for just one device node.  That will simplify your code a lot as
> > > well.
> > 
> > Ok, I'll look into it.
> > 
> > > > +static int dice_open(struct inode *inode, struct file *filp)
> > > > +{
> > > > +	struct dice_data *data;
> > > > +
> > > > +	data = container_of(inode->i_cdev, struct dice_data, cdev);
> > > > +
> > > > +	/* Never allow write access. */
> > > > +	if (filp->f_mode & FMODE_WRITE)
> > > > +		return -EROFS;
> > > 
> > > Why do you care?  Writes just will not work anyway, right?
> > 
> > There is nothing else preventing writes, the reserved memory is just plain
> > old RAM.
> 
> And you can rely on this check only?  Nothing else needed with mmap?
> And why can't userspace write to this?  What's wrong with that
> happening?

AFAICT vm_iomap_memory takes care of it. It will allow a MAP_PRIVATE
mapping of a read-only FD but not a MAP_SHARED one. I think that gives
nice guarantees to userspace that if a process opens the char device itself,
it's getting the original data, not something another process wrote there.

-David