On Mon, Nov 25, 2024 at 07:13:18AM +0800, Herbert Xu wrote:
> On Sun, Nov 24, 2024 at 05:24:12PM +0100, Lukas Wunner wrote:
> > Hm, my impression is that this needs to be fixed in arm64's
> > virt_addr_valid() macro.
>
> Regardless of what happens on arm64, you can't put a virtual
> address into an SG list in general. It's just not allowed.
The virtual address passed to sg_init_one() is converted to a
physical address with the following call chain:
sg_init_one()
sg_set_buf()
sg_set_page(sg, virt_to_page(buf), buflen, offset_in_page(buf))
... where virt_to_page() implicitly does the right thing for
kmalloc'ed addresses ("linear map") versus kernel image addresses
on arm64 (as on other arches):
virt_to_page()
pfn_to_page(virt_to_pfn())
__phys_to_pfn(virt_to_phys())
__virt_to_phys()
__virt_to_phys_nodebug()
__is_lm_address(__x) ? __lm_to_phys(__x) : __kimg_to_phys(__x)
So this all works fine and with the patch I proposed, all crypto
selftests pass in my qemu arm64 VM.
It's just that the virt_addr_valid() check in sg_set_buf() throws
a nonsensical false-positive BUG_ON() on arm64.
> In any case, we don't even need SG lists here since the correct
> interface to use in rsassa-pkcs1.c is crypto_akcipher_sync_encrypt.
crypto_akcipher_sync_encrypt() kmalloc's a buffer and copies from
the kernel's .rodata section to that buffer. That's why it doesn't
throw the false-positive BUG_ON() on arm64: virt_addr_valid() is
happy if the virtual address is in the linear map.
Nevertheless, crypto_akcipher_sync_encrypt() likewise passes a virtual
address to sg_init_one(), which is converted to a physical address
in the linear map as shown above.
I deliberately avoided the crypto_akcipher_sync_encrypt() API
in rsassa-pkcs1.c because the extra buffer allocation plus copying
data around impacts performance for no benefit.
There is a benefit of course in that the false-positive BUG_ON()
isn't triggered but that's an arm64 oddity that other major arches
do not exhibit and that should be fixed.
So if you absolutely positively want to use crypto_akcipher_sync_encrypt()
in rsassa-pkcs1.c, I can change that. But it will come at a performance
cost without apparent benefit. Are you sure (y/n)?
Thanks,
Lukas
