David, On Wed, Jan 16, 2019 at 09:27:19PM +0300, Vitaly Chikunov wrote: > On Wed, Jan 16, 2019 at 05:12:20PM +0000, David Howells wrote: > > Umm... What do I apply this patch to? > > This should go over "crypto: testmgr - split akcipher tests by a key type" > which I sent at 20190107 to linux-crypto. Sorry for the mess. > > > In your modified public_key_verify_signature(): > > > > > - sg_init_one(&digest_sg, output, outlen); > > > - akcipher_request_set_crypt(req, &sig_sg, &digest_sg, sig->s_size, > > > + sg_init_one(&output_sg, output, outlen); > > > + akcipher_request_set_crypt(req, &sig_sg, &output_sg, sig->s_size, > > > outlen); > > > > Why is the output necessary? It was there for the decoded hash to be placed > > in prior to comparison - but now that's not necessary. > > Agreed.
I prepared the patch which factors `output' into public_key_verify_signature(). Also, I try to elucidate my arguments below. > > > - ret = crypto_wait_req(crypto_akcipher_verify(req), &cwait); > > > + ret = crypto_wait_req(crypto_akcipher_verify(req, sig->digest, > > > + sig->digest_size), &cwait); > > > > I see sig->digest is passed in here. Should it be passed in in place of > > output_sg above? In short, it's passed as parameter to not clutter `struct akcipher_request' and to distinguish it from encrypt/decrypt scatterlists. New 'verify' op requires very different parameter set than encrypt, decrypt, sign. This difference is now most illustrative in testmgr (see in the next patch). > (I tried different approaches such as passing additional arguments to > `akcipher_request_set_crypt' or having additional setter like > `akcipher_request_set_aux' just to set value of digest and that should > be used just for verify op.) > > I thought passing input parameter in `struct akcipher_request' in the > field called 'dst' could be confusing for users. So this should be an > additional parameter in the request which is never used by any other caller. > Also, it was unknown if this should be scatterlist or not (I choose that > it should not). When it's separate argument to crypto_akcipher_verify() > call user is forced to set it, and there is no cluttering of `struct > akcipher_request' (which is designed to handle just encryption/decryption) > with not needed auxiliary parameters, and because it's very separated > from request parameters which all scatterlists and all other calls > arguments usually are not scatterlists, so this looked like similar > approach to what others do. > > > > - inst->alg.verify = pkcs1pad_verify; > > > + inst->alg.verify_rsa = pkcs1pad_verify; > > > > Is there a reason that pkcs1pad_verify() can't do the comparison? > > If you agree that we have two callbacks for a full and a partial > verification (I assume you do), why should pkcs1pad_verify do a full > verification if it's allowed to do just partial one, and it's RSA > cipher which have special partial verification designed for them. > > So I decided that pkcs1pad_verify should implement verify_rsa api only > and this is beneficial for having minimal code change and somewhat > backward compatible. > > > > - .verify = rsa_verify, > > > + .verify_rsa = rsa_verify, > > > > Likewise verify_rsa()? > > > > Granted, this might involve pkcs1pad_verify() dressing up the signature in > > the > > appropriate wrappings and passing it along to verify_rsa() to do the actual > > comparison there (ie. what pkcs1pad_verify_complete() does). a) RSA verify works differently (is it just disguised encrypt), b) We have separate wrapper module for it (pkcs1pad). Thus: Old API can not be removed. In other words, we can not replace .verify_rsa with .verify in these drivers or PKCS1 will not work. We can replace .verify_rsa with .verify in pkcs1pad, but there is no need for that if we stay with two API calls, which we can't avoid. > If we stay with the two api calls (verify and verify_rsa) pkcs1pad_verify > does not need to do any verification leaving it to the akcipher core. > > There is only potential "problem" that pkcs1pad code will not be able to > use other akciphers besides rsa family (implementing only verify_rsa), > but I assumed this is not needed (since only RSA is actually using > PKCS1) and maybe even beneficial restriction. > > > > - .verify = caam_rsa_enc, > > > + .verify_rsa = caam_rsa_enc, > > > > I presume this is the reason - because this reuses its encrypt operation > > directly. But could this instead perform the comparison upon completion, > > say > > in rsa_pub_done()? No, or pkcs1pad will not work. Thanks, > > > > > - .verify = qat_rsa_enc, > > > + .verify_rsa = qat_rsa_enc, > > > > Again, this could do the comparison, say, in qat_rsa_cb(). > > Abandoning idea with two api calls (full verify and partial verify_rsa) > will require me to modify code for all these drivers for devices that I > don't have and can't test. So, I choose approach with less new code. > > If you think that partial verify api should be completely removed that > change will require much bigger rework. Please tell if you would prefer > that. > > Thanks,
