Re: [PATCH v2] media: atomisp: reject frame dimensions that overflow the size calculation

[Andy Shevchenko]

On Sat, Jun 27, 2026 at 01:03:55PM +0200, Doruk Tan Ozturk wrote:
> ia_css_frame_allocate() computes frame->data_bytes (a u32) in the
> frame_init_*_planes() helpers as width/padded_width * height *
> bytes-per-pixel * plane-count, using plain unsigned arithmetic with no
> overflow check, and then hands the result to hmm_alloc().

Translate this from programmer's mess to plain English.

> A large
> width/height pair wraps the u32, so hmm_alloc() returns an undersized

Same, u32 --> 32-bit value

> buffer that a subsequent copy can overflow.
> 
> Reject up front, in ia_css_frame_allocate() (which already returns
> -EINVAL for bad arguments), any dimensions whose worst-case byte count
> cannot be represented in the u32 data_bytes field. The factor 16

Same here, 32-bit field (no need to even mention data_bytes, it gives just
a noise to the reader).

> conservatively bounds the largest per-pixel multiplier across all
> supported formats (up to 6 planes, or 3x RGB planes with up to 4 bytes
> per element).
> 
> The most directly user-influenced caller,
> atomisp_v4l2_framebuffer_to_css_frame() (S_ISP_FPN_TABLE), is currently
> gated off by 2b7eb2c5dc72 ("staging: media: atomisp: Disallow all
> private IOCTLs"), and other callers pass driver-derived dimensions, so
> this is defense-in-depth on the shared allocator rather than a live
> userspace overflow.

> Found by 0sec's autonomous vulnerability analysis (https://0sec.ai).
> Found by static analysis; not yet runtime-reproduced (Intel atomisp
> hardware required).

This paragraph is not anymore required and considered a noise in the commit
message. Comment block is perfectly fine for this...

> Fixes: a49d25364dfb ("staging/atomisp: Add support for the Intel IPU v2")
> Assisted-by: 0sec:claude-opus-4.8
> Signed-off-by: Doruk Tan Ozturk <doruk@xxxxxxx>
> ---

...somewhere here.

>  drivers/staging/media/atomisp/pci/runtime/frame/src/frame.c | 6 ++++++
>  1 file changed, 6 insertions(+)

...

>  	if (!frame || width == 0 || height == 0)
>  		return -EINVAL;
>  
> +	if (check_mul_overflow(max(width, padded_width), height, &bytes) ||
> +	    check_mul_overflow(bytes, 16u, &bytes))

What does 'u' give us here?

> +		return -EINVAL;
> +
>  	ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE,
>  			    "ia_css_frame_allocate() enter: width=%d, height=%d, format=%d, padded_width=%d, raw_bit_depth=%d\n",
>  			    width, height, format, padded_width, raw_bit_depth);

Shouldn't we dtrace still the error cases? Wouldn't be helpful in debugging?

-- 
With Best Regards,
Andy Shevchenko