Re: [PATCH v2] ipmi: si: Fix NULL pointer dereference after failed registration

[Corey Minyard]

On Wed, Jul 01, 2026 at 02:43:48AM +0900, Seiji Nishikawa wrote:
> try_smi_init() allocates new_smi->si_sm and later calls
> ipmi_register_smi_mod(), which maps to ipmi_add_smi().
> 
> During ipmi_add_smi(), the upper IPMI message handler obtains the
> initial BMC device information through __bmc_get_device_id(). This can
> fail if the BMC does not return a successful response to the Get Device
> ID command.
> 
> When the BMC returns a nonzero completion code, the device-id helper
> retries the command and eventually returns -EIO if the device ID still
> cannot be fetched.
> 
> On this failure path, ipmi_add_smi() logs "Unable to get the device id"
> and goes to out_err_started, where it invokes the lower driver's
> shutdown callback. try_smi_init() then logs the returned registration
> failure:
> 
>  ipmi_si IPI0001:00: IPMI message handler: Unable to get the device id: -5
>  ipmi_si IPI0001:00: Unable to register device: error -5
> 
> For ipmi_si, the shutdown callback is shutdown_smi(), which cleans up
> the SI state machine data, frees smi_info->si_sm, and sets
> smi_info->si_sm and smi_info->intf to NULL.
> 
> However, intf->in_shutdown is not set on this failed-registration
> rollback path. Therefore, the asynchronous redo_bmc_reg work item can
> still retry BMC device-id probing after the lower driver has already
> cleared its SI state machine data. In the observed case, that retry path
> reached start_next_msg(), which passed the NULL smi_info->si_sm pointer
> to the selected KCS state machine handler:
> 
> BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
> Workqueue: events redo_bmc_reg [ipmi_msghandler]
> RIP: start_kcs_transaction+0x2c/0x190 [ipmi_si]
> Call Trace:
>  start_next_msg+0x50/0x80 [ipmi_si]
>  check_start_timer_thread.part.9+0x3b/0x50 [ipmi_si]
>  sender+0x69/0x80 [ipmi_si]
>  i_ipmi_request+0x2ac/0x9d0 [ipmi_msghandler]
>  __get_device_id.isra.29+0xaa/0x180 [ipmi_msghandler]
>  __bmc_get_device_id+0xef/0x950 [ipmi_msghandler]
>  redo_bmc_reg+0x52/0x60 [ipmi_msghandler]
>  process_one_work+0x1a7/0x360
> 
> Set intf->in_shutdown on the out_err_started path before invoking the
> lower driver's shutdown callback. This prevents later redo_bmc_reg
> retries from using an interface whose lower driver state has been
> cleaned up, and applies the same shutdown state to other IPMI interfaces
> as well.
> 
> Signed-off-by: Seiji Nishikawa <snishika@xxxxxxxxxx>
> ---
> Thanks, I agree. I moved the fix to the ipmi_add_smi() rollback path in v2
> and dropped the SI-side NULL checks.

Thanks, this is in my for-next tree.

-corey

> 
> Changes in v2:
> - Move the fix to ipmi_add_smi() by setting intf->in_shutdown on the
>   out_err_started path, as suggested by Corey.
> - Drop the NULL checks in ipmi_si_intf.c.
> 
>  drivers/char/ipmi/ipmi_msghandler.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
> index ab4c85f3d6fe..8d9f2e647d9b 100644
> --- a/drivers/char/ipmi/ipmi_msghandler.c
> +++ b/drivers/char/ipmi/ipmi_msghandler.c
> @@ -3757,6 +3757,7 @@ int ipmi_add_smi(struct module         *owner,
>   out_err_bmc_reg:
>  	ipmi_bmc_unregister(intf);
>   out_err_started:
> +	intf->in_shutdown = true;
>  	if (intf->handlers->shutdown)
>  		intf->handlers->shutdown(intf->send_info);
>   out_err:
> -- 
> 2.54.0
>