| 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
| ||
|
Message-ID:
<SN6PR02MB4157065BCB5064B3587F9E4AD4BCA@SN6PR02MB4157.namprd02.prod.outlook.com>
Date: Tue, 30 Dec 2025 03:04:20 +0000
From: Michael Kelley <mhklinux@...look.com>
To: "vdso@...lbox.org" <vdso@...lbox.org>, "mhkelley58@...il.com"
<mhkelley58@...il.com>
CC: "haiyangz@...rosoft.com" <haiyangz@...rosoft.com>, "wei.liu@...nel.org"
<wei.liu@...nel.org>, "decui@...rosoft.com" <decui@...rosoft.com>,
"kys@...rosoft.com" <kys@...rosoft.com>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, "linux-hyperv@...r.kernel.org"
<linux-hyperv@...r.kernel.org>, "dan.carpenter@...aro.org"
<dan.carpenter@...aro.org>
Subject: RE: [PATCH 1/1] Drivers: hv: Fix uninit'ed variable in hv_msg_dump()
if CONFIG_PRINTK not set
From: vdso@...lbox.org <vdso@...lbox.org> Sent: Sunday, December 28, 2025 8:12 PM
>
> > On 12/19/2025 8:08 AM mhkelley58@...il.com wrote:
[snip]
> > @@ -198,9 +199,9 @@ static void hv_kmsg_dump(struct kmsg_dumper *dumper,
> > * be single-threaded.
> > */
> > kmsg_dump_rewind(&iter);
> > - kmsg_dump_get_buffer(&iter, false, hv_panic_page, HV_HYP_PAGE_SIZE,
> > - &bytes_written);
> > - if (!bytes_written)
> > + ret = kmsg_dump_get_buffer(&iter, false, hv_panic_page, HV_HYP_PAGE_SIZE,
> > + &bytes_written);
> > + if (!ret || !bytes_written)
> > return;
> > /*
> > * P3 to contain the physical address of the panic page & P4 to
>
> The existing code
>
> 1. doesn't care about the return value from kmsg_dump_get_buffer.
> The return value wouldn't make the function return before, why does that
> need to change?
The existing code depends on the implementation of kmsg_dump_get_buffer()
always setting bytes_written, even if it fails. That's atypical behavior, but it is
what kmsg_dump_get_buffer() does -- except that if CONFIG_PRINTK=n, the
stub kmsg_dump_get_buffer() does *not* do that. Testing the return value is
the more typical pattern, and bytes_written should be used only if the return
value indicates success. So that's why I proposed this change, instead of just
initializing bytes_written to zero when it is defined. My proposed change
makes the overall pattern more typical, and would work if the implementation
of kmsg_dump_get_buffer() should ever change to not set bytes_written in
some error case.
>
> 2. returns early when there are no bytes written.
> I think it shouldn't as otherwise the crash control register isn't written to,
> and the panic isn't signalled to the host. Is there another path maybe that
> I'm not noticing?
You make an excellent point. I didn't even think about the possibility of the
current logic being wrong. There is hyperv_report_panic(), but it is not called
if hv_panic_page is allocated, in order to avoid duplicate reports. I agree that
this code should go ahead and send the panic report even if there's no
message data. And in that case the discussion about testing the return value
from kmsg_dump_get_buffer() is moot.
I'll submit a new patch to change the behavior to send the panic report to
the host even if the message length is zero. I did a quick test of that case,
and it behaves like the case where HV_CRASH_CTL_CRASH_NOTIFY_MSG
is not set, which is fine.
I'll submit a new version of the patch focused on submitting the panic
report to the hypervisor even if the message size is zero. Avoiding the
uninitialized bytes_written will fall out of that change.
See a comment below in your suggested patch.
>
> That said, would it make sense to you the patch be something similar to:
>
> diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
> index 0a3ab7efed46..20e4a9a13b32 100644
> --- a/drivers/hv/hv_common.c
> +++ b/drivers/hv/hv_common.c
> @@ -188,6 +188,7 @@ static void hv_kmsg_dump(struct kmsg_dumper *dumper,
> {
> struct kmsg_dump_iter iter;
> size_t bytes_written;
> + bool ret;
>
> /* We are only interested in panics. */
> if (detail->reason != KMSG_DUMP_PANIC || !sysctl_record_panic_msg)
> @@ -197,11 +198,16 @@ static void hv_kmsg_dump(struct kmsg_dumper *dumper,
> * Write dump contents to the page. No need to synchronize; panic should
> * be single-threaded.
> */
> + bytes_written = 0;
> kmsg_dump_rewind(&iter);
> - kmsg_dump_get_buffer(&iter, false, hv_panic_page, HV_HYP_PAGE_SIZE,
> + ret = kmsg_dump_get_buffer(&iter, false, hv_panic_page, HV_HYP_PAGE_SIZE,
> &bytes_written);
Ignoring the return value can be made explicit as:
+ (void)kmsg_dump_get_buffer(&iter, false, hv_panic_page, HV_HYP_PAGE_SIZE,
&bytes_written);
Plus an appropriate comment. Then there's no need to introduce the "ret" local
variable and the somewhat funky:
(void) ret;
Michael
> - if (!bytes_written)
> - return;
> + /*
> + * Whether there is more data available or not, send what has been captured
> + * to the host. Ignore the return value.
> + */
> + (void) ret;
> +
> /*
> * P3 to contain the physical address of the panic page & P4 to
> * contain the size of the panic data in that page. Rest of the
> @@ -210,7 +216,7 @@ static void hv_kmsg_dump(struct kmsg_dumper *dumper,
> hv_set_msr(HV_MSR_CRASH_P0, 0);
> hv_set_msr(HV_MSR_CRASH_P1, 0);
> hv_set_msr(HV_MSR_CRASH_P2, 0);
> - hv_set_msr(HV_MSR_CRASH_P3, virt_to_phys(hv_panic_page));
> + hv_set_msr(HV_MSR_CRASH_P3, bytes_written ? virt_to_phys(hv_panic_page) : NULL);
> hv_set_msr(HV_MSR_CRASH_P4, bytes_written);
>
> /*
>
> --
> Cheers,
> Roman
>
> > --
> > 2.25.1
Powered by blists - more mailing lists