Re: [PATCH v9 7/9] platform: cznic: turris-omnia-mcu: Add support for digital message signing via debugfs

["Marek Behún" <kabel@kernel.org>]

On Wed, 8 May 2024 14:33:12 +0300
Andy Shevchenko <andy@kernel.org> wrote:

> On Wed, May 08, 2024 at 12:31:16PM +0200, Marek Behún wrote:

...

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

Yes, the pair (errno, signature) is collected.

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

Indeed I shall use
  scoped_cond_guard(mutex_intr, return -ERESTARTSYS, &mcu->sign_lock) {
    ...
  }

And -ERESTARTSYS instead of -EINTR also for the subsequent
wait_for_completion_interruptible(), and also in trng from patch 6/9.

> ...
> 
> > +#define OMNIA_MCU_CRYPTO_PUBLIC_KEY_LEN	33  
> 
> 33? Hmm... does it mean (32 + 1)?

Rather (1 + 32), the first byte is 0x02 or 0x03, determining whether
the y coordinate of the public key elliptic curve point is positive or
negative, and the rest 32 bytes are the x coordinate.

Marek