[PATCH v2] kern: perform NULL check in unregister paths (command/extcmd)

[PATCH v2] kern: perform NULL check in unregister paths (command/extcmd)

[Srish Srinivasan]

Many modules call grub_unregister_{command(), extcmd()} from
GRUB_MOD_FINI() without checking for a failure in registration.
This could lead to a NULL pointer dereference in unregistration.

Perform explicit NULL check in both the unregister helpers.

Signed-off-by: Srish Srinivasan <ssrish@linux.ibm.com>
---
 grub-core/commands/extcmd.c | 3 +++
 grub-core/kern/command.c    | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/grub-core/commands/extcmd.c b/grub-core/commands/extcmd.c
index c236be13a..7f8cb3f67 100644
--- a/grub-core/commands/extcmd.c
+++ b/grub-core/commands/extcmd.c
@@ -139,6 +139,9 @@ grub_register_extcmd_lockdown (const char *name, grub_extcmd_func_t func,
 void
 grub_unregister_extcmd (grub_extcmd_t ext)
 {
+  if (ext == NULL)
+    return;
+
   grub_unregister_command (ext->cmd);
   grub_free (ext);
 }
diff --git a/grub-core/kern/command.c b/grub-core/kern/command.c
index 5812e131c..a1b4c81a9 100644
--- a/grub-core/kern/command.c
+++ b/grub-core/kern/command.c
@@ -104,6 +104,9 @@ grub_register_command_lockdown (const char *name,
 void
 grub_unregister_command (grub_command_t cmd)
 {
+  if (cmd == NULL)
+    return;
+
   if ((cmd->prio & GRUB_COMMAND_FLAG_ACTIVE) && (cmd->next))
     cmd->next->prio |= GRUB_COMMAND_FLAG_ACTIVE;
   grub_list_remove (GRUB_AS_LIST (cmd));
-- 
2.51.0


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2] kern: perform NULL check in unregister paths (command/extcmd)

[Sudhakar Kuppusamy]

> On 9 Sep 2025, at 12:25 PM, Srish Srinivasan <ssrish@linux.ibm.com> wrote:
> 
> Many modules call grub_unregister_{command(), extcmd()} from
> GRUB_MOD_FINI() without checking for a failure in registration.
> This could lead to a NULL pointer dereference in unregistration.
> 
> Perform explicit NULL check in both the unregister helpers.
> 
> Signed-off-by: Srish Srinivasan <ssrish@linux.ibm.com>

Reviewed-by: Sudhakar Kuppusamy <sudhakar@linux.ibm.com>

> ---
> grub-core/commands/extcmd.c | 3 +++
> grub-core/kern/command.c    | 3 +++
> 2 files changed, 6 insertions(+)
> 
> diff --git a/grub-core/commands/extcmd.c b/grub-core/commands/extcmd.c
> index c236be13a..7f8cb3f67 100644
> --- a/grub-core/commands/extcmd.c
> +++ b/grub-core/commands/extcmd.c
> @@ -139,6 +139,9 @@ grub_register_extcmd_lockdown (const char *name, grub_extcmd_func_t func,
> void
> grub_unregister_extcmd (grub_extcmd_t ext)
> {
> +  if (ext == NULL)
> +    return;
> +
>   grub_unregister_command (ext->cmd);
>   grub_free (ext);
> }
> diff --git a/grub-core/kern/command.c b/grub-core/kern/command.c
> index 5812e131c..a1b4c81a9 100644
> --- a/grub-core/kern/command.c
> +++ b/grub-core/kern/command.c
> @@ -104,6 +104,9 @@ grub_register_command_lockdown (const char *name,
> void
> grub_unregister_command (grub_command_t cmd)
> {
> +  if (cmd == NULL)
> +    return;
> +
>   if ((cmd->prio & GRUB_COMMAND_FLAG_ACTIVE) && (cmd->next))
>     cmd->next->prio |= GRUB_COMMAND_FLAG_ACTIVE;
>   grub_list_remove (GRUB_AS_LIST (cmd));
> -- 
> 2.51.0
> 


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2] kern: perform NULL check in unregister paths (command/extcmd)

[Stefan Berger]

On 9/9/25 4:22 AM, Sudhakar Kuppusamy wrote:
> 
> 
>> On 9 Sep 2025, at 12:25 PM, Srish Srinivasan <ssrish@linux.ibm.com> wrote:
>>
>> Many modules call grub_unregister_{command(), extcmd()} from
>> GRUB_MOD_FINI() without checking for a failure in registration.
>> This could lead to a NULL pointer dereference in unregistration.
>>
>> Perform explicit NULL check in both the unregister helpers.
>>
>> Signed-off-by: Srish Srinivasan <ssrish@linux.ibm.com>
> 
> Reviewed-by: Sudhakar Kuppusamy <sudhakar@linux.ibm.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

> 
>> ---
>> grub-core/commands/extcmd.c | 3 +++
>> grub-core/kern/command.c    | 3 +++
>> 2 files changed, 6 insertions(+)
>>
>> diff --git a/grub-core/commands/extcmd.c b/grub-core/commands/extcmd.c
>> index c236be13a..7f8cb3f67 100644
>> --- a/grub-core/commands/extcmd.c
>> +++ b/grub-core/commands/extcmd.c
>> @@ -139,6 +139,9 @@ grub_register_extcmd_lockdown (const char *name, grub_extcmd_func_t func,
>> void
>> grub_unregister_extcmd (grub_extcmd_t ext)
>> {
>> +  if (ext == NULL)
>> +    return;
>> +
>>    grub_unregister_command (ext->cmd);
>>    grub_free (ext);
>> }
>> diff --git a/grub-core/kern/command.c b/grub-core/kern/command.c
>> index 5812e131c..a1b4c81a9 100644
>> --- a/grub-core/kern/command.c
>> +++ b/grub-core/kern/command.c
>> @@ -104,6 +104,9 @@ grub_register_command_lockdown (const char *name,
>> void
>> grub_unregister_command (grub_command_t cmd)
>> {
>> +  if (cmd == NULL)
>> +    return;
>> +
>>    if ((cmd->prio & GRUB_COMMAND_FLAG_ACTIVE) && (cmd->next))
>>      cmd->next->prio |= GRUB_COMMAND_FLAG_ACTIVE;
>>    grub_list_remove (GRUB_AS_LIST (cmd));
>> -- 
>> 2.51.0
>>
> 


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2] kern: perform NULL check in unregister paths (command/extcmd)

[Avnish Chouhan]

On 2025-09-10 18:27, grub-devel-request@gnu.org wrote:
> ----------------------------------------------------------------------
> 
> Message: 1
> Date: Tue, 9 Sep 2025 11:09:55 -0400
> From: Stefan Berger <stefanb@linux.ibm.com>
> To: Sudhakar Kuppusamy <sudhakar@linux.ibm.com>, Srish Srinivasan
> 	<ssrish@linux.ibm.com>
> Cc: grub-devel@gnu.org, daniel.kiper@oracle.com, phcoder@gmail.com,
> 	nayna@linux.ibm.com, sridharm@linux.ibm.com
> Subject: Re: [PATCH v2] kern: perform NULL check in unregister paths
> 	(command/extcmd)
> Message-ID: <57eced6f-6b12-4d95-953c-98329bce3b82@linux.ibm.com>
> Content-Type: text/plain; charset=UTF-8; format=flowed
> 
> 
> 
> On 9/9/25 4:22 AM, Sudhakar Kuppusamy wrote:
>> 
>> 
>>> On 9 Sep 2025, at 12:25 PM, Srish Srinivasan <ssrish@linux.ibm.com> 
>>> wrote:
>>> 
>>> Many modules call grub_unregister_{command(), extcmd()} from
>>> GRUB_MOD_FINI() without checking for a failure in registration.
>>> This could lead to a NULL pointer dereference in unregistration.
>>> 
>>> Perform explicit NULL check in both the unregister helpers.
>>> 
>>> Signed-off-by: Srish Srinivasan <ssrish@linux.ibm.com>
>> 
>> Reviewed-by: Sudhakar Kuppusamy <sudhakar@linux.ibm.com>
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> 

Hi Srish,
Thank you so much for the patch!

A failure in registration will anyways returns NULL. Re checking it 
might not make a sense. Do you have any specific scenario for this?
Thank you!


Regards,
Avnish Chouhan

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2] kern: perform NULL check in unregister paths (command/extcmd)

[Srish Srinivasan]

On 9/11/25 12:29 PM, Avnish Chouhan wrote:
> On 2025-09-10 18:27, grub-devel-request@gnu.org wrote:
>> ----------------------------------------------------------------------
>>
>> Message: 1
>> Date: Tue, 9 Sep 2025 11:09:55 -0400
>> From: Stefan Berger <stefanb@linux.ibm.com>
>> To: Sudhakar Kuppusamy <sudhakar@linux.ibm.com>, Srish Srinivasan
>>     <ssrish@linux.ibm.com>
>> Cc: grub-devel@gnu.org, daniel.kiper@oracle.com, phcoder@gmail.com,
>>     nayna@linux.ibm.com, sridharm@linux.ibm.com
>> Subject: Re: [PATCH v2] kern: perform NULL check in unregister paths
>>     (command/extcmd)
>> Message-ID: <57eced6f-6b12-4d95-953c-98329bce3b82@linux.ibm.com>
>> Content-Type: text/plain; charset=UTF-8; format=flowed
>>
>>
>>
>> On 9/9/25 4:22 AM, Sudhakar Kuppusamy wrote:
>>>
>>>
>>>> On 9 Sep 2025, at 12:25 PM, Srish Srinivasan <ssrish@linux.ibm.com> 
>>>> wrote:
>>>>
>>>> Many modules call grub_unregister_{command(), extcmd()} from
>>>> GRUB_MOD_FINI() without checking for a failure in registration.
>>>> This could lead to a NULL pointer dereference in unregistration.
>>>>
>>>> Perform explicit NULL check in both the unregister helpers.
>>>>
>>>> Signed-off-by: Srish Srinivasan <ssrish@linux.ibm.com>
>>>
>>> Reviewed-by: Sudhakar Kuppusamy <sudhakar@linux.ibm.com>
>> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
>>
>
> Hi Srish,
> Thank you so much for the patch!
>
> A failure in registration will anyways returns NULL. Re checking it 
> might not make a sense. Do you have any specific scenario for this?
> Thank you!
>
>
> Regards,
> Avnish Chouhan
Hi Avnish,
During module registration, if there is a failure in memory allocation 
then a NULL is returned.
https://github.com/olafhering/grub/blob/master/grub-core/kern/command.c#L40

But during unregistration, not all modules check for this NULL.

here is an example where NULL is checked.
https://github.com/olafhering/grub/blob/master/grub-core/disk/diskfilter.c#L1491

But for example in
https://github.com/olafhering/grub/blob/master/grub-core/commands/efi/efitextmode.c#L151

there is no explicit check done before unregistration. And there are 
many such instances.

Thanks,
Srish
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [RFC PATCH 1/2] target/i386: add compatibility property for arch_capabilities

[Avnish Chouhan]

On 2025-09-10 18:27, grub-devel-request@gnu.org wrote:
> Message: 2
> Date: Wed, 10 Sep 2025 10:24:31 +0200
> From: Hector Cao <hector.cao@canonical.com>
> To: grub-devel@gnu.org
> Subject: [RFC PATCH 1/2] target/i386: add compatibility property for
> 	arch_capabilities
> Message-ID: <20250910082432.14764-2-hector.cao@canonical.com>
> 
> Prior to v10.1, if requested by user, arch-capabilities is always on
> despite the fact that CPUID advertises it to be off/unvailable.
> this causes a migration issue for VMs that are run on a machine
> without arch-capabilities and expect this feature to be present
> on the destination host with QEMU 10.1.
> 
> This commit add a compatibility property to restore the legacy
> behavior for all machines with version prior to 10.1
> 
> Signed-off-by: Hector Cao <hector.cao@canonical.com>
> ---
>  hw/core/machine.c     |  1 +
>  migration/migration.h | 12 ++++++++++++
>  migration/options.c   |  3 +++
>  target/i386/kvm/kvm.c |  5 ++++-
>  4 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 38c949c4f2..8ad5d79cb3 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -45,6 +45,7 @@ GlobalProperty hw_compat_10_0[] = {
>      { "vfio-pci", "x-migration-load-config-after-iter", "off" },
>      { "ramfb", "use-legacy-x86-rom", "true"},
>      { "vfio-pci-nohotplug", "use-legacy-x86-rom", "true" },
> +    { "migration", "arch-cap-always-on", "true" },
>  };
>  const size_t hw_compat_10_0_len = G_N_ELEMENTS(hw_compat_10_0);
> 
> diff --git a/migration/migration.h b/migration/migration.h
> index 01329bf824..5124ff3636 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -510,6 +510,18 @@ struct MigrationState {
>      bool rdma_migration;
> 
>      GSource *hup_source;
> +
> +    /*
> +     * This variable allows to keep the backward compatibility with
> QEMU (<10.1)
> +     * on the arch-capabilities detection.
> +     * With the commit d3a2413 (since 10.1), the arch-capabilities
> feature is gated
> +     * with the CPUID bit (CPUID_7_0_EDX_ARCH_CAPABILITIES) instead
> of being always
> +     * enabled when user requests for it. this new behavior breaks
> migration of VMs
> +     * created and run with older QEMU on machines without
> IA32_ARCH_CAPABILITIES MSR,
> +     * those VMs might have arch-capabilities enabled and break when 
> migrating
> +     * to a host with QEMU 10.1 with error : missing feature 
> arch-capabilities
> +     */
> +    bool arch_cap_always_on;
>  };
> 
>  void migrate_set_state(MigrationStatus *state, MigrationStatus 
> old_state,
> diff --git a/migration/options.c b/migration/options.c
> index 4e923a2e07..3a80dba9c5 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -203,6 +203,9 @@ const Property migration_properties[] = {
>                          MIGRATION_CAPABILITY_SWITCHOVER_ACK),
>      DEFINE_PROP_MIG_CAP("x-dirty-limit", 
> MIGRATION_CAPABILITY_DIRTY_LIMIT),
>      DEFINE_PROP_MIG_CAP("mapped-ram", 
> MIGRATION_CAPABILITY_MAPPED_RAM),
> +
> +    DEFINE_PROP_BOOL("arch-cap-always-on", MigrationState,

Hi Hector,

Missing space before '('

> +                     arch_cap_always_on, false),
>  };
>  const size_t migration_properties_count = 
> ARRAY_SIZE(migration_properties);
> 
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index 306430a052..e2ec4e6de5 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -42,6 +42,7 @@
>  #include "xen-emu.h"
>  #include "hyperv.h"
>  #include "hyperv-proto.h"
> +#include "migration/migration.h"
> 
>  #include "gdbstub/enums.h"
>  #include "qemu/host-utils.h"
> @@ -438,6 +439,7 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s,
> uint32_t function,
>      uint32_t ret = 0;
>      uint32_t cpuid_1_edx, unused;
>      uint64_t bitmask;
> +    MigrationState *ms = migrate_get_current();
> 
>      cpuid = get_supported_cpuid(s);
> 
> @@ -508,7 +510,8 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s,
> uint32_t function,
>           * mcahines at all, do not show the fake ARCH_CAPABILITIES MSR 
> that
>           * KVM sets up.
>           */
> -        if (!has_msr_arch_capabs || !(edx & 
> CPUID_7_0_EDX_ARCH_CAPABILITIES)) {
> +        if (!has_msr_arch_capabs
> +            || (!(edx & CPUID_7_0_EDX_ARCH_CAPABILITIES) &&
> (!ms->arch_cap_always_on))) {

Please move '{' in the next line.

Thank you!

Regards,
Avnish Chouhan

>              ret &= ~CPUID_7_0_EDX_ARCH_CAPABILITIES;
>          }
>      } else if (function == 7 && index == 1 && reg == R_EAX) {
> --
> 2.45.2
> 
> 
> 
> 
> ------------------------------

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [RFC PATCH 2/2] target/i386: add compatibility property for pdcm feature

[Avnish Chouhan]

On 2025-09-10 18:27, grub-devel-request@gnu.org wrote:
> Message: 3
> Date: Wed, 10 Sep 2025 10:24:32 +0200
> From: Hector Cao <hector.cao@canonical.com>
> To: grub-devel@gnu.org
> Subject: [RFC PATCH 2/2] target/i386: add compatibility property for
> 	pdcm feature
> Message-ID: <20250910082432.14764-3-hector.cao@canonical.com>
> 
> The pdcm feature is supposed to be disabled when PMU is not
> available. Up until v10.1, pdcm feature is enabled even when PMU
> is off. This behavior has been fixed but this change breaks the
> migration of VMs that are run with QEMU < 10.0 and expect the pdcm
> feature to be enabled on the destination host.
> 
> This commit restores the legacy behavior for machines with version
> prior to 10.1 to allow the migration from older QEMU to QEMU 10.1.
> 
> Signed-off-by: Hector Cao <hector.cao@canonical.com>
> ---
>  hw/core/machine.c     |  1 +
>  migration/migration.h | 11 +++++++++++
>  migration/options.c   |  3 +++
>  target/i386/cpu.c     | 17 ++++++++++++++---
>  4 files changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 8ad5d79cb3..535184c221 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -46,6 +46,7 @@ GlobalProperty hw_compat_10_0[] = {
>      { "ramfb", "use-legacy-x86-rom", "true"},
>      { "vfio-pci-nohotplug", "use-legacy-x86-rom", "true" },
>      { "migration", "arch-cap-always-on", "true" },
> +    { "migration", "pdcm-on-even-without-pmu", "true" },
>  };
>  const size_t hw_compat_10_0_len = G_N_ELEMENTS(hw_compat_10_0);
> 
> diff --git a/migration/migration.h b/migration/migration.h
> index 5124ff3636..7d5b2aa042 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -522,6 +522,17 @@ struct MigrationState {
>       * to a host with QEMU 10.1 with error : missing feature 
> arch-capabilities
>       */
>      bool arch_cap_always_on;
> +
> +    /*
> +     * This variable allows to keep the backward compatibility with
> QEMU (<10.1)
> +     * on the pdcm feature detection. The pdcm feature should be 
> disabled when
> +     * PMU is not available. Prio to 10.1, there is a bug and pdcm can 
> still be
> +     * enabled even if PMU is off. This behavior has been fixed by the 
> commit
> +     * e68ec29 (since 10.1).
> +     * This new behavior breaks migration of VMs that expect, with the 
> QEMU
> +     * (since 10.1), pdcm to be disabled.
> +     */
> +    bool pdcm_on_even_without_pmu;
>  };
> 
>  void migrate_set_state(MigrationStatus *state, MigrationStatus 
> old_state,
> diff --git a/migration/options.c b/migration/options.c
> index 3a80dba9c5..a2a95dfcc4 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -206,6 +206,9 @@ const Property migration_properties[] = {
> 
>      DEFINE_PROP_BOOL("arch-cap-always-on", MigrationState,
>                       arch_cap_always_on, false),
> +
> +    DEFINE_PROP_BOOL("pdcm-on-even-without-pmu", MigrationState,

Hi Hector,

Missing space before '('

> +                     pdcm_on_even_without_pmu, false),
>  };
>  const size_t migration_properties_count = 
> ARRAY_SIZE(migration_properties);
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 6d85149e6e..1f0f2c8dbf 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -51,6 +51,8 @@
>  #include "disas/capstone.h"
>  #include "cpu-internal.h"
> 
> +#include "migration/migration.h"
> +
>  static void x86_cpu_realizefn(DeviceState *dev, Error **errp);
>  static void x86_cpu_get_supported_cpuid(uint32_t func, uint32_t index,
>                                          uint32_t *eax, uint32_t *ebx,
> @@ -7839,6 +7841,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t
> index, uint32_t count,
>      uint32_t signature[3];
>      X86CPUTopoInfo *topo_info = &env->topo_info;
>      uint32_t threads_per_pkg;
> +    MigrationState *ms = migrate_get_current();
> 
>      threads_per_pkg = x86_threads_per_pkg(topo_info);
> 
> @@ -7894,6 +7897,11 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t
> index, uint32_t count,
>              /* Fixup overflow: max value for bits 23-16 is 255. */
>              *ebx |= MIN(num, 255) << 16;
>          }
> +        if (ms->pdcm_on_even_without_pmu) {

Please move '{' to next line.

> +            if (!cpu->enable_pmu) {

Same as above!

> +                *ecx &= ~CPUID_EXT_PDCM;
> +            }
> +        }
>          break;
>      case 2: { /* cache info: needed for Pentium Pro compatibility */
>          const CPUCaches *caches;
> @@ -8892,6 +8900,7 @@ void x86_cpu_expand_features(X86CPU *cpu, Error 
> **errp)
>      FeatureWord w;
>      int i;
>      GList *l;
> +    MigrationState *ms = migrate_get_current();
> 
>      for (l = plus_features; l; l = l->next) {
>          const char *prop = l->data;
> @@ -8944,9 +8953,11 @@ void x86_cpu_expand_features(X86CPU *cpu, Error 
> **errp)
>          }
>      }
> 
> -    /* PDCM is fixed1 bit for TDX */
> -    if (!cpu->enable_pmu && !is_tdx_vm()) {
> -        env->features[FEAT_1_ECX] &= ~CPUID_EXT_PDCM;
> +    if (!ms->pdcm_on_even_without_pmu) {

Same as above!

> +        /* PDCM is fixed1 bit for TDX */
> +        if (!cpu->enable_pmu && !is_tdx_vm()) {

same as above!
Thank you!

Regards,
Avnish Chouhan

> +            env->features[FEAT_1_ECX] &= ~CPUID_EXT_PDCM;
> +        }
>      }
> 
>      for (i = 0; i < ARRAY_SIZE(feature_dependencies); i++) {
> --
> 2.45.2
> 
> 

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel