Thank you for your contribution! Sashiko AI review found 13 potential issue(s) to consider:
- [High] Stack buffer overflow in `max_ser_get_frame_desc_state` due to missing bounds check on `fd->num_entries`.
- [High] Writing unshifted 7-bit I2C client address into 8-bit hardware registers in `max_ser_fix_tx_ids`.
- [High] If `media_create_pad_link()` fails in `max_ser_notify_bound()`, `source->sd` is left pointing to the subdev, creating a dangling pointer.
- [High] Stack-allocated buffers are passed directly to `i2c_transfer()`, violating I2C API requirements and causing panics on DMA-enabled systems.
- [High] The global `ser->active` flag is incorrectly overwritten by the state of a single pad, allowing format modifications while the hardware is actively streaming.
- [High] Concurrent I2C ATR attach/detach operations cause data races on the shared `i2c_xlates` array.
- [High] TOCTOU race between async subdevice unbind and route processing leads to a use-after-free of the source subdevice.
- [High] The return value of `i2c_transfer()` is checked incorrectly, leading to silent partial transfer failures and garbage reads.
- [Medium] Improper use of devres APIs (`devm_kcalloc` and `devm_kfree`) in the hot streaming path.
- [Medium] Routing logic restricts all streams from a given PHY to a single pipe, ignoring multiple pipes.
- [Medium] Returning -EINVAL for unsupported formats in `set_fmt` and `set_frame_interval` breaks V4L2 format negotiation.
- [Low] Potential kernel panic due to division by zero if `num_phys` is 0.
- [Low] `max_ser_log_status` prematurely aborts logging when encountering the first disabled I2C translation, missing active translations at higher indices.
--
--- Patch [14]: [PATCH v15 14/22] media: i2c: add Maxim GMSL2/3 serializer framework ---
commit 60c008b16504a2b4c009035ea0ab39d7dfbf93c1
Author: Cosmin Tanislav <[email protected]>
media: i2c: add Maxim GMSL2/3 serializer framework
This patch adds a common framework for Maxim GMSL2/3 serializers supporting
features like the Streams API, get_frame_desc(), I2C ATR, and automatic
configurations.
> diff --git a/drivers/media/i2c/maxim-serdes/max_ser.c b/drivers/media/i2c/maxim-serdes/max_ser.c
> index 0000000000000..2699c57c18404 100644
> --- /dev/null
> +++ b/drivers/media/i2c/maxim-serdes/max_ser.c
[ ... ]
> @@ -109,6 +109,7 @@ static struct max_ser_pipe *
> max_ser_find_phy_pipe(struct max_ser *ser, struct max_ser_phy *phy)
> {
> unsigned int i;
>
> for (i = 0; i < ser->ops->num_pipes; i++) {
> struct max_ser_pipe *pipe = &ser->pipes[i];
>
> if (pipe->phy_id == phy->index)
> return pipe;
[Severity: Medium]
Will this unconditionally return the first matching pipe for a PHY?
If the hardware supports multiple pipes mapped to a single PHY, this
appears to render the additional assigned pipes unreachable.
> }
>
> return NULL;
> }
[ ... ]
> @@ -219,6 +219,8 @@ static int max_ser_route_to_hw(struct max_ser_priv *priv,
>
> hw->source = max_ser_get_phy_source(priv, phy);
> if (!hw->source->sd)
> return 0;
[Severity: High]
Does returning 0 unconditionally when the source subdevice is missing
allow arbitrary userspace-defined routes to bypass validation and
incorrectly increment the entry counter?
Also, could a TOCTOU race occur here? If max_ser_notify_unbind() clears
source->sd asynchronously without holding a lock, this might pass the NULL
check but result in a use-after-free when v4l2_subdev_call dereferences it.
>
> ret = v4l2_subdev_call(hw->source->sd, pad, get_frame_desc,
> hw->source->pad, &fd);
[ ... ]
> @@ -239,6 +239,8 @@ static int max_ser_i2c_atr_attach_addr(struct i2c_atr *atr, u32 chan_id,
> int ret;
>
> for (i = 0; i < ser->ops->num_i2c_xlates; i++)
> if (!ser->i2c_xlates[i].en)
> break;
[Severity: High]
Are concurrent attach and detach operations serialized here?
It looks like this finds a free slot and modifies the global translation array
without locking. Since the ATR core locks are per-channel, concurrent hotplugs
on different channels could cause data races on i2c_xlates.
>
> if (i == ser->ops->num_i2c_xlates) {
[ ... ]
> @@ -262,6 +262,8 @@ static void max_ser_i2c_atr_detach_addr(struct i2c_atr *atr, u32 chan_id, u16 ad
> unsigned int i;
>
> /* Find index of matching I2C translation. */
> for (i = 0; i < ser->ops->num_i2c_xlates; i++)
> if (ser->i2c_xlates[i].dst == addr)
> break;
[Severity: High]
Could clearing a slot in the global translation array race with other
attach/detach operations?
>
> if (WARN_ON(i == ser->ops->num_i2c_xlates))
[ ... ]
> @@ -420,6 +420,8 @@ static int max_ser_set_tpg_fmt(struct v4l2_subdev *sd,
>
> entry = max_ser_find_tpg_entry(ser, 0, fmt->width, fmt->height,
> fmt->code, 0, 0);
> if (!entry)
> return -EINVAL;
[Severity: Medium]
Should this return -EINVAL if the exact format is not found?
The V4L2 specification generally requires falling back to a supported format
rather than returning an error during format negotiation.
>
> in = v4l2_subdev_state_get_interval(state, format->pad, format->stream);
[ ... ]
> @@ -557,6 +557,7 @@ static int max_ser_log_status(struct v4l2_subdev *sd)
> v4l2_info(sd, "\ten: %u, src: 0x%02x dst: 0x%02x\n",
> ser->i2c_xlates[i].en, ser->i2c_xlates[i].src,
> ser->i2c_xlates[i].dst);
> if (!ser->i2c_xlates[i].en)
> break;
[Severity: Low]
Will the break statement prematurely abort logging?
If an intermediate I2C translation was detached, this might mask later valid
entries in the array.
> }
> v4l2_info(sd, "\n");
[ ... ]
> @@ -597,6 +597,8 @@ static int max_ser_set_frame_interval(struct v4l2_subdev *sd,
>
> entry = max_ser_find_tpg_entry(ser, 0, fmt->width, fmt->height,
> fmt->code, fi->interval.numerator,
> fi->interval.denominator);
> if (!entry)
> return -EINVAL;
[Severity: Medium]
Is it intentional to return -EINVAL for unsupported frame intervals?
V4L2 typically expects drivers to fall back to the closest supported interval.
>
> in = v4l2_subdev_state_get_interval(state, fi->pad, fi->stream);
[ ... ]
> @@ -678,6 +678,7 @@ static int max_ser_get_frame_desc_state(struct v4l2_subdev *sd,
> if (ret)
> return ret;
>
> hw.entry.stream = route->source_stream;
>
> fd->entry[fd->num_entries++] = hw.entry;
[Severity: High]
Can fd->num_entries exceed V4L2_FRAME_DESC_ENTRY_MAX here?
If a userspace application configures more than 8 active routes, this could
lead to a stack buffer overflow.
> }
>
> return 0;
[ ... ]
> @@ -897,6 +897,8 @@ static int max_ser_update_pipe(struct max_ser_priv *priv,
>
> if (!ser->ops->num_dts_per_pipe)
> return 0;
>
> dts = devm_kcalloc(priv->dev, ser->ops->num_dts_per_pipe, sizeof(*dts),
> GFP_KERNEL);
> if (!dts)
[Severity: Medium]
Is it safe to use devm_kcalloc dynamically in a runtime path?
Using devres APIs in the hot streaming path requires an O(N) list traversal
under a spinlock for every free operation, which could lead to locking
overhead and memory fragmentation.
> return -ENOMEM;
[ ... ]
> @@ -1098,6 +1098,7 @@ static int max_ser_update_streams(struct v4l2_subdev *sd,
> goto err_revert_phys_update;
> }
>
> devm_kfree(priv->dev, priv->streams_masks);
[Severity: Medium]
Similarly, is it appropriate to call devm_kfree dynamically here?
> priv->streams_masks = streams_masks;
> ser->active = !!streams_masks[pad];
[Severity: High]
Could overwriting the global ser->active flag with a single pad's state
allow format modifications while the hardware is actively streaming?
If Pad A is streaming and userspace disables Pad B, ser->active becomes 0,
potentially allowing set_fmt on Pad A.
>
> return 0;
[ ... ]
> @@ -1528,6 +1528,8 @@ static int max_ser_notify_bound(struct v4l2_async_notifier *nf,
> return ret;
> }
>
> source->sd = subdev;
> source->pad = ret;
>
> ret = media_create_pad_link(&source->sd->entity, source->pad,
[Severity: High]
If media_create_pad_link fails, will source->sd be left pointing to the subdev?
This could create a dangling pointer and result in a use-after-free later if
the async subdevice binding is aborted and freed.
> &priv->sd.entity, pad,
[ ... ]
> @@ -1549,6 +1549,7 @@ static void max_ser_notify_unbind(struct v4l2_async_notifier *nf,
> struct max_serdes_asc *asc = asc_to_max(base_asc);
> struct max_serdes_source *source = asc->source;
>
> source->sd = NULL;
[Severity: High]
Could a TOCTOU race occur between this unbind and route processing?
source->sd is cleared asynchronously without locks, which could race with
max_ser_route_to_hw() as mentioned earlier.
> }
[ ... ]
> @@ -1734,6 +1734,7 @@ static int max_ser_parse_dt(struct max_ser_priv *priv)
> for (i = 0; i < ser->ops->num_pipes; i++) {
> pipe = &ser->pipes[i];
> pipe->index = i;
> pipe->phy_id = i % ser->ops->num_phys;
[Severity: Low]
Could ser->ops->num_phys be 0 here, leading to a division by zero panic?
> pipe->stream_id = i % MAX_SERDES_STREAMS_NUM;
> }
[ ... ]
> @@ -2137,6 +2137,8 @@ static int max_ser_read_reg(struct i2c_adapter *adapter, u8 addr,
> u16 reg, u8 *val)
> {
> u8 buf[2] = { reg >> 8, reg & 0xff };
> struct i2c_msg msg[2] = {
> {
[Severity: High]
Is it safe to pass the stack-allocated buf directly to i2c_transfer()?
On systems with DMA-enabled I2C controllers and CONFIG_VMAP_STACK=y, passing
a stack buffer can cause memory corruption or kernel panics.
> .addr = addr,
[ ... ]
> @@ -2153,6 +2153,8 @@ static int max_ser_read_reg(struct i2c_adapter *adapter, u8 addr,
> };
> int ret;
>
> ret = i2c_transfer(adapter, msg, ARRAY_SIZE(msg));
> if (ret < 0)
> return ret;
[Severity: High]
Does this correctly check the return value of i2c_transfer()?
If a partial transfer occurs (e.g. ret == 1 due to a NAK on the second msg),
the driver might silently accept it and process uninitialized data from buf.
>
> *val = buf[0];
[ ... ]
> @@ -2165,6 +2165,8 @@ static int max_ser_write_reg(struct i2c_adapter *adapter, u8 addr,
> u16 reg, u8 val)
> {
> u8 buf[3] = { reg >> 8, reg & 0xff, val };
> struct i2c_msg msg[1] = {
> {
[Severity: High]
Similarly, passing the stack-allocated buf to i2c_transfer() here could
cause issues with DMA-enabled I2C controllers.
> .addr = addr,
[ ... ]
> @@ -2177,6 +2177,8 @@ static int max_ser_write_reg(struct i2c_adapter *adapter, u8 addr,
> };
> int ret;
>
> ret = i2c_transfer(adapter, msg, ARRAY_SIZE(msg));
> if (ret < 0)
> return ret;
[Severity: High]
Does if (ret < 0) fail to catch partial transfers for writes as well?
>
> return 0;
[ ... ]
> @@ -2236,6 +2236,7 @@ int max_ser_fix_tx_ids(struct i2c_adapter *adapter, u8 addr)
> int ret;
>
> for (i = 0; i < ARRAY_SIZE(addr_regs); i++) {
> ret = max_ser_write_reg(adapter, addr, addr_regs[i], addr);
[Severity: High]
Is the 7-bit I2C client address written directly without the required 1-bit
left shift? In max_ser_change_address, FIELD_PREP is used correctly, but
here the unshifted address is written to an 8-bit hardware register.
> if (ret)
> return ret;
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=14
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.