* Re: [PATCH] media: mali-c55: fix dropped last AEC histogram zone weight
2026-06-02 18:53 [PATCH] media: mali-c55: fix dropped last AEC histogram zone weight David Carlier
@ 2026-06-02 19:09 ` sashiko-bot
2026-06-05 8:20 ` Jacopo Mondi
2026-06-05 8:05 ` Jacopo Mondi
2026-06-10 6:48 ` Dan Scally
2 siblings, 1 reply; 5+ messages in thread
From: sashiko-bot @ 2026-06-02 19:09 UTC (permalink / raw)
To: David Carlier; +Cc: linux-media
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.
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.
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?
> 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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] media: mali-c55: fix dropped last AEC histogram zone weight
2026-06-02 19:09 ` sashiko-bot
@ 2026-06-05 8:20 ` Jacopo Mondi
0 siblings, 0 replies; 5+ messages in thread
From: Jacopo Mondi @ 2026-06-05 8:20 UTC (permalink / raw)
To: sashiko-reviews; +Cc: David Carlier, linux-media
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] media: mali-c55: fix dropped last AEC histogram zone weight
2026-06-02 18:53 [PATCH] media: mali-c55: fix dropped last AEC histogram zone weight David Carlier
2026-06-02 19:09 ` sashiko-bot
@ 2026-06-05 8:05 ` Jacopo Mondi
2026-06-10 6:48 ` Dan Scally
2 siblings, 0 replies; 5+ messages in thread
From: Jacopo Mondi @ 2026-06-05 8:05 UTC (permalink / raw)
To: David Carlier
Cc: Daniel Scally, Jacopo Mondi, Mauro Carvalho Chehab,
Nayden Kanchev, Hans Verkuil, linux-media, linux-kernel, stable
Hi David
On Tue, Jun 02, 2026 at 07:53:05PM +0100, David Carlier wrote:
> 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.
>
> That separate path computes the address and value for the 225th weight
> (the bottom-right zone) but never issues the register write, so the zone
> keeps its stale or default weight. Any non-default weight userspace sets
> for the last zone is silently ignored, skewing auto-exposure metering.
> Both the AEXP_HIST_WEIGHTS and AEXP_IHIST_WEIGHTS blocks are affected as
> they share this handler.
>
> Issue the missing write, masking the value as the loop does.
>
> Fixes: 01535ea08674 ("media: platform: Add mali-c55 parameters video node")
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: David Carlier <devnexen@gmail.com>
> ---
> drivers/media/platform/arm/mali-c55/mali-c55-params.c | 1 +
> 1 file changed, 1 insertion(+)
>
> 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 de0e9d898db7..33e2232ec8f5 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,
>
> 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);
Indeed!
You could also mask val with 0xf0 when assigning it if you prefer, as
we really only care about the lower byte for zone 255.
Shouldn't make any difference though
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Thanks
j
> }
>
> static void mali_c55_params_digital_gain(struct mali_c55 *mali_c55,
> --
> 2.53.0
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] media: mali-c55: fix dropped last AEC histogram zone weight
2026-06-02 18:53 [PATCH] media: mali-c55: fix dropped last AEC histogram zone weight David Carlier
2026-06-02 19:09 ` sashiko-bot
2026-06-05 8:05 ` Jacopo Mondi
@ 2026-06-10 6:48 ` Dan Scally
2 siblings, 0 replies; 5+ messages in thread
From: Dan Scally @ 2026-06-10 6:48 UTC (permalink / raw)
To: David Carlier, Jacopo Mondi
Cc: Mauro Carvalho Chehab, Nayden Kanchev, Hans Verkuil, linux-media,
linux-kernel, stable
Hi David, thanks for the patch
On 02/06/2026 19:53, David Carlier wrote:
> 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.
>
> That separate path computes the address and value for the 225th weight
> (the bottom-right zone) but never issues the register write, so the zone
> keeps its stale or default weight. Any non-default weight userspace sets
> for the last zone is silently ignored, skewing auto-exposure metering.
> Both the AEXP_HIST_WEIGHTS and AEXP_IHIST_WEIGHTS blocks are affected as
> they share this handler.
>
> Issue the missing write, masking the value as the loop does.
>
> Fixes: 01535ea08674 ("media: platform: Add mali-c55 parameters video node")
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: David Carlier <devnexen@gmail.com>
> ---
Oops! Change looks good to me:
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
> drivers/media/platform/arm/mali-c55/mali-c55-params.c | 1 +
> 1 file changed, 1 insertion(+)
>
> 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 de0e9d898db7..33e2232ec8f5 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,
>
> 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);
> }
>
> static void mali_c55_params_digital_gain(struct mali_c55 *mali_c55,
^ permalink raw reply [flat|nested] 5+ messages in thread