Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 1 Oct 2021 09:29:45 +0200
From:   Greg KH <gregkh@linuxfoundation.org>
To:     "David E. Box" <david.e.box@linux.intel.com>
Cc:     lee.jones@linaro.org, hdegoede@redhat.com, mgross@linux.intel.com,
        bhelgaas@google.com, andriy.shevchenko@linux.intel.com,
        srinivas.pandruvada@intel.com, linux-kernel@vger.kernel.org,
        linux-doc@vger.kernel.org, platform-driver-x86@vger.kernel.org,
        linux-pci@vger.kernel.org
Subject: Re: [PATCH 5/5] platform/x86: Add Intel Software Defined Silicon
 driver
Message-ID: <YVa46eU1VX7CM+Xd@kroah.com>
References: <20211001012815.1999501-1-david.e.box@linux.intel.com>
 <20211001012815.1999501-6-david.e.box@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211001012815.1999501-6-david.e.box@linux.intel.com>
Precedence: bulk

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?

thanks,

greg k-h