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

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: <Z0RRiq-BJp8CYdNk@wunner.de>
Date: Mon, 25 Nov 2024 11:29:30 +0100
From: Lukas Wunner <lukas@...ner.de>
To: Herbert Xu <herbert@...dor.apana.org.au>
Cc: Ard Biesheuvel <ardb@...nel.org>, Zorro Lang <zlang@...hat.com>,
	linux-crypto@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [Bug report] kernel BUG at include/linux/scatterlist.h

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