linux-kernel - RE: PING: [PATCH v2 3/3] virtio-crypto: implement RSA algorithm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Tue, 1 Mar 2022 11:13:02 +0000
From:   "Gonglei (Arei)" <arei.gonglei@...wei.com>
To:     zhenwei pi <pizhenwei@...edance.com>
CC:     "jasowang@...hat.com" <jasowang@...hat.com>,
        "mst@...hat.com" <mst@...hat.com>,
        "virtualization@...ts.linux-foundation.org" 
        <virtualization@...ts.linux-foundation.org>,
        "linux-crypto@...r.kernel.org" <linux-crypto@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "helei.sig11@...edance.com" <helei.sig11@...edance.com>,
        "herbert@...dor.apana.org.au" <herbert@...dor.apana.org.au>,
        kernel test robot <lkp@...el.com>
Subject: RE: PING: [PATCH v2 3/3] virtio-crypto: implement RSA algorithm



> -----Original Message-----
> From: zhenwei pi [mailto:pizhenwei@...edance.com]
> Sent: Tuesday, March 1, 2022 6:26 PM
> To: Gonglei (Arei) <arei.gonglei@...wei.com>
> Cc: jasowang@...hat.com; mst@...hat.com;
> virtualization@...ts.linux-foundation.org; linux-crypto@...r.kernel.org;
> linux-kernel@...r.kernel.org; helei.sig11@...edance.com;
> herbert@...dor.apana.org.au; kernel test robot <lkp@...el.com>
> Subject: PING: [PATCH v2 3/3] virtio-crypto: implement RSA algorithm
> 
> PING!
> 
> Hi, Lei
> I also take a look at other crypto drivers qat/ccp/hisilicon, they separate
> akcipher/skcipher algo. If you consider that reusing
> virtio_crypto_algs_register/unregister seems better, I will try to merge them
> into a single function.
> 

I'm fine with separating them in different c files. Then should we rename virtio_crypto_algs.c
to virtio_crypto_skcipher_algo.c?


Regards,
-Gonglei

> On 2/23/22 6:17 PM, zhenwei pi wrote:
> >
> > On 2/18/22 11:12 AM, zhenwei pi wrote:
> >>>> +void virtio_crypto_akcipher_algs_unregister(struct virtio_crypto
> >>>> +*vcrypto) {
> >>>> +    int i = 0;
> >>>> +
> >>>> +    mutex_lock(&algs_lock);
> >>>> +
> >>>> +    for (i = 0; i < ARRAY_SIZE(virtio_crypto_akcipher_algs); i++)
> >>>> +{
> >>>> +        uint32_t service = virtio_crypto_akcipher_algs[i].service;
> >>>> +        uint32_t algonum = virtio_crypto_akcipher_algs[i].algonum;
> >>>> +
> >>>> +        if (virtio_crypto_akcipher_algs[i].active_devs == 0 ||
> >>>> +            !virtcrypto_algo_is_supported(vcrypto, service,
> >>>> +algonum))
> >>>> +            continue;
> >>>> +
> >>>> +        if (virtio_crypto_akcipher_algs[i].active_devs == 1)
> >>>> +
> >>>>
> >>>> crypto_unregister_akcipher(&virtio_crypto_akcipher_algs[i].algo);
> >>>> +
> >>>> +        virtio_crypto_akcipher_algs[i].active_devs--;
> >>>> +    }
> >>>> +
> >>>> +    mutex_unlock(&algs_lock);
> >>>> +}
> >>>
> >>> Why don't you reuse the virtio_crypto_algs_register/unregister
> >>> functions?
> >>> The current code is too repetitive. Maybe we don't need create the
> >>> new file virtio_crypto_akcipher_algo.c because we had
> >>> virtio_crypto_algs.c which includes all algorithms.
> >>>
> >>
> >> Yes, this looks similar to virtio_crypto_algs_register/unregister.
> >>
> >> Let's look at the difference:
> >> struct virtio_crypto_akcipher_algo {
> >>          uint32_t algonum;
> >>          uint32_t service;
> >>          unsigned int active_devs;
> >>          struct akcipher_alg algo;
> >> };
> >>
> >> struct virtio_crypto_algo {
> >>          uint32_t algonum;
> >>          uint32_t service;
> >>          unsigned int active_devs;
> >>          struct skcipher_alg algo; /* akcipher_alg VS skcipher_alg */
> >> };
> >>
> >> If reusing virtio_crypto_algs_register/unregister, we need to modify
> >> the data structure like this:
> >> struct virtio_crypto_akcipher_algo {
> >>          uint32_t algonum;
> >>          uint32_t service;    /* use service to distinguish
> >> akcipher/skcipher */
> >>          unsigned int active_devs;
> >>      union {
> >>          struct skcipher_alg skcipher;
> >>              struct akcipher_alg akcipher;
> >>      } alg;
> >> };
> >>
> >> int virtio_crypto_akcipher_algs_register(struct virtio_crypto
> >> *vcrypto) {
> >>      ...
> >>          for (i = 0; i < ARRAY_SIZE(virtio_crypto_akcipher_algs);
> >> i++) {
> >>                  uint32_t service =
> >> virtio_crypto_akcipher_algs[i].service;
> >>          ...
> >>          /* test service type then call
> >> crypto_register_akcipher/crypto_register_skcipher */
> >>                  if (service == VIRTIO_CRYPTO_SERVICE_AKCIPHER)
> >> crypto_register_akcipher(&virtio_crypto_akcipher_algs[i].algo.akciphe
> >> r);
> >>          else
> >> crypto_register_skcipher(&virtio_crypto_skcipher_algs[i].algo.skciphe
> >> r);
> >>          ...
> >>          }
> >>      ...
> >> }
> >>
> >> Also test service type and call
> >> crypto_unregister_skcipher/crypto_unregister_akcipher.
> >>
> >> This gets unclear from current v2 version.
> >>
> >> On the other hand, the kernel side prefers to separate skcipher and
> >> akcipher(separated header files and implementations).
> >>
> >
> 
> --
> zhenwei pi