Re: [PATCH v2] drivers/crypto: Mark QCE as BROKEN

[Demi Marie Obenour <demiobenour@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 6144 bytes --]

On 7/22/26 04:33, Bartosz Golaszewski wrote:
> On Tue, 21 Jul 2026 20:29:39 +0200, Demi Marie Obenour
> <demiobenour@gmail.com> said:
>> On 7/21/26 04:24, Bartosz Golaszewski wrote:
>>> On Fri, Jul 17, 2026 at 06:18:48PM +1000, Herbert Xu wrote:
>>>> On Sun, Jul 12, 2026 at 05:31:31PM -0400, Demi Marie Obenour via B4 Relay wrote:
>>>>> From: Demi Marie Obenour <demiobenour@gmail.com>
>>>>>
>>>>> This driver is harmful:
>>>>>
>>>>> - It is much slower than the CPU [1] [2].
>>>>> - It Has a history of bugs [2] [3].
>>>>> - It does not have exclusive access to the hardware [4], causing races
>>>>>   with the secure world.
>>>>> - It register its implementations with too low a cra_priority for them
>>>>>   to be actually used [5].
>>>>>
>>>>> Therefore, disable it to ensure that nobody builds it into kernels they
>>>>> intend to ship.
>>>>>
>>>>> In the future, the driver will be used for processing restricted media
>>>>> content.  However, the kernel does not currently support this.  Since
>>>>> the driver will have future uses, allow building it if COMPILE_TEST is
>>>>> enabled.
>>>>>
>>>>> [1]: https://lore.kernel.org/r/20250704070322.20692-1-ebiggers@kernel.org/
>>>>> [2]: https://lore.kernel.org/r/20250615031807.GA81869@sol/
>>>>> [3]: https://lore.kernel.org/r/20260706-qce-fix-self-tests-v5-0-86f461ff1829@oss.qualcomm.com/
>>>>> [4]: https://lore.kernel.org/r/20260629-qcom-qce-cmd-descr-v20-0-56f67da84c05@oss.qualcomm.com/
>>>>> [5]: https://lore.kernel.org/r/20260524204537.GB110177@quark/
>>>>>
>>>>> Fixes: ec8f5d8f6f76 ("crypto: qce - Qualcomm crypto engine driver")
>>>>> Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>
>>>>> ---
>>>>> Changes in v2:
>>>>> - Add Fixes: ec8f5d8f6f76 ("crypto: qce - Qualcomm crypto engine driver")
>>>>> - Add Cc: stable@vger.kernel.org
>>>>> - Link to v1: https://lore.kernel.org/r/20260712-qce-broken-v1-1-85e2bff17871@gmail.com
>>>>> ---
>>>>>  arch/arm/configs/multi_v7_defconfig | 1 -
>>>>>  arch/arm64/configs/defconfig        | 1 -
>>>>>  drivers/crypto/Kconfig              | 6 +++++-
>>>>>  3 files changed, 5 insertions(+), 3 deletions(-)
>>>>
>>>> Patch applied.  Thanks.
>>>
>>> Hi Herbert,
>>>
>>> I'd like to ask you to back this patch out. Admittedly I should have NAKed this
>>> under the v1 thread but I thought the discussion we had under the Shikra series
>>> and the subsequent patch making myself the maintainer of this driver (note: Demi
>>> did not Cc me on this nor the Qcom mailing list at linux-arm-msm) as well as the
>>> pending series[1] fixing the QCE issues was enough to imply we should not mark
>>> it as broken. Please consider applying [1] if the fixes in v6 are correct.
>>>
>>> Bartosz
>>>
>>> [1] https://lore.kernel.org/all/20260717-qce-fix-self-tests-v6-0-455775fe5f6c@oss.qualcomm.com/
>>
>> Is the BAM locking patchset also needed?  My understanding is that
>> without it, the QCE driver can still produce wrong results due to races
>> with the TrustZone firmware.  Until it is merged and the self-tests
>> are fixed, the driver definitely needs to be marked BROKEN.
>>
> 
> No, we currently don't ship TZ firmware that could cause such interactions.
> The only driver that currently suffers from such races is the NAND driver
> which races with the modem firmware and it too will use the BAM locking
> feature once it's upstream. However we do plan to support DRM using this IP
> and this is when it'll need to account for possible races. IOW: BAM locking
> is not strictly required to fix the QCE.

Thanks for the correction.

Will updated firmware alone cause races, or does the kernel need to
opt-in to firmware features that can cause them?  I want to make sure
that a firmware update will not cause the driver to start racing with
the secure world.

(Nit: DRM usually stands for "Direct Rendering Manager" in Linux.)

>> Also, what can the current driver do that cannot be done better using
>> software crypto?  I understand that you have future plans to add more
>> features to the driver, but those are not relevant to this discussion.
>>
> 
> We can offload crypto operations to the QCE saving CPU cycles but more
> importantly, the QCE provides HW based isolation which some vendors care
> about.

Would you mind explaining what you mean by "HW based isolation"?
While some crypto engines support keys that Linux doesn't have access
to, Linux only supports this on IBM mainframes.  Linux does support
using hardware-wrapped keys for inline storage encryption, but those
do not use this driver.
>> Finally, I very much appreciate the work you have done.  I don't
>> expect that the QCE driver will stay BROKEN forever.  This patch is
>> purely a reflection of the current state of the driver, the speed of
>> CPU-accelerated crypto, and the small message sizes used by the kernel.
>>
> 
> That may have been true when you sent the v1 but with a set of proposed fixes
> it'll no longer be the case.

I trust (and have always trusted) that you can fix the driver.
I apologize for not CCing you on the patch submission.

>> That said, is drivers/crypto really the right place for a restricted
>> content driver?  A driver that uses dmabufs might be a better
>> alternative, and would be easier to integrate with the Direct Rendering
>> Manager (DRM) and Video for Linux 2 (V4L2) subsystems.
>> --
> 
> These are future plans, yes. But right now we do have active users of this
> IP and now also a maintainer - yours truly - who stepped up and is actively
> working on this module. With the fixes applied, the driver will again pass
> crypto self-tests and should not be BROKEN.

With your patch, does the driver also pass stress tests, such as
multiple threads using it in parallel with varying algorithms and
message sizes?

If so, then I agree that the driver should no longer depend on
BROKEN, but I still question whether it is useful in its current form.
That's not specific to QCE, though.  I would say the same about many
other drivers.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]