Re: [PATCH v15 8/8] i3c: hub: p3h2x4x: Add SMBus slave mode support

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Failing to clear the software slave state on hardware disable failure causes a use-after-free or invalid code execution when the slave driver unloads.
- [High] A stack-allocated buffer is passed to `regmap_bulk_read()` for an I3C device, violating the I3C core's requirement for DMA-able buffers and leading to DMA corruption.
- [Low] Missing I2C_FUNC_SLAVE flag in adapter functionality mask when CONFIG_I2C_SLAVE is enabled.
--

--- Patch [8]: [PATCH v15 8/8] i3c: hub: p3h2x4x: Add SMBus slave mode support ---
commit 1191525da64840a794eb47502525402f71bb5f79
Author: Lakshay Piplani <[email protected]>

i3c: hub: p3h2x4x: Add SMBus slave mode support

This commit adds SMBus slave mode support for the P3H2x4x hub SMBus target
ports. The hub SMBus slave agent can receive downstream payloads into target
buffers and report receive events through IBI.

> diff --git a/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c b/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
> index f6d4ade506e25..c095ae524e664 100644
> --- a/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
> +++ b/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
[ ... ]
> @@ -15,6 +17,177 @@ enum p3h2x4x_smbus_desc_idx {
>  	P3H2X4X_DESC_READ_LEN,
>  };
>  
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +static void p3h2x4x_read_smbus_agent_rx_buf(struct i3c_device *i3cdev, enum p3h2x4x_rcv_buf rfbuf,
> +					    enum p3h2x4x_tp tp)
> +{
> +	struct p3h2x4x *p3h2x4x = i3cdev_get_drvdata(i3cdev);
> +	struct p3h2x4x_i3c_hub_dev *p3h2x4x_i3c_hub;
> +	u8 slave_rx_buffer[P3H2X4X_SMBUS_TARGET_PAYLOAD_SIZE] = { 0 };
[ ... ]
> +	/* read data */
> +	if (packet_len) {
> +		ret = regmap_bulk_read(p3h2x4x_i3c_hub->regmap, P3H2X4X_TARGET_BUFF_DATA,
> +				       slave_rx_buffer, packet_len);

[Severity: High]
Is it safe to pass the stack-allocated slave_rx_buffer directly to
regmap_bulk_read()?

Since the underlying regmap is for an I3C device, this resolves to
regmap_i3c_read(), which maps the user buffer into an i3c_xfer struct.

The I3C subsystem mandates that buffers passed to transfers must be DMA-able.
Could passing a stack buffer here cause DMA corruption or kernel panics on
systems where the stack is virtually mapped?

> +		if (ret)
> +			goto ibi_err;
> +	}
> +
> +	client = p3h2x4x_i3c_hub->tp_bus[tp].tp_smbus_client;
> +	if (!client)
> +		goto ibi_err;
[ ... ]
> @@ -206,6 +379,115 @@ static u32 p3h2x4x_tp_smbus_funcs(struct i2c_adapter *adapter)
>  	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_BLOCK_DATA;
>  }

[Severity: Low]
Should I2C_FUNC_SLAVE be included in this mask when CONFIG_I2C_SLAVE is
enabled?

Without it, userspace tools and subsystems querying the adapter capabilities
via the I2C_FUNCS ioctl might incorrectly assume the adapter does not support
slave mode.

> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +static int p3h2x4x_tp_i2c_reg_slave(struct i2c_client *slave)
[ ... ]
> +static int p3h2x4x_tp_i2c_unreg_slave(struct i2c_client *slave)
> +{
> +	struct tp_bus *bus = i2c_get_adapdata(slave->adapter);
> +	struct p3h2x4x_i3c_hub_dev *hub = bus->p3h2x4x_i3c_hub;
> +	struct p3h2x4x *p3h2x4x = dev_get_drvdata(hub->dev->parent);
> +	int relock_ret = 0;
> +	int ret = 0;
> +
> +	guard(mutex)(&hub->etx_mutex);
> +
> +	if (bus->tp_smbus_client != slave)
> +		return -EINVAL;
> +
> +	scoped_guard(mutex, &p3h2x4x->protected_reg_lock) {
> +		/* Unlock access to protected registers */
> +		ret = regmap_write(hub->regmap, P3H2X4X_DEV_REG_PROTECTION_CODE,
> +				   P3H2X4X_REGISTERS_UNLOCK_CODE);
> +		if (ret)
> +			break;
> +
> +		ret = regmap_clear_bits(hub->regmap,
> +					P3H2X4X_TP_SMBUS_AGNT_IBI_CONFIG,
> +					bus->tp_mask);
> +
> +		/* Lock access to protected registers */
> +		relock_ret = regmap_write(hub->regmap,
> +					  P3H2X4X_DEV_REG_PROTECTION_CODE,
> +					  P3H2X4X_REGISTERS_LOCK_CODE);
> +	}
> +
> +	/*
> +	 * A relock failure does not re-enable the IBI, so report it
> +	 * separately and keep the callback result tied to the IBI operation.
> +	 */
> +	if (relock_ret)
> +		dev_err(hub->dev,
> +			"failed to restore protected register lock: %d\n",
> +			relock_ret);
> +
> +	if (ret)
> +		return ret;

[Severity: High]
If ret is non-zero (for example due to an I3C bus error), this returns early
without clearing bus->tp_smbus_client.

The i2c_slave_unregister() core function propagates the error, but the driver
will typically unbind anyway, freeing its private data and potentially
unloading the module.

If an IBI arrives later, could p3h2x4x_read_smbus_agent_rx_buf() see the
dangling tp_smbus_client pointer and call a freed slave_cb via
i2c_slave_event(), resulting in a use-after-free or kernel panic?

Should bus->tp_smbus_client be cleared unconditionally?

> +
> +	/*
> +	 * Clear the software state only after the hardware IBI has been
> +	 * disabled successfully.
> +	 */
> +	bus->tp_smbus_client = NULL;
> +	hub->hub_config.tp_config[bus->tp_port].ibi_en = false;
> +
> +	return 0;
> +}
> +#endif

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8

-- 
linux-i3c mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-i3c