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

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:

quoted

On Tue, Dec 07, 2021 at 12:36:17PM +0000, David Brazdil wrote:

quoted

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 :)

quoted

quoted

+#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.

quoted

quoted

+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?

thanks,

greg k-h