| Message ID | 20210204214359.1993065-6-thara.gopinath@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Regression fixes/clean ups in the Qualcomm crypto engine driver | expand |
On Thu, Feb 04, 2021 at 04:43:53PM -0500, Thara Gopinath wrote: > Crypto engine BAM dma does not support 0 length data. Return unsupported > if zero length messages are passed for transformation. > > Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org> > --- > drivers/crypto/qce/skcipher.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/crypto/qce/skcipher.c b/drivers/crypto/qce/skcipher.c > index de1f37ed4ee6..331b3c3a5b59 100644 > --- a/drivers/crypto/qce/skcipher.c > +++ b/drivers/crypto/qce/skcipher.c > @@ -8,6 +8,7 @@ > #include <linux/interrupt.h> > #include <linux/moduleparam.h> > #include <linux/types.h> > +#include <linux/errno.h> > #include <crypto/aes.h> > #include <crypto/internal/des.h> > #include <crypto/internal/skcipher.h> > @@ -260,6 +261,10 @@ static int qce_skcipher_crypt(struct skcipher_request *req, int encrypt) > rctx->flags |= encrypt ? QCE_ENCRYPT : QCE_DECRYPT; > keylen = IS_XTS(rctx->flags) ? ctx->enc_keylen >> 1 : ctx->enc_keylen; > > + /* CE does not handle 0 length messages */ > + if (!req->cryptlen) > + return -EOPNOTSUPP; > + For the algorithms in question, the correct behavior is to return 0. Aren't the tests catching that difference? - Eric
Hi Eric, On 2/4/21 5:48 PM, Eric Biggers wrote: > On Thu, Feb 04, 2021 at 04:43:53PM -0500, Thara Gopinath wrote: >> Crypto engine BAM dma does not support 0 length data. Return unsupported >> if zero length messages are passed for transformation. >> >> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org> >> --- >> drivers/crypto/qce/skcipher.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/drivers/crypto/qce/skcipher.c b/drivers/crypto/qce/skcipher.c >> index de1f37ed4ee6..331b3c3a5b59 100644 >> --- a/drivers/crypto/qce/skcipher.c >> +++ b/drivers/crypto/qce/skcipher.c >> @@ -8,6 +8,7 @@ >> #include <linux/interrupt.h> >> #include <linux/moduleparam.h> >> #include <linux/types.h> >> +#include <linux/errno.h> >> #include <crypto/aes.h> >> #include <crypto/internal/des.h> >> #include <crypto/internal/skcipher.h> >> @@ -260,6 +261,10 @@ static int qce_skcipher_crypt(struct skcipher_request *req, int encrypt) >> rctx->flags |= encrypt ? QCE_ENCRYPT : QCE_DECRYPT; >> keylen = IS_XTS(rctx->flags) ? ctx->enc_keylen >> 1 : ctx->enc_keylen; >> >> + /* CE does not handle 0 length messages */ >> + if (!req->cryptlen) >> + return -EOPNOTSUPP; >> + > > For the algorithms in question, the correct behavior is to return 0. What do you mean? The driver should return a 0 ? > > Aren't the tests catching that difference? I was anyways planning on sending an email to the list with these queries. But since you asked, these are my observations with fuzz testing which I have been doing quite a bit now (I am also working on adding a few qualcomm AEAD algorithms support in mainline). - if the generic algorithm supports 0 length messages and the transformation I am testing does not, the test framework throws an error and stops. - key support mismatch between the generic algorithm vs my algorithm /engine also does the same thing.For eg, Qualcomm CE engine does not support any three keys being same for triple des algorithms. Where as a two key 3des is a valid scenario for generic algorithm(k1=k3). Another example is hardware engine not supporting AES192. How are these scenarios usually handled ? Why not allow the test framework to proceed with the testing if the algorithm does not support a particular scenario ? > > - Eric > -- Warm Regards Thara
On Thu, Feb 04, 2021 at 07:09:53PM -0500, Thara Gopinath wrote: > > > @@ -260,6 +261,10 @@ static int qce_skcipher_crypt(struct skcipher_request *req, int encrypt) > > > rctx->flags |= encrypt ? QCE_ENCRYPT : QCE_DECRYPT; > > > keylen = IS_XTS(rctx->flags) ? ctx->enc_keylen >> 1 : ctx->enc_keylen; > > > + /* CE does not handle 0 length messages */ > > > + if (!req->cryptlen) > > > + return -EOPNOTSUPP; > > > + > > > > For the algorithms in question, the correct behavior is to return 0. > > What do you mean? The driver should return a 0 ? Yes, there is nothing to do for empty inputs, so just return 0 (success). > > Aren't the tests catching that difference? > > I was anyways planning on sending an email to the list with these queries. > But since you asked, these are my observations with fuzz testing which I > have been doing quite a bit now (I am also working on adding a few qualcomm > AEAD algorithms support in mainline). > > - if the generic algorithm supports 0 length messages and the transformation > I am testing does not, the test framework throws an error and stops. > - key support mismatch between the generic algorithm vs my algorithm /engine > also does the same thing.For eg, Qualcomm CE engine does not support any > three keys being same for triple des algorithms. Where as a two key 3des is > a valid scenario for generic algorithm(k1=k3). Another example is hardware > engine not supporting AES192. > > How are these scenarios usually handled ? Why not allow the test framework > to proceed with the testing if the algorithm does not support a particular > scenario ? Omitting support for certain inputs isn't allowed. Anyone in the kernel who wants to use a particular algorithm could get this driver for it, and if they happen to use inputs which the driver decided not to support, things will break. The way that drivers handle this is to use a fallback cipher for inputs they don't support. - Eric
On 2/4/21 7:26 PM, Eric Biggers wrote: > On Thu, Feb 04, 2021 at 07:09:53PM -0500, Thara Gopinath wrote: >>>> @@ -260,6 +261,10 @@ static int qce_skcipher_crypt(struct skcipher_request *req, int encrypt) >>>> rctx->flags |= encrypt ? QCE_ENCRYPT : QCE_DECRYPT; >>>> keylen = IS_XTS(rctx->flags) ? ctx->enc_keylen >> 1 : ctx->enc_keylen; >>>> + /* CE does not handle 0 length messages */ >>>> + if (!req->cryptlen) >>>> + return -EOPNOTSUPP; >>>> + >>> >>> For the algorithms in question, the correct behavior is to return 0. >> >> What do you mean? The driver should return a 0 ? Ok. I will re-spin the series once more with this change.. > > Yes, there is nothing to do for empty inputs, so just return 0 (success). > >>> Aren't the tests catching that difference? >> >> I was anyways planning on sending an email to the list with these queries. >> But since you asked, these are my observations with fuzz testing which I >> have been doing quite a bit now (I am also working on adding a few qualcomm >> AEAD algorithms support in mainline). >> >> - if the generic algorithm supports 0 length messages and the transformation >> I am testing does not, the test framework throws an error and stops. >> - key support mismatch between the generic algorithm vs my algorithm /engine >> also does the same thing.For eg, Qualcomm CE engine does not support any >> three keys being same for triple des algorithms. Where as a two key 3des is >> a valid scenario for generic algorithm(k1=k3). Another example is hardware >> engine not supporting AES192. >> >> How are these scenarios usually handled ? Why not allow the test framework >> to proceed with the testing if the algorithm does not support a particular >> scenario ? > > Omitting support for certain inputs isn't allowed. Anyone in the kernel who > wants to use a particular algorithm could get this driver for it, and if they > happen to use inputs which the driver decided not to support, things will break. Ya sounds reasonable. > > The way that drivers handle this is to use a fallback cipher for inputs they > don't support. Ok. So I will add this to my todo and make sure to have fallback ciphers for all the non-supported inputs. I will send this as a separate series and not this one. In this case, though not supporting 0 length messages for encryption is valid. I don't think I have to have a fallback for this. I could have sworn that the test framework throws up an error for this. But I have been testing a lot and may be I am just confused. I will double check this. > > - Eric > -- Warm Regards Thara
diff --git a/drivers/crypto/qce/skcipher.c b/drivers/crypto/qce/skcipher.c index de1f37ed4ee6..331b3c3a5b59 100644 --- a/drivers/crypto/qce/skcipher.c +++ b/drivers/crypto/qce/skcipher.c @@ -8,6 +8,7 @@ #include <linux/interrupt.h> #include <linux/moduleparam.h> #include <linux/types.h> +#include <linux/errno.h> #include <crypto/aes.h> #include <crypto/internal/des.h> #include <crypto/internal/skcipher.h> @@ -260,6 +261,10 @@ static int qce_skcipher_crypt(struct skcipher_request *req, int encrypt) rctx->flags |= encrypt ? QCE_ENCRYPT : QCE_DECRYPT; keylen = IS_XTS(rctx->flags) ? ctx->enc_keylen >> 1 : ctx->enc_keylen; + /* CE does not handle 0 length messages */ + if (!req->cryptlen) + return -EOPNOTSUPP; + /* qce is hanging when AES-XTS request len > QCE_SECTOR_SIZE and * is not a multiple of it; pass such requests to the fallback */
Crypto engine BAM dma does not support 0 length data. Return unsupported if zero length messages are passed for transformation. Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org> --- drivers/crypto/qce/skcipher.c | 5 +++++ 1 file changed, 5 insertions(+) -- 2.25.1