David, 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. > > - 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? (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). 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()? > > > - .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,
