Laurent Vivier
2021-Sep-16 08:52 UTC
Use of uninitialized memory with CONFIG_HW_RANDOM_VIRTIO
On 13/09/2021 10:25, Alexander Potapenko wrote:> Hi Laurent, > > I took the latest kernel (5.15-rc1, > 6880fa6c56601bb8ed59df6c30fd390cc5f6dd8f) and a slightly modified > config from syzbot (see attached) > The latter has a lot of unnecessary debug checks, but those should not > affect the RNG. > > You then need to apply the following patch to the kernel: > > ===================================================> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c > index a3db27916256d..a4cba9f0ff8cb 100644 > --- a/drivers/char/hw_random/core.c > +++ b/drivers/char/hw_random/core.c > @@ -433,8 +433,11 @@ static int hwrng_fillfn(void *unused) > if (IS_ERR(rng) || !rng) > break; > mutex_lock(&reading_mutex); > + memset(rng_fillbuf, 'A', rng_buffer_size()); > + rng_fillbuf[rng_buffer_size()-1] = 0; > rc = rng_get_data(rng, rng_fillbuf, > rng_buffer_size(), 1); > + pr_err("rng_fillbuf: %s\n", rng_fillbuf); > mutex_unlock(&reading_mutex); > put_rng(rng); > if (rc <= 0) { > ===================================================> > and run the kernel under QEMU. > > On my machine I'm seeing the following output: > > $ cat log | strings | grep rng_fillbuf > [ 4.901931][ T897] rng_fillbuf: > AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA > [ 4.903104][ T897] rng_fillbuf: > > [ 4.903641][ T897] rng_fillbuf: > [ 4.904846][ T897] rng_fillbuf: ? > [ 4.913442][ T897] rng_fillbuf: [ > > , which denotes that the first call to rng_get_data() leaves > rng_fillbuf uninitialized.Thank you for the detailed steps. The problem happens because we mix two different buffers: - in add_early_randomness() we provide rng_buffer but don't wait it is full (see [1]) - in hwrng_fillfn() we provide rng_fillbuf, and we wait data here, but we received the signal from QEMU that there are data, but these data are in rng_buffer while we expect them in rng_fillbuf. There are several ways to fix/workaround that: 1- ignore the read when wait=0 : diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c index a90001e02bf7..8466d76566fd 100644 --- a/drivers/char/hw_random/virtio-rng.c +++ b/drivers/char/hw_random/virtio-rng.c @@ -59,15 +59,15 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait) if (vi->hwrng_removed) return -ENODEV; + if (!wait) + return 0; + if (!vi->busy) { vi->busy = true; reinit_completion(&vi->have_data); register_buffer(vi, buf, size); } - if (!wait) - return 0; - ret = wait_for_completion_killable(&vi->have_data); if (ret < 0) return ret; 2- Use an internal intermediate buffer in virtio-rng, at a cost of a copy, I have some patches (somewhere) I can refresh to do that. 3- modify hw_random/core.c to use only one buffer Thanks, Laurent [1] 78887832e765 ("hwrng: core - don't wait on add_early_randomness()")> > HTH, > Alex > > On Mon, Sep 13, 2021 at 8:52 AM Laurent Vivier <lvivier at redhat.com> wrote: >> >> Hi Alexander, >> >> On 12/09/2021 19:05, Alexander Potapenko wrote: >>> Hi Laurent, >>> >>> Do you by any chance have an update on this? >> >> I'm sorry I didn't have the time until now. >> >> I try today. >> >> Could you give more details how to reproduce this? >> (kernel version, .config, tools to run?) >> >> Thanks, >> Laurent >>> Thanks, >>> Alex >>> >>> On Fri, Aug 20, 2021 at 6:15 PM Laurent Vivier <lvivier at redhat.com> wrote: >>>> >>>> On 19/08/2021 22:58, Michael S. Tsirkin wrote: >>>>> On Fri, Nov 13, 2020 at 06:26:16PM +0100, Alexander Potapenko wrote: >>>>>> Hi Amos, Rusty, Amit, Michael, >>>>>> >>>>>> I am hitting something that I believe to be a minor problem in the >>>>>> virtio RNG driver. >>>>>> When running the kernel under KMSAN with "-device virtio-rng-pci" >>>>>> passed to QEMU, I am seeing reports about rng_fillbuf in >>>>>> drivers/char/hw_random/core.c being used before initialization (see >>>>>> the report below). >>>>>> >>>>>> This can be verified by initializing rng_fillbuf with 'A' as follows: >>>>>> =========================================>>>>>> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c >>>>>> index 8c1c47dd9f46..44d609a5796a 100644 >>>>>> --- a/drivers/char/hw_random/core.c >>>>>> +++ b/drivers/char/hw_random/core.c >>>>>> @@ -439,8 +439,11 @@ static int hwrng_fillfn(void *unused) >>>>>> if (IS_ERR(rng) || !rng) >>>>>> break; >>>>>> mutex_lock(&reading_mutex); >>>>>> + memset(rng_fillbuf, 'A', rng_buffer_size()); >>>>>> + rng_fillbuf[rng_buffer_size()-1] = 0; >>>>>> rc = rng_get_data(rng, rng_fillbuf, >>>>>> rng_buffer_size(), 1); >>>>>> + pr_err("rng_fillbuf: %s\n", rng_fillbuf); >>>>>> mutex_unlock(&reading_mutex); >>>>>> put_rng(rng); >>>>>> if (rc <= 0) { >>>>>> =========================================>>>>>> >>>>>> and booting the kernel: the first call of hwrng_fillfn() will print >>>>>> "AAAAAAA.." instead of random data. >>>>>> >>>>>> For some reason on that first iteration vi->busy is true here: >>>>>> https://elixir.bootlin.com/linux/latest/source/drivers/char/hw_random/virtio-rng.c#L62, >>>>>> therefore the buffer is not being sent to virtio ring. >>>>>> >>>>>> While probably being benign, this bug is preventing syzkaller from >>>>>> finding more bugs, so it would be nice to fix it. >>>>>> Perhaps the easiest solution is to kzalloc rng_fillbuf, but if it's >>>>>> critical for this driver to not skip even the first read, then maybe >>>>>> you have better ideas? >>>>>> >>>>>> KMSAN report follows: >>>>>> >>>>>> ====================================================>>>>>> BUG: KMSAN: uninit-value in _mix_pool_bytes+0x7d2/0x950 >>>>>> drivers/char/random.c:570 >>>>>> CPU: 0 PID: 2711 Comm: hwrng Not tainted 5.9.0-rc8-syzkaller #0 >>>>>> Hardware name: Google Google Compute Engine/Google Compute Engine, >>>>>> BIOS Google 01/01/2011 >>>>>> Call Trace: >>>>>> __dump_stack lib/dump_stack.c:77 [inline] >>>>>> dump_stack+0x21c/0x280 lib/dump_stack.c:118 >>>>>> kmsan_report+0xf7/0x1e0 mm/kmsan/kmsan_report.c:122 >>>>>> __msan_warning+0x5f/0xa0 mm/kmsan/kmsan_instr.c:201 >>>>>> _mix_pool_bytes+0x7d2/0x950 drivers/char/random.c:570 >>>>>> mix_pool_bytes+0xca/0x2a0 drivers/char/random.c:599 >>>>>> add_hwgenerator_randomness+0x4ac/0x500 drivers/char/random.c:2319 >>>>>> hwrng_fillfn+0x6ae/0x940 drivers/char/hw_random/core.c:452 >>>>>> kthread+0x51c/0x560 kernel/kthread.c:293 >>>>>> ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:294 >>>>>> >>>>>> Uninit was created at: >>>>>> kmsan_save_stack_with_flags mm/kmsan/kmsan.c:129 [inline] >>>>>> kmsan_internal_poison_shadow+0x5c/0xf0 mm/kmsan/kmsan.c:112 >>>>>> kmsan_slab_alloc+0x8d/0xe0 mm/kmsan/kmsan_hooks.c:80 >>>>>> slab_alloc_node mm/slub.c:2903 [inline] >>>>>> slab_alloc mm/slub.c:2912 [inline] >>>>>> kmem_cache_alloc_trace+0x61e/0xc90 mm/slub.c:2929 >>>>>> kmalloc include/linux/slab.h:554 [inline] >>>>>> hwrng_modinit+0x103/0x2ef drivers/char/hw_random/core.c:621 >>>>>> do_one_initcall+0x371/0x9c0 init/main.c:1208 >>>>>> do_initcall_level+0x1e5/0x3c6 init/main.c:1281 >>>>>> do_initcalls+0x127/0x1cb init/main.c:1297 >>>>>> do_basic_setup+0x33/0x36 init/main.c:1317 >>>>>> kernel_init_freeable+0x238/0x38b init/main.c:1517 >>>>>> kernel_init+0x1f/0x840 init/main.c:1406 >>>>>> ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:294 >>>>>> ====================================================>>>>>> >>>>>> Thanks, >>>>>> Alex >>>>> >>>>> >>>>> Cc Laurent - I think he said he was going to look at virtio rng. >>>> >>>> I will have look next week. >>>> >>>> Thanks, >>>> Laurent >>>> >>> >>> >> > >
Michael S. Tsirkin
2021-Sep-16 22:58 UTC
Use of uninitialized memory with CONFIG_HW_RANDOM_VIRTIO
On Thu, Sep 16, 2021 at 10:52:59AM +0200, Laurent Vivier wrote:> On 13/09/2021 10:25, Alexander Potapenko wrote: > > Hi Laurent, > > > > I took the latest kernel (5.15-rc1, > > 6880fa6c56601bb8ed59df6c30fd390cc5f6dd8f) and a slightly modified > > config from syzbot (see attached) > > The latter has a lot of unnecessary debug checks, but those should not > > affect the RNG. > > > > You then need to apply the following patch to the kernel: > > > > ===================================================> > diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c > > index a3db27916256d..a4cba9f0ff8cb 100644 > > --- a/drivers/char/hw_random/core.c > > +++ b/drivers/char/hw_random/core.c > > @@ -433,8 +433,11 @@ static int hwrng_fillfn(void *unused) > > if (IS_ERR(rng) || !rng) > > break; > > mutex_lock(&reading_mutex); > > + memset(rng_fillbuf, 'A', rng_buffer_size()); > > + rng_fillbuf[rng_buffer_size()-1] = 0; > > rc = rng_get_data(rng, rng_fillbuf, > > rng_buffer_size(), 1); > > + pr_err("rng_fillbuf: %s\n", rng_fillbuf); > > mutex_unlock(&reading_mutex); > > put_rng(rng); > > if (rc <= 0) { > > ===================================================> > > > and run the kernel under QEMU. > > > > On my machine I'm seeing the following output: > > > > $ cat log | strings | grep rng_fillbuf > > [ 4.901931][ T897] rng_fillbuf: > > AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA > > [ 4.903104][ T897] rng_fillbuf: > > > [ 4.903641][ T897] rng_fillbuf: > > [ 4.904846][ T897] rng_fillbuf: ? > > [ 4.913442][ T897] rng_fillbuf: [ > > > > , which denotes that the first call to rng_get_data() leaves > > rng_fillbuf uninitialized. > > > Thank you for the detailed steps. > > The problem happens because we mix two different buffers: > - in add_early_randomness() we provide rng_buffer but don't wait it is full (see [1]) > - in hwrng_fillfn() we provide rng_fillbuf, and we wait data here, but we > received the signal from QEMU that there are data, but these data are in > rng_buffer while we expect them in rng_fillbuf. > > There are several ways to fix/workaround that: > > 1- ignore the read when wait=0 : > > diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c > index a90001e02bf7..8466d76566fd 100644 > --- a/drivers/char/hw_random/virtio-rng.c > +++ b/drivers/char/hw_random/virtio-rng.c > @@ -59,15 +59,15 @@ static int virtio_read(struct hwrng *rng, void *buf, > size_t size, bool wait) > if (vi->hwrng_removed) > return -ENODEV; > > + if (!wait) > + return 0; > + > if (!vi->busy) { > vi->busy = true; > reinit_completion(&vi->have_data); > register_buffer(vi, buf, size); > } > > - if (!wait) > - return 0; > - > ret = wait_for_completion_killable(&vi->have_data); > if (ret < 0) > return ret; > > > 2- Use an internal intermediate buffer in virtio-rng, at a cost of a copy, > I have some patches (somewhere) I can refresh to do that. > > 3- modify hw_random/core.c to use only one buffer > > Thanks, > Laurent > > [1] 78887832e765 ("hwrng: core - don't wait on add_early_randomness()")4. actually differentiate between the two using the pointer returned by get_buf.> > > > HTH, > > Alex > > > > On Mon, Sep 13, 2021 at 8:52 AM Laurent Vivier <lvivier at redhat.com> wrote: > > > > > > Hi Alexander, > > > > > > On 12/09/2021 19:05, Alexander Potapenko wrote: > > > > Hi Laurent, > > > > > > > > Do you by any chance have an update on this? > > > > > > I'm sorry I didn't have the time until now. > > > > > > I try today. > > > > > > Could you give more details how to reproduce this? > > > (kernel version, .config, tools to run?) > > > > > > Thanks, > > > Laurent > > > > Thanks, > > > > Alex > > > > > > > > On Fri, Aug 20, 2021 at 6:15 PM Laurent Vivier <lvivier at redhat.com> wrote: > > > > > > > > > > On 19/08/2021 22:58, Michael S. Tsirkin wrote: > > > > > > On Fri, Nov 13, 2020 at 06:26:16PM +0100, Alexander Potapenko wrote: > > > > > > > Hi Amos, Rusty, Amit, Michael, > > > > > > > > > > > > > > I am hitting something that I believe to be a minor problem in the > > > > > > > virtio RNG driver. > > > > > > > When running the kernel under KMSAN with "-device virtio-rng-pci" > > > > > > > passed to QEMU, I am seeing reports about rng_fillbuf in > > > > > > > drivers/char/hw_random/core.c being used before initialization (see > > > > > > > the report below). > > > > > > > > > > > > > > This can be verified by initializing rng_fillbuf with 'A' as follows: > > > > > > > =========================================> > > > > > > diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c > > > > > > > index 8c1c47dd9f46..44d609a5796a 100644 > > > > > > > --- a/drivers/char/hw_random/core.c > > > > > > > +++ b/drivers/char/hw_random/core.c > > > > > > > @@ -439,8 +439,11 @@ static int hwrng_fillfn(void *unused) > > > > > > > if (IS_ERR(rng) || !rng) > > > > > > > break; > > > > > > > mutex_lock(&reading_mutex); > > > > > > > + memset(rng_fillbuf, 'A', rng_buffer_size()); > > > > > > > + rng_fillbuf[rng_buffer_size()-1] = 0; > > > > > > > rc = rng_get_data(rng, rng_fillbuf, > > > > > > > rng_buffer_size(), 1); > > > > > > > + pr_err("rng_fillbuf: %s\n", rng_fillbuf); > > > > > > > mutex_unlock(&reading_mutex); > > > > > > > put_rng(rng); > > > > > > > if (rc <= 0) { > > > > > > > =========================================> > > > > > > > > > > > > > and booting the kernel: the first call of hwrng_fillfn() will print > > > > > > > "AAAAAAA.." instead of random data. > > > > > > > > > > > > > > For some reason on that first iteration vi->busy is true here: > > > > > > > https://elixir.bootlin.com/linux/latest/source/drivers/char/hw_random/virtio-rng.c#L62, > > > > > > > therefore the buffer is not being sent to virtio ring. > > > > > > > > > > > > > > While probably being benign, this bug is preventing syzkaller from > > > > > > > finding more bugs, so it would be nice to fix it. > > > > > > > Perhaps the easiest solution is to kzalloc rng_fillbuf, but if it's > > > > > > > critical for this driver to not skip even the first read, then maybe > > > > > > > you have better ideas? > > > > > > > > > > > > > > KMSAN report follows: > > > > > > > > > > > > > > ====================================================> > > > > > > BUG: KMSAN: uninit-value in _mix_pool_bytes+0x7d2/0x950 > > > > > > > drivers/char/random.c:570 > > > > > > > CPU: 0 PID: 2711 Comm: hwrng Not tainted 5.9.0-rc8-syzkaller #0 > > > > > > > Hardware name: Google Google Compute Engine/Google Compute Engine, > > > > > > > BIOS Google 01/01/2011 > > > > > > > Call Trace: > > > > > > > __dump_stack lib/dump_stack.c:77 [inline] > > > > > > > dump_stack+0x21c/0x280 lib/dump_stack.c:118 > > > > > > > kmsan_report+0xf7/0x1e0 mm/kmsan/kmsan_report.c:122 > > > > > > > __msan_warning+0x5f/0xa0 mm/kmsan/kmsan_instr.c:201 > > > > > > > _mix_pool_bytes+0x7d2/0x950 drivers/char/random.c:570 > > > > > > > mix_pool_bytes+0xca/0x2a0 drivers/char/random.c:599 > > > > > > > add_hwgenerator_randomness+0x4ac/0x500 drivers/char/random.c:2319 > > > > > > > hwrng_fillfn+0x6ae/0x940 drivers/char/hw_random/core.c:452 > > > > > > > kthread+0x51c/0x560 kernel/kthread.c:293 > > > > > > > ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:294 > > > > > > > > > > > > > > Uninit was created at: > > > > > > > kmsan_save_stack_with_flags mm/kmsan/kmsan.c:129 [inline] > > > > > > > kmsan_internal_poison_shadow+0x5c/0xf0 mm/kmsan/kmsan.c:112 > > > > > > > kmsan_slab_alloc+0x8d/0xe0 mm/kmsan/kmsan_hooks.c:80 > > > > > > > slab_alloc_node mm/slub.c:2903 [inline] > > > > > > > slab_alloc mm/slub.c:2912 [inline] > > > > > > > kmem_cache_alloc_trace+0x61e/0xc90 mm/slub.c:2929 > > > > > > > kmalloc include/linux/slab.h:554 [inline] > > > > > > > hwrng_modinit+0x103/0x2ef drivers/char/hw_random/core.c:621 > > > > > > > do_one_initcall+0x371/0x9c0 init/main.c:1208 > > > > > > > do_initcall_level+0x1e5/0x3c6 init/main.c:1281 > > > > > > > do_initcalls+0x127/0x1cb init/main.c:1297 > > > > > > > do_basic_setup+0x33/0x36 init/main.c:1317 > > > > > > > kernel_init_freeable+0x238/0x38b init/main.c:1517 > > > > > > > kernel_init+0x1f/0x840 init/main.c:1406 > > > > > > > ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:294 > > > > > > > ====================================================> > > > > > > > > > > > > > Thanks, > > > > > > > Alex > > > > > > > > > > > > > > > > > > Cc Laurent - I think he said he was going to look at virtio rng. > > > > > > > > > > I will have look next week. > > > > > > > > > > Thanks, > > > > > Laurent > > > > > > > > > > > > > > > > > > > >