Re: [Bug report] kernel BUG at include/linux/scatterlist.h

[Lukas Wunner <lukas@wunner.de>]

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