Received-SPF: pass (google.com: domain of linux-crypto+bounces-4070-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Wed, 8 May 2024 14:33:12 +0300
From: Andy Shevchenko <andy@kernel.org>
To: Marek =?iso-8859-1?Q?Beh=FAn?= <kabel@kernel.org>
Cc: Gregory CLEMENT <gregory.clement@bootlin.com>,
	Arnd Bergmann <arnd@arndb.de>, soc@kernel.org, arm@kernel.org,
	Hans de Goede <hdegoede@redhat.com>,
	Ilpo =?iso-8859-1?Q?J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-crypto@vger.kernel.org,
	Dan Carpenter <dan.carpenter@linaro.org>
Subject: Re: [PATCH v9 7/9] platform: cznic: turris-omnia-mcu: Add support
 for digital message signing via debugfs
Message-ID: <Zjti-FkUCAQzMmrQ@smile.fi.intel.com>
References: <20240508103118.23345-1-kabel@kernel.org>
 <20240508103118.23345-8-kabel@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20240508103118.23345-8-kabel@kernel.org>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo

On Wed, May 08, 2024 at 12:31:16PM +0200, Marek Beh?n wrote:
> Add support for digital message signing with private key stored in the
> MCU. Boards with MKL MCUs have a NIST256p ECDSA private key created
> when manufactured. The private key is not readable from the MCU, but
> MCU allows for signing messages with it and retrieving the public key.
> 
> As described in a similar commit 50524d787de3 ("firmware:
> turris-mox-rwtm: support ECDSA signatures via debugfs"):
>   The optimal solution would be to register an akcipher provider via
>   kernel's crypto API, but crypto API does not yet support accessing
>   akcipher API from userspace (and probably won't for some time, see
>   https://www.spinics.net/lists/linux-crypto/msg38388.html).
> 
> Therefore we add support for accessing this signature generation
> mechanism via debugfs for now, so that userspace can access it.

...

> +static irqreturn_t omnia_msg_signed_irq_handler(int irq, void *dev_id)
> +{
> +	u8 reply[1 + OMNIA_MCU_CRYPTO_SIGNATURE_LEN];
> +	struct omnia_mcu *mcu = dev_id;
> +	int err;
> +
> +	err = omnia_cmd_read(mcu->client, OMNIA_CMD_CRYPTO_COLLECT_SIGNATURE,
> +			     reply, sizeof(reply));
> +	if (!err && reply[0] != OMNIA_MCU_CRYPTO_SIGNATURE_LEN)
> +		err = -EIO;
> +
> +	guard(mutex)(&mcu->sign_lock);
> +
> +	if (mcu->sign_state == SIGN_STATE_REQUESTED) {
> +		mcu->sign_err = err;
> +		if (!err)
> +			memcpy(mcu->signature, &reply[1],
> +			       OMNIA_MCU_CRYPTO_SIGNATURE_LEN);

> +		mcu->sign_state = SIGN_STATE_COLLECTED;

Even for an error case?

> +		complete(&mcu->msg_signed_completion);
> +	}
> +
> +	return IRQ_HANDLED;
> +}

...

> +	scoped_guard(mutex, &mcu->sign_lock)
> +		if (mcu->sign_state != SIGN_STATE_REQUESTED &&
> +		    mcu->sign_state != SIGN_STATE_COLLECTED)
> +			return -ENODATA;

{}

Don't you want interruptible mutex? In such case you might need to return
-ERESTARTSYS. OTOH, this is debugfs, we don't much care.

...

> +#define OMNIA_MCU_CRYPTO_PUBLIC_KEY_LEN	33

33? Hmm... does it mean (32 + 1)?

-- 
With Best Regards,
Andy Shevchenko