linux-kernel - Re: [PATCH] crypto: virtio-crypto: call finalize with bh disabled

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230926184158.4ca2c0c3.pasic@linux.ibm.com>
Date:   Tue, 26 Sep 2023 18:41:58 +0200
From:   Halil Pasic <pasic@...ux.ibm.com>
To:     "Gonglei (Arei)" <arei.gonglei@...wei.com>
Cc:     Herbert Xu <herbert@...dor.apana.org.au>,
        "linux-crypto@...r.kernel.org" <linux-crypto@...r.kernel.org>,
        Marc Hartmayer <mhartmay@...ux.ibm.com>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        Jason Wang <jasowang@...hat.com>,
        "virtualization@...ts.linux-foundation.org" 
        <virtualization@...ts.linux-foundation.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "pizhenwei@...edance.com" <pizhenwei@...edance.com>,
        Halil Pasic <pasic@...ux.ibm.com>,
        Cornelia Huck <cohuck@...hat.com>
Subject: Re: [PATCH] crypto: virtio-crypto: call finalize with bh disabled

[..]
> --- a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> +++ b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
> @@ -61,8 +61,9 @@ static void virtio_crypto_akcipher_finalize_req(
>  	vc_akcipher_req->src_buf = NULL;
>  	vc_akcipher_req->dst_buf = NULL;
>  	virtcrypto_clear_request(&vc_akcipher_req->base);
> -
> +	local_bh_disable();
>  	crypto_finalize_akcipher_request(vc_akcipher_req->base.dataq->engine, req, err);
> +	local_bh_enable();

Thanks Gonglei!

I did this a quick spin, and it does not seem to be sufficient on s390x.
Which does not come as a surprise to me, because 

#define lockdep_assert_in_softirq()                                     \
do {                                                                    \
        WARN_ON_ONCE(__lockdep_enabled                  &&              \
                     (!in_softirq() || in_irq() || in_nmi()));          \
} while (0)

will still warn because  in_irq() still evaluates to true (your patch
addresses the !in_softirq() part).

I don't have any results on x86 yet. My current understanding is that the
virtio-pci transport code disables interrupts locally somewhere in the
call chain (actually in vp_vring_interrupt() via spin_lock_irqsave())
and then x86 would be fine. But I will get that verified.

On the other hand virtio_airq_handler() calls vring_interrupt() with
interrupts enabled. (While vring_interrupt() is called in a (read)
critical section in virtio_airq_handler() we use read_lock() and
not read_lock_irqsave() to grab the lock. Whether that is correct in
it self (i.e. disregarding the crypto problem) or not I'm not sure right
now. Will think some more about it tomorrow.) If the way to go forward
is disabling interrupts in virtio-ccw before vring_interrupt() is
called, I would be glad to spin a patch for that.

Copying Conny, as she may have an opinion on this (if I'm not wrong she
authored that code).

Regards,
Halil