Re: [PATCH] media: mali-c55: fix dropped last AEC histogram zone weight

[Jacopo Mondi <jacopo.mondi@ideasonboard.com>]

Hi Sashiko

On Tue, Jun 02, 2026 at 07:09:42PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> New issues:
> - [Medium] Endianness bug places the last AEC histogram zone weight in the wrong hardware register byte on big-endian architectures.
>
> Pre-existing issues:
> - [High] Casting the unaligned `zone_weights` byte array to `u32 *` causes undefined behavior and alignment faults.
> --
>
> commit 731a31e43f59014f133f6c66b9d8861ce15cfc76
> Author: David Carlier <devnexen@gmail.com>
>
> media: mali-c55: fix dropped last AEC histogram zone weight
>
> The 15x15 AEC histogram metering grid has 225 per-zone weights, packed
> by userspace as a u8 array. The driver writes the first 56 registers
> (zones 0 through 223) in a loop, then handles the final register on its
> own to keep static analysers from flagging the array access.
>
> > diff --git a/drivers/media/platform/arm/mali-c55/mali-c55-params.c b/drivers/media/platform/arm/mali-c55/mali-c55-params.c
> > index de0e9d898db7b..33e2232ec8f5b 100644
> > --- a/drivers/media/platform/arm/mali-c55/mali-c55-params.c
> > +++ b/drivers/media/platform/arm/mali-c55/mali-c55-params.c
> > @@ -212,6 +212,7 @@ mali_c55_params_aexp_hist_weights(struct mali_c55 *mali_c55,
>
> [ ... ]
>
> >  	for (unsigned int i = 0; i < 56; i++) {
> >  		val = ((u32 *)params->zone_weights)[i]
> >  			    & MALI_C55_AEXP_HIST_ZONE_WEIGHT_MASK;
>
> [Severity: High]
> This isn't a bug introduced by this patch, but could this cast in
> mali_c55_params_aexp_hist_weights() in
> drivers/media/platform/arm/mali-c55/mali-c55-params.c trigger an alignment
> fault on strict architectures?
>
> The zone_weights array is located at offset 10 within struct
> mali_c55_params_aexp_weights. Because the configuration block resides at a
> 4-byte aligned offset within the V4L2 buffer, the zone_weights array starts
> at an address that is only 2-byte aligned.

I'm a bit surprised because I thought the compiler was meant to insert
padding to align the 'zone_weights' member to a 4 bytes boundary, but
as 'zone_weights' is a u8 pointer, it is stored unaligned.

pahole confirms this:

struct mali_c55_params_aexp_weights {
	struct v4l2_isp_params_block_header header __attribute__((__aligned__(8))); /*     0     8 */
	__u8                       nodes_used_horiz;     /*     8     1 */
	__u8                       nodes_used_vert;      /*     9     1 */
	__u8                       zone_weights[225];    /*    10   225 */

	/* size: 240, cachelines: 4, members: 4 */
	/* padding: 5 */
	/* forced alignments: 1 */
	/* last cacheline: 48 bytes */
} __attribute__((__aligned__(8)));

>
> Casting this unaligned pointer to a 32-bit pointer and dereferencing it
> inside the loop triggers undefined behavior in C. On strict architectures
> like ARM with SCTLR.A enabled, or when auto-vectorized by the compiler,
> this can trigger kernel alignment faults.

This code only runs on ARMv8 where I read unaligned access is
permitted but less efficient ?

>
> Because userspace controls the V4L2 configuration buffer, can an
> unprivileged user use this to trigger a kernel oops and cause a local denial
> of service?

No I don't think so, the uABI is fixed userspace cannot modify its
layout.

I do however think it would be worth fixing the above loop to access
'zone_weigths' as a u8 * ?

>
> >  		addr = base + MALI_C55_AEXP_HIST_ZONE_WEIGHTS_OFFSET + (4 * i);
> >
> >  		mali_c55_ctx_write(mali_c55, addr, val);
> >  	}
> >
> >  	val = params->zone_weights[MALI_C55_MAX_ZONES - 1];
> >  	addr = base + MALI_C55_AEXP_HIST_ZONE_WEIGHTS_OFFSET + (4 * 56);
> > +	mali_c55_ctx_write(mali_c55, addr, val & MALI_C55_AEXP_HIST_ZONE_WEIGHT_MASK);
>
> [Severity: Medium]
> Is there an endianness issue on big-endian platforms when extracting this
> weight as an 8-bit value and implicitly zero-extending it to a 32-bit
> integer in mali_c55_params_aexp_hist_weights()?
>
> The original loop processes 4 weights at a time by reading them as a 32-bit
> integer and writing that to the memory-backed ctx->registers array. This
> preserves the byte stream natively on both architectures.
>
> However, extracting the final 225th weight as an 8-bit value zero-extends
> it to a 32-bit integer like 0x0000000E. On big-endian systems, storing this
> 32-bit value natively places the numeric value at the highest byte address
> of the 32-bit word (byte offset 3).
>
> When memcpy_toio() flushes this to the device, will the hardware receive
> the weight in the 4th byte of the register instead of the 1st byte (offset 0)
> as it expects, silently corrupting the auto-exposure metering weight?
>
> >  }
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260602185305.30759-1-devnexen@gmail.com?part=1
>