Re: [PATCH RESEND] hwrng: core - don't pass stack allocated buffer to rng->read()

[Laszlo Ersek]

On 10/21/16 23:17, Richard W.M. Jones wrote:
> On Fri, Oct 21, 2016 at 02:04:27PM -0700, Andy Lutomirski wrote:
>> https://git.kernel.org/cgit/linux/kernel/git/herbert/cryptodev-2.6.git/commit/?id=6d4952d9d9d4dc2bb9c0255d95a09405a1e958f7
> 
> I have tested this one, and it also fixes the bug I was seeing.
> 
> Thanks Laszlo as well for his fix, and sorry for not finding the
> patch above first.

No problem, it was fun :)

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RESEND] hwrng: core - don't pass stack allocated buffer to rng->read()

[Laszlo Ersek]

On 10/21/16 23:04, Andy Lutomirski wrote:
> On Fri, Oct 21, 2016 at 1:48 PM, Laszlo Ersek  wrote:
>> The virtio-rng backend for hwrng passes the buffer that it receives for
>> filling to sg_set_buf() directly, in:
>>
>> virtio_read()   [drivers/char/hw_random/virtio-rng.c]
>>   register_buffer() [drivers/char/hw_random/virtio-rng.c]
>> sg_init_one()   [lib/scatterlist.c]
>>   sg_set_buf()  [include/linux/scatterlist.h]
>>
>> In turn, the sg_set_buf() function, when built with CONFIG_DEBUG_SG,
>> actively enforces (justifiedly) that the buffer used within the
>> scatter-gather list live in physically contiguous memory:
>>
>>   BUG_ON(!virt_addr_valid(buf));
>>
>> The combination of the above two facts means that whatever calls
>> virtio_read() -- via the hwrng.read() method -- has to allocate the
>> recipient buffer in physically contiguous memory.
> 
> Indeed.  This bug should be fixed by:
> 
> https://git.kernel.org/cgit/linux/kernel/git/herbert/cryptodev-2.6.git/commit/?id=6d4952d9d9d4dc2bb9c0255d95a09405a1e958f7
> 

Cool, thanks!

(My commit message is better tho ;))

Cheers
Laszlo
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RESEND] hwrng: core - don't pass stack allocated buffer to rng->read()

[Richard W.M. Jones]

On Fri, Oct 21, 2016 at 02:04:27PM -0700, Andy Lutomirski wrote:
> https://git.kernel.org/cgit/linux/kernel/git/herbert/cryptodev-2.6.git/commit/?id=6d4952d9d9d4dc2bb9c0255d95a09405a1e958f7

I have tested this one, and it also fixes the bug I was seeing.

Thanks Laszlo as well for his fix, and sorry for not finding the
patch above first.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RESEND] hwrng: core - don't pass stack allocated buffer to rng->read()

[Andy Lutomirski]

On Fri, Oct 21, 2016 at 1:48 PM, Laszlo Ersek  wrote:
> The virtio-rng backend for hwrng passes the buffer that it receives for
> filling to sg_set_buf() directly, in:
>
> virtio_read()   [drivers/char/hw_random/virtio-rng.c]
>   register_buffer() [drivers/char/hw_random/virtio-rng.c]
> sg_init_one()   [lib/scatterlist.c]
>   sg_set_buf()  [include/linux/scatterlist.h]
>
> In turn, the sg_set_buf() function, when built with CONFIG_DEBUG_SG,
> actively enforces (justifiedly) that the buffer used within the
> scatter-gather list live in physically contiguous memory:
>
>   BUG_ON(!virt_addr_valid(buf));
>
> The combination of the above two facts means that whatever calls
> virtio_read() -- via the hwrng.read() method -- has to allocate the
> recipient buffer in physically contiguous memory.

Indeed.  This bug should be fixed by:

https://git.kernel.org/cgit/linux/kernel/git/herbert/cryptodev-2.6.git/commit/?id=6d4952d9d9d4dc2bb9c0255d95a09405a1e958f7
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH RESEND] hwrng: core - don't pass stack allocated buffer to rng->read()

[Laszlo Ersek]

The virtio-rng backend for hwrng passes the buffer that it receives for
filling to sg_set_buf() directly, in:

virtio_read()   [drivers/char/hw_random/virtio-rng.c]
  register_buffer() [drivers/char/hw_random/virtio-rng.c]
sg_init_one()   [lib/scatterlist.c]
  sg_set_buf()  [include/linux/scatterlist.h]

In turn, the sg_set_buf() function, when built with CONFIG_DEBUG_SG,
actively enforces (justifiedly) that the buffer used within the
scatter-gather list live in physically contiguous memory:

  BUG_ON(!virt_addr_valid(buf));

The combination of the above two facts means that whatever calls
virtio_read() -- via the hwrng.read() method -- has to allocate the
recipient buffer in physically contiguous memory.

Although this ends up being a generic interface restriction that is not
documented at the abstract hwrng level ("include/linux/hw_random.h",
"Documentation/hw_random.txt"), the virtio-rng provider has not been
changed to implement bounce buffering. Instead, existing core commits have
accommodated the silent restriction, such as:

- f7f154f1246c hw_random: make buffer usable in scatterlist.

  which would allocate "rng_buffer" with kmalloc(), and

- be4000bc4644 hwrng: create filler thread

  which would allocate the new "rng_fillbuf" similarly.

One call site remains that breaks the silent restriction: the
add_early_randomness() function passes an on-stack array to hwrng.read(),
via rng_get_data(), resulting in the following (valid) BUG, when
CONFIG_DEBUG_SG is enabled:

> [ cut here ]
> kernel BUG at ./include/linux/scatterlist.h:140!
> invalid opcode:  [#1] SMP
> Modules linked in: virtio_pci(+) virtio_mmio virtio_input virtio_balloon
> virtio_scsi nd_pmem nd_btt virtio_net virtio_console virtio_rng
> virtio_blk virtio_ring virtio nfit crc32_generic crct10dif_pclmul
> crc32c_intel crc32_pclmul
> CPU: 0 PID: 1 Comm: init Not tainted 4.9.0-0.rc0.git6.2.fc26.x86_64 #1
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.9.3-1.fc26
> 04/01/2014
> task: 91f29de53240 task.stack: b82cc000
> RIP: 0010:[]  []
> sg_init_one+0x8c/0xa0
> RSP: 0018:b82cf7d0  EFLAGS: 00010246
> RAX:  RBX: b82cf858 RCX: 0028
> RDX: 262d800cf858 RSI: 0026 RDI: b820800cf858
> RBP: b82cf7e8 R08: 006a R09: b82cf7f8
> R10:  R11:  R12: 0010
> R13: b82cf7f8 R14: 0010 R15: 
> FS:  7fffd6e6e140() GS:91f29ee0()
> knlGS:
> CS:  0010 DS:  ES:  CR0: 80050033
> CR2: 7fc67e24e000 CR3: 1bdad000 CR4: 000406f0
> Stack:
>  91f29be3b400 0001 b82cf858 b82cf848
>  c0056226 87654321 0002 
>    2a14e409 91f29be3b400
> Call Trace:
>  [] virtio_read+0xc6/0x110 [virtio_rng]
>  [] add_early_randomness+0x5e/0xd0
>  [] set_current_rng+0x45/0x160
>  [] hwrng_register+0xf7/0x130
>  [] virtrng_scan+0x19/0x30 [virtio_rng]
>  [] virtio_dev_probe+0x198/0x1e0 [virtio]
>  [] driver_probe_device+0x223/0x430
>  [] __device_attach_driver+0x8c/0x100
>  [] ? __driver_attach+0xf0/0xf0
>  [] bus_for_each_drv+0x6a/0xb0
>  [] __device_attach+0xe2/0x160
>  [] device_initial_probe+0x13/0x20
>  [] bus_probe_device+0xa3/0xb0
>  [] device_add+0x382/0x650
>  [] ? vp_modern_find_vqs+0x70/0x70 [virtio_pci]
>  [] ? vp_modern_find_vqs+0x70/0x70 [virtio_pci]
>  [] device_register+0x1a/0x20
>  [] register_virtio_device+0xb9/0x100 [virtio]
>  [] virtio_pci_probe+0xc3/0x140 [virtio_pci]
>  [] local_pci_probe+0x45/0xa0
>  [] ? pci_match_device+0xca/0x110
>  [] pci_device_probe+0x103/0x150
>  [] driver_probe_device+0x223/0x430
>  [] __driver_attach+0xe3/0xf0
>  [] ? driver_probe_device+0x430/0x430
>  [] bus_for_each_dev+0x73/0xc0
>  [] driver_attach+0x1e/0x20
>  [] bus_add_driver+0x173/0x270
>  [] ? 0xc0099000
>  [] driver_register+0x60/0xe0
>  [] ? 0xc0099000
>  [] __pci_register_driver+0x60/0x70
>  [] virtio_pci_driver_init+0x1e/0x1000 [virtio_pci]
>  [] do_one_initcall+0x50/0x180
>  [] ? rcu_read_lock_sched_held+0x45/0x80
>  [] ? kmem_cache_alloc_trace+0x277/0x2d0
>  [] ? do_init_module+0x27/0x1f1
>  [] do_init_module+0x5f/0x1f1
>  [] load_module+0x2401/0x2b40
>  [] ? __symbol_put+0x70/0x70
>  [] ? sched_clock_cpu+0x90/0xc0
>  [] ? __might_fault+0x43/0xa0
>  [] SYSC_init_module+0x19b/0x1c0
>  [] SyS_init_module+0xe/0x10
>  [] entry_SYSCALL_64_fastpath+0x1f/0xc2
> Code: ca 75 2c 49 8b 55 08 f6 c2 01 75 25 83 e2 03 81 e3 ff 0f 00 00 45
> 89 65 14 48 09 d0 41 89 5d 10 49 89 45 08 5b 41 5c 41 5d 5d c3 <0f> 0b
> 0f 0b 0f 0b 0f 0b 48 8b 15 05 ec 98 00 eb a3 0f 1f 00 55
> RIP  [] sg_init_one+0x8c/0xa0
>  RSP 
> ---[ end trace 8120a17353b469c4 ]---

Prevent this by allocating a temporary buffer in add_early_randomness()
with kmalloc(). (The function