Re: [PATCH v11 9/9] i3c: hub: p3h2x4x: Add SMBus slave mode support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] Unconditional dereference of `tp_smbus_client` prior to its NULL check in `p3h2x4x_read_smbus_agent_rx_buf`.
- [High] Failure to clear `tp_smbus_client` on hardware error in `p3h2x4x_tp_i2c_unreg_slave` leads to Use-After-Free.
- [High] IBI requests are not cleaned up on the `probe` error path, leaving active interrupts pointing to freed device memory.
- [High] Early return on `regmap_read` failure in `p3h2x4x_ibi_handler` aborts processing for other ports, risking an interrupt storm.

Pre-existing issues:
- [High] I2C adapters registered in `p3h2x4x_tp_smbus_algo` are leaked on subsequent probe errors, leading to Use-After-Free.
--

commit c3438427374cdabfa4f0a09443f69b5b076d009a
Author: Lakshay Piplani <lakshay.piplani@nxp.com>

i3c: hub: p3h2x4x: Add SMBus slave mode support

Add SMBus slave mode support for the P3H2x4x hub SMBus target ports,
allowing the hub to receive downstream payloads into target buffers and
report receive events through IBI to registered I2C slave clients.

> diff --git a/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c b/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c
> --- a/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c
> +++ b/drivers/i3c/hub/p3h2840_i3c_hub_i3c.c
[ ... ]
> @@ -120,5 +128,24 @@ int p3h2x4x_tp_i3c_algo(struct p3h2x4x_i3c_hub_dev *p3h2x4x_hub)
>  		p3h2x4x_hub->tp_bus[tp].is_registered = true;
>  		p3h2x4x_hub->hub_config.tp_config[tp].always_enable = true;
>  	}
> -	return regmap_write(p3h2x4x_hub->regmap, P3H2X4X_TP_NET_CON_CONF, ntwk_mask);
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +	ret = i3c_device_request_ibi(p3h2x4x_hub->i3cdev, &p3h2x4x_ibireq);
> +	if (ret)
> +		return ret;
> +
> +	ret = i3c_device_enable_ibi(p3h2x4x_hub->i3cdev);

[Severity: High]
Because these IBI functions are not devres-managed, do we need to manually
clean them up on a probe failure?

If a subsequent step in p3h2x4x_i3c_hub_probe() fails after this completes,
probe will abort and devres will free the p3h2x4x_i3c_hub structure. Since
the I3C core isn't informed to free the IBI, the hardware remains
configured. If an IBI fires later, could p3h2x4x_ibi_handler() try to
dereference the freed driver data?

[ ... ]
> diff --git a/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c b/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
> --- a/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
> +++ b/drivers/i3c/hub/p3h2840_i3c_hub_smbus.c
[ ... ]
> @@ -15,6 +15,146 @@ 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, bool is_of)
> +{
[ ... ]
> +	/* notify slave driver about received data */
> +	if ((p3h2x4x_i3c_hub->tp_bus[tp].tp_smbus_client->addr & 0x7f) == (slave_address >> 1)) {
> +		client = p3h2x4x_i3c_hub->tp_bus[tp].tp_smbus_client;
> +		if (!client)
> +			goto ibi_err;

[Severity: High]
Can this code cause a panic if an In-Band Interrupt occurs for a port
without a bound SMBus slave client?

We dereference tp_smbus_client->addr unconditionally here, right before
the NULL check happens.

[ ... ]
> +void p3h2x4x_ibi_handler(struct i3c_device *i3cdev,
> +			 const struct i3c_ibi_payload *payload)
> +{
[ ... ]
> +	for (i = 0; i < P3H2X4X_TP_MAX_COUNT; ++i) {
> +		if (p3h2x4x_i3c_hub->tp_bus[i].is_registered && (payload_byte_two >> i) & 0x01) {
> +			ret = regmap_read(p3h2x4x_i3c_hub->regmap, P3H2X4X_TP0_SMBUS_AGNT_STS + i,
> +					  &target_port_status);
> +			if (ret) {
> +				dev_err(&i3cdev->dev, "target port read status failed %d\n", ret);
> +				return;
> +			}

[Severity: High]
If regmap_read() fails for one port, this return statement prematurely
exits the handler.

Does this skip clearing the hardware status of the current port and prevent
servicing other ports with pending events? If the underlying hardware
interrupt conditions are not cleared, could the device immediately re-assert
the IBI line and cause an interrupt storm?

[ ... ]
> +static int p3h2x4x_tp_i2c_unreg_slave(struct i2c_client *slave)
> +{
[ ... ]
> +	ret = regmap_clear_bits(hub->regmap,
> +				P3H2X4X_TP_SMBUS_AGNT_IBI_CONFIG,
> +				bus->tp_mask);
> +	if (ret)
> +		return ret;
> +
> +	bus->tp_smbus_client = NULL;

[Severity: High]
If regmap_clear_bits() fails, we return the error without clearing
bus->tp_smbus_client.

Since the client driver might still free its i2c_client structure during
removal despite the unregistration failure, could this leave a dangling
pointer that gets dereferenced if an IBI fires later?

[ ... ]
> @@ -205,11 +345,63 @@ static u32 p3h2x4x_tp_smbus_funcs(struct i2c_adapter *adapter)
>  	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_BLOCK_DATA;
>  }
>  
[ ... ]
>  /*
>  * I2C algorithm Structure
>  */
>  static struct i2c_algorithm p3h2x4x_tp_i2c_algorithm = {
>  	.master_xfer    = p3h2x4x_tp_i2c_xfer,
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> +	.reg_slave = p3h2x4x_tp_i2c_reg_slave,
> +	.unreg_slave = p3h2x4x_tp_i2c_unreg_slave,
> +#endif
>  	.functionality  = p3h2x4x_tp_smbus_funcs,
>  };

[Severity: High]
This is a pre-existing issue, but I2C adapters registered in
p3h2x4x_tp_smbus_algo() lack a devres action for automatic cleanup.

If p3h2x4x_i3c_hub_probe() encounters an error after adapters are added
with i2c_add_adapter(), devres will free the memory backing the adapters,
but they remain registered with the I2C subsystem.

Could this lead to a use-after-free if the I2C core accesses these ghost
adapters?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612111816.3688240-1-lakshay.piplani@nxp.com?part=9