diff mbox series

[2/9] crypto: inside-secure - silently return -EINVAL for input error cases

Message ID 1562078400-969-5-git-send-email-pvanleeuwen@verimatrix.com (mailing list archive)
State Accepted
Delegated to: Herbert Xu
Headers show
Series crypto: inside-secure - fix cryptomgr extratests issues | expand

Commit Message

Pascal van Leeuwen July 2, 2019, 2:39 p.m. UTC 
From: Pascal van Leeuwen <pvanleeuwen@insidesecure.com>

Driver was printing an error message for certain input error cases that
should just return -EINVAL, which caused the related testmgr extra tests
to flood the kernel message log. Ensured those cases remain silent while
making some other device-specific errors a bit more verbose.

Signed-off-by: Pascal van Leeuwen <pvanleeuwen@verimatrix.com>
---
 drivers/crypto/inside-secure/safexcel.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

Comments

Antoine Tenart July 5, 2019, 2:36 p.m. UTC | #1 
Hi Pascal,

On Tue, Jul 02, 2019 at 04:39:53PM +0200, Pascal van Leeuwen wrote:
> From: Pascal van Leeuwen <pvanleeuwen@insidesecure.com>
> 
> diff --git a/drivers/crypto/inside-secure/safexcel.c b/drivers/crypto/inside-secure/safexcel.c
> index 503fef0..8e8c01d 100644
> --- a/drivers/crypto/inside-secure/safexcel.c
> +++ b/drivers/crypto/inside-secure/safexcel.c
> @@ -694,16 +694,31 @@ void safexcel_dequeue(struct safexcel_crypto_priv *priv, int ring)
>  inline int safexcel_rdesc_check_errors(struct safexcel_crypto_priv *priv,
>  				       struct safexcel_result_desc *rdesc)
>  {
> -	if (likely(!rdesc->result_data.error_code))
> +	if (likely((!rdesc->descriptor_overflow) &&
> +		   (!rdesc->buffer_overflow) &&
> +		   (!rdesc->result_data.error_code)))

You don't need the extra () here.

> +	if (rdesc->descriptor_overflow)
> +		dev_err(priv->dev, "Descriptor overflow detected");
> +
> +	if (rdesc->buffer_overflow)
> +		dev_err(priv->dev, "Buffer overflow detected");

You're not returning an error here, is there a reason for that?

I also remember having issues when adding those checks a while ago, Did
you see any of those two error messages when using the crypto engine?

Thanks!
Antoine
Pascal Van Leeuwen July 5, 2019, 2:43 p.m. UTC | #2 
> -----Original Message-----
> From: Antoine Tenart <antoine.tenart@bootlin.com>
> Sent: Friday, July 5, 2019 4:36 PM
> To: Pascal van Leeuwen <pascalvanl@gmail.com>
> Cc: linux-crypto@vger.kernel.org; antoine.tenart@bootlin.com; herbert@gondor.apana.org.au; davem@davemloft.net; Pascal Van
> Leeuwen <pvanleeuwen@insidesecure.com>; Pascal Van Leeuwen <pvanleeuwen@verimatrix.com>
> Subject: Re: [PATCH 2/9] crypto: inside-secure - silently return -EINVAL for input error cases
> 
> Hi Pascal,
> 
> On Tue, Jul 02, 2019 at 04:39:53PM +0200, Pascal van Leeuwen wrote:
> > From: Pascal van Leeuwen <pvanleeuwen@insidesecure.com>
> >
> > diff --git a/drivers/crypto/inside-secure/safexcel.c b/drivers/crypto/inside-secure/safexcel.c
> > index 503fef0..8e8c01d 100644
> > --- a/drivers/crypto/inside-secure/safexcel.c
> > +++ b/drivers/crypto/inside-secure/safexcel.c
> > @@ -694,16 +694,31 @@ void safexcel_dequeue(struct safexcel_crypto_priv *priv, int ring)
> >  inline int safexcel_rdesc_check_errors(struct safexcel_crypto_priv *priv,
> >  				       struct safexcel_result_desc *rdesc)
> >  {
> > -	if (likely(!rdesc->result_data.error_code))
> > +	if (likely((!rdesc->descriptor_overflow) &&
> > +		   (!rdesc->buffer_overflow) &&
> > +		   (!rdesc->result_data.error_code)))
> 
> You don't need the extra () here.
> 
> > +	if (rdesc->descriptor_overflow)
> > +		dev_err(priv->dev, "Descriptor overflow detected");
> > +
> > +	if (rdesc->buffer_overflow)
> > +		dev_err(priv->dev, "Buffer overflow detected");
> 
> You're not returning an error here, is there a reason for that?
> 
I guess the reason for that would be that it's a driver internal error, but the
result may still be just fine ... so I do want testmgr to continue its checks.
These should really only fire during driver development, see answer below.

> I also remember having issues when adding those checks a while ago, Did
> you see any of those two error messages when using the crypto engine?
> 
Only during development when I implemented things not fully correctly.

> Thanks!
> Antoine
> 
> --
> Antoine Ténart, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com
Antoine Tenart July 5, 2019, 3:50 p.m. UTC | #3 
On Fri, Jul 05, 2019 at 02:43:16PM +0000, Pascal Van Leeuwen wrote:
> > From: Antoine Tenart <antoine.tenart@bootlin.com>
> > On Tue, Jul 02, 2019 at 04:39:53PM +0200, Pascal van Leeuwen wrote:
> > > From: Pascal van Leeuwen <pvanleeuwen@insidesecure.com>
> > 
> > > +	if (rdesc->descriptor_overflow)
> > > +		dev_err(priv->dev, "Descriptor overflow detected");
> > > +
> > > +	if (rdesc->buffer_overflow)
> > > +		dev_err(priv->dev, "Buffer overflow detected");
> > 
> > You're not returning an error here, is there a reason for that?
> > 
> I guess the reason for that would be that it's a driver internal error, but the
> result may still be just fine ... so I do want testmgr to continue its checks.
> These should really only fire during driver development, see answer below.
> 
> > I also remember having issues when adding those checks a while ago, Did
> > you see any of those two error messages when using the crypto engine?
> > 
> Only during development when I implemented things not fully correctly.

OK, that makes sense.

Thanks!
Antoine
diff mbox series

Patch

diff --git a/drivers/crypto/inside-secure/safexcel.c b/drivers/crypto/inside-secure/safexcel.c
index 503fef0..8e8c01d 100644
--- a/drivers/crypto/inside-secure/safexcel.c
+++ b/drivers/crypto/inside-secure/safexcel.c
@@ -694,16 +694,31 @@  void safexcel_dequeue(struct safexcel_crypto_priv *priv, int ring)
 inline int safexcel_rdesc_check_errors(struct safexcel_crypto_priv *priv,
 				       struct safexcel_result_desc *rdesc)
 {
-	if (likely(!rdesc->result_data.error_code))
+	if (likely((!rdesc->descriptor_overflow) &&
+		   (!rdesc->buffer_overflow) &&
+		   (!rdesc->result_data.error_code)))
 		return 0;
 
-	if (rdesc->result_data.error_code & 0x407f) {
-		/* Fatal error (bits 0-7, 14) */
+	if (rdesc->descriptor_overflow)
+		dev_err(priv->dev, "Descriptor overflow detected");
+
+	if (rdesc->buffer_overflow)
+		dev_err(priv->dev, "Buffer overflow detected");
+
+	if (rdesc->result_data.error_code & 0x4067) {
+		/* Fatal error (bits 0,1,2,5,6 & 14) */
 		dev_err(priv->dev,
-			"cipher: result: result descriptor error (0x%x)\n",
+			"result descriptor error (%x)",
 			rdesc->result_data.error_code);
+		return -EIO;
+	} else if (rdesc->result_data.error_code &
+		   (BIT(7) | BIT(4) | BIT(3))) {
+		/*
+		 * Give priority over authentication fails:
+		 * Blocksize & overflow errors, something wrong with the input!
+		 */
 		return -EINVAL;
-	} else if (rdesc->result_data.error_code == BIT(9)) {
+	} else if (rdesc->result_data.error_code & BIT(9)) {
 		/* Authentication failed */
 		return -EBADMSG;
 	}