Date:   Tue, 7 Dec 2021 16:44:18 +0000
From:   David Brazdil <dbrazdil@google.com>
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     Rob Herring <robh+dt@kernel.org>, Jonathan Corbet <corbet@lwn.net>,
        Derek Kiernan <derek.kiernan@xilinx.com>,
        Dragan Cvetic <dragan.cvetic@xilinx.com>,
        Arnd Bergmann <arnd@arndb.de>,
        Hans de Goede <hdegoede@redhat.com>,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-doc@vger.kernel.org, Andrew Scull <ascull@google.com>,
        Will Deacon <will@kernel.org>
Subject: Re: [PATCH 2/2] misc: dice: Add driver to forward secrets to
 userspace
Message-ID: <Ya+PYiP43YxfLS4x@google.com>
References: <20211207123617.3040177-1-dbrazdil@google.com>
 <20211207123617.3040177-3-dbrazdil@google.com>
 <Ya9cwZ94QatewwIc@kroah.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Ya9cwZ94QatewwIc@kroah.com>
Precedence: bulk

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?

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

Thanks,
David