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
