Re: [PATCH v7 3/3] media: dvb-core: extirpate defective and unsafe ULE debug block

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The patch silently includes an unrelated modification in the LED subsystem Kconfig.
- [Low] The commit removes the `DVB_ULE_DEBUG` code blocks from `dvb_net.c` but fails to remove the corresponding Kconfig option from `drivers/media/dvb-core/Kconfig`.
- [Low] The parameter `iov` in `dvb_net_ule_check_crc()` is now unused after the removal of the debug block.

Pre-existing issues:
- [Critical] The ULE SNDU payload parser lacks robust bounds checking on user-controlled length fields, allowing an attacker to underflow `skb->len` via `skb_pull()` and read/write out-of-bounds memory.
- [High] Dereferencing a casted `unsigned short *` pointer from an unaligned byte stream causes a kernel panic due to alignment faults on strict architectures.
--

commit fe63e38a3f8932fddc46f48a9206976093b6c1c6
Author: Breno Rodrigues Alves <breno3011alves@gmail.com>

    media: dvb-core: extirpate defective and unsafe ULE debug block

    Completely purge the unmaintained DVB_ULE_DEBUG code block from
    dvb_net.c to eliminate severe lockless race conditions and buffer overflow
    vulnerabilities on global network state variables.

>     Completely purge the unmaintained DVB_ULE_DEBUG code block from
>     dvb_net.c to eliminate severe lockless race conditions and buffer overflow
>     vulnerabilities on global network state variables.

[Severity: Low]
Did this commit miss removing the config DVB_ULE_DEBUG definition from
drivers/media/dvb-core/Kconfig? Leaving it in makes the Kconfig option
selectable but has no effect.

> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index d0d3182236be4..800fbd56093ec 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -873,7 +873,6 @@ config LEDS_UPBOARD
>  	  This driver can also be built as a module. If so the module will be
>  	  called leds-upboard.
>  
> -comment "LED driver for blink(1) USB RGB LED is under Special HID drivers (HID_THINGM)"
>  
>  config LEDS_BLINKM
>  	tristate "LED support for the BlinkM I2C RGB LED"

[Severity: Medium]
Is this unrelated modification in the LED subsystem Kconfig intentional? It
doesn't seem to be documented in the commit message and introduces an
undocumented side effect in an unrelated subsystem.

> diff --git a/drivers/media/dvb-core/dvb_net.c b/drivers/media/dvb-core/dvb_net.c
> index a2159b2bc1766..ae0073b949a50 100644
> --- a/drivers/media/dvb-core/dvb_net.c
> +++ b/drivers/media/dvb-core/dvb_net.c

[ ... ]

> @@ -659,23 +636,6 @@ static void dvb_net_ule_check_crc(struct dvb_net_ule_handle *h,
>  			h->ts_remain > 2 ?
>  				*(unsigned short *)h->from_where : 0);

[Severity: High]
This isn't a bug introduced by this patch, but does the unaligned pointer cast
here cause a fatal alignment fault on strict architectures?

In drivers/media/dvb-core/dvb_net.c:dvb_net_ule_check_crc(), when an attacker
sends a ULE packet with an invalid CRC, the error path unsafely casts the
unaligned h->from_where pointer to an unsigned short * and dereferences it:

    h->ts_remain > 2 ? *(unsigned short *)h->from_where : 0);

This can cause a kernel panic (Denial of Service) on architectures like SPARC
or older ARM that do not support unaligned accesses.

[Severity: Low]
With the removal of the DVB_ULE_DEBUG block below, is the parameter iov in
dvb_net_ule_check_crc() now unused?

drivers/media/dvb-core/dvb_net.c:dvb_net_ule_check_crc(struct dvb_net_ule_handle *h,
                                  struct kvec iov[3],
                                  u32 ule_crc, u32 expected_crc)

> -	#ifdef DVB_ULE_DEBUG
> -		hexdump(iov[0].iov_base, iov[0].iov_len);
> -		hexdump(iov[1].iov_base, iov[1].iov_len);
> -		hexdump(iov[2].iov_base, iov[2].iov_len);
> -
> -		if (ule_where == ule_hist) {
> -			hexdump(&ule_hist[98*TS_SZ], TS_SZ);
> -			hexdump(&ule_hist[99*TS_SZ], TS_SZ);
> -		} else if (ule_where == &ule_hist[TS_SZ]) {
> -			hexdump(&ule_hist[99*TS_SZ], TS_SZ);
> -			hexdump(ule_hist, TS_SZ);
> -		} else {
> -			hexdump(ule_where - TS_SZ - TS_SZ, TS_SZ);
> -			hexdump(ule_where - TS_SZ, TS_SZ);
> -		}
> -		ule_dump = 1;
> -	#endif
>  
>  		h->dev->stats.rx_errors++;
>  		h->dev->stats.rx_crc_errors++;

[Severity: Critical]
This is a pre-existing issue, but does the ULE SNDU payload parser lack
robust bounds checking on user-controlled length fields?

In drivers/media/dvb-core/dvb_net.c:dvb_net_ule_new_payload(), the minimum
length check fails to account for the 6-byte MAC address when ule_dbit is 0:

    if (h->priv->ule_sndu_len < 5) {

If an attacker sends a malformed packet with ule_dbit == 0 and a payload
length of 5, after the 4-byte CRC is removed in dvb_net_ule_check_crc(),
skb->len becomes 1. Then skb_pull(h->priv->ule_skb, ETH_ALEN) subtracts 6,
causing skb->len to underflow:

drivers/media/dvb-core/dvb_net.c:dvb_net_ule_check_crc() {
    ...
    if (!h->priv->ule_dbit) {
        ...
        skb_pull(h->priv->ule_skb, ETH_ALEN);
    ...
}

Similarly, in handle_ule_extensions(), the loop accumulates lengths without
validating they fit in the payload buffer:

drivers/media/dvb-core/dvb_net.c:handle_ule_extensions() {
    ...
        total_ext_len += l;
    ...
}

This unbounded length is then passed to skb_pull() in dvb_net_ule_check_crc(),
again causing an skb->len underflow:

drivers/media/dvb-core/dvb_net.c:dvb_net_ule_check_crc() {
    ...
        skb_pull(h->priv->ule_skb, l);
    ...
}

This allows an attacker to underflow skb->len and read or write
out-of-bounds memory, leading to network stack memory corruption.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260905124434.6163-1-breno3011alves@gmail.com?part=3