[PATCH 1/1] Drivers: hv: Fix uninit'ed variable in hv_msg_dump() if CONFIG_PRINTK not set

[PATCH 1/1] Drivers: hv: Fix uninit'ed variable in hv_msg_dump() if CONFIG_PRINTK not set

[mhkelley58]

From: Michael Kelley <mhklinux@outlook.com>

When CONFIG_PRINTK is not set, kmsg_dump_get_buffer() returns 'false'
without setting the bytes_written argument. In such case, bytes_written
is uninitialized when it is tested for zero.

This is admittedly an unlikely scenario, but in the interest of correctness
and avoiding tool noise about uninitialized variables, fix this by testing
the return value before testing bytes_written.

Fixes: 9c318a1d9b50 ("Drivers: hv: move panic report code from vmbus to hv early init code")
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/all/202512172102.OcUspn1Z-lkp@intel.com/
Signed-off-by: Michael Kelley <mhklinux@outlook.com>
---
 drivers/hv/hv_common.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
index f466a6099eff..de9e069c5a0c 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)
@@ -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
-- 
2.25.1

Re: [PATCH 1/1] Drivers: hv: Fix uninit'ed variable in hv_msg_dump() if CONFIG_PRINTK not set

[vdso]

> On 12/19/2025 8:08 AM  mhkelley58@gmail.com wrote:
> 
>  
> From: Michael Kelley <mhklinux@outlook.com>
> 
> When CONFIG_PRINTK is not set, kmsg_dump_get_buffer() returns 'false'
> without setting the bytes_written argument. In such case, bytes_written
> is uninitialized when it is tested for zero.
> 
> This is admittedly an unlikely scenario, but in the interest of correctness
> and avoiding tool noise about uninitialized variables, fix this by testing
> the return value before testing bytes_written.
> 
> Fixes: 9c318a1d9b50 ("Drivers: hv: move panic report code from vmbus to hv early init code")
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/all/202512172102.OcUspn1Z-lkp@intel.com/
> Signed-off-by: Michael Kelley <mhklinux@outlook.com>
> ---
>  drivers/hv/hv_common.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
> index f466a6099eff..de9e069c5a0c 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)
> @@ -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? 

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?

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);
-       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

RE: [PATCH 1/1] Drivers: hv: Fix uninit'ed variable in hv_msg_dump() if CONFIG_PRINTK not set

[Michael Kelley]

From: vdso@mailbox.org <vdso@mailbox.org> Sent: Sunday, December 28, 2025 8:12 PM
> 
> > On 12/19/2025 8:08 AM  mhkelley58@gmail.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

RE: [PATCH 1/1] Drivers: hv: Fix uninit'ed variable in hv_msg_dump() if CONFIG_PRINTK not set

[Michael Kelley]

From: Michael Kelley <mhklinux@outlook.com> Sent: Monday, December 29, 2025 7:04 PM
> 
> From: vdso@mailbox.org <vdso@mailbox.org> Sent: Sunday, December 28, 2025 8:12 PM
> >
> > > On 12/19/2025 8:08 AM  mhkelley58@gmail.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.

FWIW, the plot thickens here a bit. In testing my new version of
the patch, it turns out that hv_kmsg_dump() is never called when
CONFIG_PRINTK is not set. In that case, kmsg_dump_register()
fails, and hv_kmsg_dump_register() sets hv_panic_page back
to NULL. So hyperv_report_panic() is used to report the panic
to Hyper-V with no guest message.

The original uninitialized variable report is actually bogus since
the non-stub kmsg_dump_get_buffer() always sets bytes_written,
even if it returns false. But it's still worth initializing bytes_written
to zero so the static checkers don't generate noise.

I'll take all this into account in my new version of the patch.

Michael

> 
> 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