On Fri, 2021-10-01 at 13:26 +0200, Greg KH wrote:
> On Fri, Oct 01, 2021 at 04:13:58AM -0700, David E. Box wrote:
> > On Fri, 2021-10-01 at 09:29 +0200, Greg KH wrote:
> > > On Thu, Sep 30, 2021 at 06:28:15PM -0700, David E. Box wrote:
> > > > +static long sdsi_device_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> > > > +{
> > > > + struct miscdevice *miscdev = file->private_data;
> > > > + struct sdsi_priv *priv = to_sdsi_priv(miscdev);
> > > > + void __user *argp = (void __user *)arg;
> > > > + long ret = -EINVAL;
> > > > +
> > > > + if (!priv->dev_present)
> > > > + return -ENODEV;
> > > > +
> > > > + if (!priv->sdsi_enabled)
> > > > + return -EPERM;
> > > > +
> > > > + if (cmd == SDSI_IF_READ_STATE)
> > > > + return sdsi_if_read_state_cert(priv, argp);
> > > > +
> > > > + mutex_lock(&priv->akc_lock);
> > > > + switch (cmd) {
> > > > + case SDSI_IF_PROVISION_AKC:
> > > > + /*
> > > > + * While writing an authentication certificate disallow other openers
> > > > + * from using AKC or CAP.
> > > > + */
> > > > + if (!priv->akc_owner)
> > > > + priv->akc_owner = file;
> > > > +
> > > > + if (priv->akc_owner != file) {
> > >
> > > Please explain how this test would ever trigger and how you tested it?
> > >
> > > What exactly are you trying to protect from here? If userspace has your
> > > file descriptor, it can do whatever it wants, don't try to be smarter
> > > than it as you will never win.
> > >
> > > And why are you using ioctls at all here? As you are just
> > > reading/writing to the hardware directly, why not just use a binary
> > > sysfs file to be that pipe? What requires an ioctl at all?
> >
> > So an original internal version of this did use binary attributes. But there was concern during
> > review that a flow, particularly when doing the two write operations, could not be handled
> > atomically while exposed as separate files. Above is the attempt to handle the situation in the
> > ioctl. That is, whichever opener performs AKC write first would lock out all other openers from
> > performing any write until that file is closed. This is to avoid interfering with that process,
> > should the opener also decide to perform a CAP operation.
>
> Unfortunately, your code here does not prevent that at all, so your
> moving off of a binary sysfs attribute changed nothing.
>
> You can "prevent" this from happening just as easily through a sysfs
> attribute as you can a character device node.
>
> > There may be future commands requiring RW ioctls as well.
>
> How am I or anyone else supposed to know that? We write code and review
> it for _today_, not what might be sometime in the future someday. As
> that will be dealt with when it actually happens.
Sure. Thanks for the insightful review. I'll take your comments back and submit with the reviewed-by
tag. Will probably switch back to sysfs.
David
>
> greg k-h
