Re: [PATCH 11/11] ipmi: Add NVIDIA LSTP IPMI BMC driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] devres: device mismatch in devm_add_action_or_reset causes UAF and resource leak
--

commit 9f3744092e4398b83c4dd08ac11a951e27142671
Author: Adrian Ambrozewicz <aambrozewicz@nvidia.com>

ipmi: Add NVIDIA LSTP IPMI BMC driver

This commit adds a BMC-side driver for the IPMI channels of NVIDIA LSTP USB
adapters. The adapter carries IPMI messages between the host and the BMC,
exposing each channel as an ipmi-lstp<n> character device.

> diff --git a/drivers/char/ipmi/ipmi-lstp.c b/drivers/char/ipmi/ipmi-lstp.c
> index 0000000000000..787d633d4ada7
> --- /dev/null
> +++ b/drivers/char/ipmi/ipmi-lstp.c

[ ... ]

> +static int lstp_ipmi_start(struct lstp_channel *ch, struct lstp_ipmi_ctx *ctx)
> +{
> +	int ret = misc_register(&ctx->miscdev);
> +
> +	if (ret) {
> +		dev_err(ch->dev, "%s: ch_%d: Could not register miscdevice (%pe)\n", __func__,
> +			ch->ch_id, ERR_PTR(ret));
> +		return ret;
> +	}
> +
> +	ret = devm_add_action_or_reset(ch->dev, lstp_ipmi_stop_and_deregister, ctx);

[Severity: High]
Are these devres actions being attached to the correct device?

In an auxiliary driver, devres actions should typically be attached to the
device being probed (&auxdev->dev), not its parent. Here, the driver
incorrectly attaches its teardown action (lstp_ipmi_stop_and_deregister)
to the parent USB device (ch->dev).

If the auxiliary driver is unbound (for example, via rmmod), the driver core
cleans up actions attached to &auxdev->dev. Because this action is queued on
ch->dev, it will not execute and the miscdevice will remain registered.

When the module is unloaded, the dangling actions remain on the parent's
devres list. If userspace opens the lingering device node, or if the USB
device is later disconnected, the kernel will jump to unmapped module memory
and crash.

> +	if (ret)
> +		return ret;
> +
> +	dev_info(ch->dev, "%s: ch_%d: Started as %s\n", __func__, ch->ch_id, ch->display_name);
> +	return 0;
> +}
> +
> +static int lstp_ipmi_probe(struct auxiliary_device *auxdev, const struct auxiliary_device_id *id)
> +{
> +	struct lstp_channel *ch = lstp_channel_from_auxdev(auxdev);
> +	const struct lstp_channel_init_info *info = dev_get_platdata(&auxdev->dev);
> +
> +	if (!info)
> +		return -ENODEV;
> +
> +	int ret = lstp_ipmi_init(ch, info);
> +
> +	if (ret)
> +		return ret;
> +
> +	/* Booked first, so it unwinds after the node's deregister action. */
> +	ret = devm_add_action_or_reset(ch->dev, lstp_ipmi_ctx_put, ch->priv);

[Severity: High]
Similar to the issue above, does this context put action also attach to the
wrong device?

By registering against ch->dev instead of &auxdev->dev, the context memory
will leak upon unbind because the driver core will not execute this cleanup
action.

Additionally, if misc_register() fails during probe, the devres unwind
targets &auxdev->dev, leaving the lstp_ipmi_ctx_put action queued on
ch->dev and leaking the context.

> +	if (ret)
> +		return ret;
> +
> +	return lstp_ipmi_start(ch, ch->priv);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012-lstp-usb-adapter-v1-0-aambrozewicz@nvidia.com?part=11