Re: [PATCH v9 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 3 potential issue(s) to consider:

New issues:
- [Low] Removal of the `iov` array declaration breaks compilation because it is still required for CRC32 computation.
- [Low] The `iov` parameter in `dvb_net_ule_check_crc()` is left entirely unused, contradicting the commit message claim.

Pre-existing issues:
- [High] Missing length checks in ULE extension header parsing allows an arbitrary out-of-bounds heap memory read.
--

commit 8bbfcd7d5129e4e8e1d24fd8e69989a6dcdf34d2
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 and its Kconfig definition to eliminate severe lockless race
conditions and clean up unused function parameters.

> diff --git a/drivers/media/dvb-core/dvb_net.c b/drivers/media/dvb-core/dvb_net.c
> index a2159b2bc1766..29f99dfe282a3 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);
>  
> -	#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: Low]
Does the iov parameter in dvb_net_ule_check_crc() need to be kept in the
function signature?

The commit message mentions cleaning up unused function parameters, but
iov remains in the signature while being entirely unused inside the function
now that the debug code is removed.

[ ... ]

> @@ -823,12 +783,6 @@ static void dvb_net_ule(struct net_device *dev, const u8 *buf, size_t buf_len)
>  			__be16 ulen = htons(h.priv->ule_sndu_len);
>  			__be16 utype = htons(h.priv->ule_sndu_type);
>  			const u8 *tail;
> -			struct kvec iov[3] = {
> -				{ &ulen, sizeof ulen },
> -				{ &utype, sizeof utype },
> -				{ h.priv->ule_skb->data,
> -				  h.priv->ule_skb->len - 4 }
> -			};
>  			u32 ule_crc = ~0L, expected_crc;
>  			if (h.priv->ule_dbit) {
>  				/* Set D-bit for CRC32 verification,

[Severity: Low]
Does the removal of the iov array break compilation?

Looking further down in dvb_net_ule(), iov is still passed to both
iov_crc32() and dvb_net_ule_check_crc():

			ule_crc = iov_crc32(ule_crc, iov, 3);
			...
			dvb_net_ule_check_crc(&h, iov, ule_crc, expected_crc);

Removing the declaration causes a compile error.

[Severity: High]
This is a pre-existing issue, but can the ULE extension header parsing enter
an infinite out-of-bounds read?

When handle_ule_extensions() iteratively calls handle_one_ule_extension()
on incoming packets:

drivers/media/dvb-core/dvb_net.c:handle_ule_extensions() {
	...
	do {
		l = handle_one_ule_extension( p );
		if (l < 0)
			return l;
		...
	} while (p->ule_sndu_type < ETH_P_802_3_MIN);
	...
}

Inside handle_one_ule_extension(), p->ule_next_hdr is advanced without
verifying if the pointer exceeds the bounds of the skb->data payload:

drivers/media/dvb-core/dvb_net.c:handle_one_ule_extension() {
	...
	} else {
		/* Optional extension header.  Calculate the length. */
		ext_len = hlen << 1;
		...
		p->ule_next_hdr += ext_len;
		p->ule_sndu_type = ntohs( *(__be16 *)(p->ule_next_hdr-2) );
	...
}

Could a malicious DVB stream payload with untrusted lengths force the kernel
to continuously read past the skb->data payload until a non-extension header
type is found or a panic occurs?

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