Re: [PATCH] media: dvb-core: fix OOB read in dtv_property_process_get()

[Slawomir Stepien <sst@poczta.fm>]

Hi

On paź 09, 2026 19:53, Mirza Ishan Beg wrote:
> dtv_property_process_get() switches on 'tvp->cmd' and, in each case,
> writes a different member of the union 'u' in struct dtv_property.
> 'u.data' carries the scalar commands, while u.st carries the
> 'DTV_STAT_*' commands, and u.buffer only DTV_ENUM_DELSYS.
> 
> After the switch, a single shared dev_dbg() reads 'u.buffer.len'
> and 'u.buffer.data' via %*ph, regardless of which member the case
> wrote.
> 
> struct 'dtv_property' is 76 bytes and union 'u' begins at offset 16:
> 
>     u.data      : 4 bytes at 16
>     u.st        : 37 bytes at 16
>     u.buffer    : 56 bytes at 16 ('data[32]' at 16-47, 'len' at 48-51)
> 
> For commands that only write 'u.data', 'u.buffer.len' is never set by
> the kernel and keeps whatever userspace supplied to FE_GET_PROPERTY,
> because dvb_get_property() copies the array in with
> memdup_array_user() and does not clear the union. A large value (the
> reproducer uses 1163001896) makes vsprintf() emit "field width ...
> out of range" and hex_string() clamp the length to 64 bytes. The
> read starts at 'u.buffer.data', offset 16, so 64 bytes reach offset 80
> and KASAN reports a slab-out-of-bounds read past the 76-byte object.
> 
> The DTV_STAT_* cases hit the same shared print. 'u.buffer.len' overlaps
> 'u.st.stat[3].uvalue', so the debug output is meaningless even when the
> read stays in bounds. The same applies to the clamp added by
> 60f0618d157b, which writes to 'u.buffer.len' and therefore into the
> just-written stats payload.
> 
> Fix by storing the point of writing which union member each case
> populated, and by branching the final dev_dbg() on that stored point.
> Add a local enum 'dtv_get_shape' with values 'SHAPE_DATA', 'SHAPE_ST'
> and 'SHAPE_BUFFER', initialised to 'SHAPE_DATA' and set in the
> 'DTV_ENUM_DELSYS' and 'DTV_STAT_*' cases. The final print then reads only
> the member the case actually wrote, and every branch stays inside the
> 76-byte object. The clamp and the now-unused 'len' variable are removed.
> 
> Fixes: 60f0618d157b ("media: dvb-core: frontend: make GET/SET safer")
> Suggested-by: Slawomir Stepien <sst@poczta.fm>
> Reported-by: syzbot+6102f1c3a0ea8073fd64@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=6102f1c3a0ea8073fd64
> Link: https://lore.kernel.org/all/2a5ea27d-445e-4ffe-9447-9d7bfccd4a36@mail.kernel.org/
> Signed-off-by: Mirza Ishan Beg <seedandsyntax@gmail.com>

Tested-by: Slawomir Stepien <sst@poczta.fm>
Reviewed-by: Slawomir Stepien <sst@poczta.fm>

Thanks for adding me! I think this patch is better than the v2 generated by syzkaller.

> ---
>  drivers/media/dvb-core/dvb_frontend.c | 73 ++++++++++++++-------------
>  1 file changed, 38 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
> index 0286da57f..f4b599a1a 100644
> --- a/drivers/media/dvb-core/dvb_frontend.c
> +++ b/drivers/media/dvb-core/dvb_frontend.c
> @@ -1342,13 +1342,19 @@ static int dtv_get_frontend(struct dvb_frontend *fe,
>  static int dvb_frontend_handle_ioctl(struct file *file,
>  				     unsigned int cmd, void *parg);
>  
> +enum dtv_get_shape {
> +	SHAPE_DATA,
> +	SHAPE_ST,
> +	SHAPE_BUFFER,
> +};
> +
>  static int dtv_property_process_get(struct dvb_frontend *fe,
>  				    const struct dtv_frontend_properties *c,
>  				    struct dtv_property *tvp,
>  				    struct file *file)
>  {
> +	enum dtv_get_shape shape = SHAPE_DATA;
>  	int ncaps;
> -	unsigned int len = 1;
>  
>  	switch (tvp->cmd) {
>  	case DTV_ENUM_DELSYS:
> @@ -1358,7 +1364,7 @@ static int dtv_property_process_get(struct dvb_frontend *fe,
>  			ncaps++;
>  		}
>  		tvp->u.buffer.len = ncaps;
> -		len = ncaps;
> +		shape = SHAPE_BUFFER;
>  		break;
>  	case DTV_FREQUENCY:
>  		tvp->u.data = c->frequency;
> @@ -1536,51 +1542,35 @@ static int dtv_property_process_get(struct dvb_frontend *fe,
>  	/* Fill quality measures */
>  	case DTV_STAT_SIGNAL_STRENGTH:
>  		tvp->u.st = c->strength;
> -		if (tvp->u.buffer.len > MAX_DTV_STATS * sizeof(u32))
> -			tvp->u.buffer.len = MAX_DTV_STATS * sizeof(u32);
> -		len = tvp->u.buffer.len;
> +		shape = SHAPE_ST;
>  		break;
>  	case DTV_STAT_CNR:
>  		tvp->u.st = c->cnr;
> -		if (tvp->u.buffer.len > MAX_DTV_STATS * sizeof(u32))
> -			tvp->u.buffer.len = MAX_DTV_STATS * sizeof(u32);
> -		len = tvp->u.buffer.len;
> +		shape = SHAPE_ST;
>  		break;
>  	case DTV_STAT_PRE_ERROR_BIT_COUNT:
>  		tvp->u.st = c->pre_bit_error;
> -		if (tvp->u.buffer.len > MAX_DTV_STATS * sizeof(u32))
> -			tvp->u.buffer.len = MAX_DTV_STATS * sizeof(u32);
> -		len = tvp->u.buffer.len;
> +		shape = SHAPE_ST;
>  		break;
>  	case DTV_STAT_PRE_TOTAL_BIT_COUNT:
>  		tvp->u.st = c->pre_bit_count;
> -		if (tvp->u.buffer.len > MAX_DTV_STATS * sizeof(u32))
> -			tvp->u.buffer.len = MAX_DTV_STATS * sizeof(u32);
> -		len = tvp->u.buffer.len;
> +		shape = SHAPE_ST;
>  		break;
>  	case DTV_STAT_POST_ERROR_BIT_COUNT:
>  		tvp->u.st = c->post_bit_error;
> -		if (tvp->u.buffer.len > MAX_DTV_STATS * sizeof(u32))
> -			tvp->u.buffer.len = MAX_DTV_STATS * sizeof(u32);
> -		len = tvp->u.buffer.len;
> +		shape = SHAPE_ST;
>  		break;
>  	case DTV_STAT_POST_TOTAL_BIT_COUNT:
>  		tvp->u.st = c->post_bit_count;
> -		if (tvp->u.buffer.len > MAX_DTV_STATS * sizeof(u32))
> -			tvp->u.buffer.len = MAX_DTV_STATS * sizeof(u32);
> -		len = tvp->u.buffer.len;
> +		shape = SHAPE_ST;
>  		break;
>  	case DTV_STAT_ERROR_BLOCK_COUNT:
>  		tvp->u.st = c->block_error;
> -		if (tvp->u.buffer.len > MAX_DTV_STATS * sizeof(u32))
> -			tvp->u.buffer.len = MAX_DTV_STATS * sizeof(u32);
> -		len = tvp->u.buffer.len;
> +		shape = SHAPE_ST;
>  		break;
>  	case DTV_STAT_TOTAL_BLOCK_COUNT:
>  		tvp->u.st = c->block_count;
> -		if (tvp->u.buffer.len > MAX_DTV_STATS * sizeof(u32))
> -			tvp->u.buffer.len = MAX_DTV_STATS * sizeof(u32);
> -		len = tvp->u.buffer.len;
> +		shape = SHAPE_ST;
>  		break;
>  	default:
>  		dev_dbg(fe->dvb->device,
> @@ -1588,15 +1578,28 @@ static int dtv_property_process_get(struct dvb_frontend *fe,
>  			__func__, tvp->cmd);
>  		return -EINVAL;
>  	}
> -
> -	if (len < 1)
> -		len = 1;
> -
> -	dev_dbg(fe->dvb->device,
> -		"%s: GET cmd 0x%08x (%s) len %d: %*ph\n",
> -		__func__, tvp->cmd, dtv_cmd_name(tvp->cmd),
> -		tvp->u.buffer.len, tvp->u.buffer.len, tvp->u.buffer.data);
> -
> +	switch (shape) {
> +	case SHAPE_BUFFER:
> +		dev_dbg(fe->dvb->device,
> +			"%s: GET cmd 0x%08x (%s) len %d: %*ph\n",
> +			__func__, tvp->cmd, dtv_cmd_name(tvp->cmd),
> +			tvp->u.buffer.len, tvp->u.buffer.len,
> +			tvp->u.buffer.data);
> +		break;
> +	case SHAPE_ST:
> +		dev_dbg(fe->dvb->device,
> +			"%s: GET cmd 0x%08x (%s) len %d: %*ph\n",
> +			__func__, tvp->cmd, dtv_cmd_name(tvp->cmd),
> +			tvp->u.st.len, (int)sizeof(tvp->u.st),
> +			&tvp->u.st);
> +		break;
> +	case SHAPE_DATA:
> +		dev_dbg(fe->dvb->device,
> +			"%s: GET cmd 0x%08x (%s) = 0x%08x\n",
> +			__func__, tvp->cmd, dtv_cmd_name(tvp->cmd),
> +			tvp->u.data);
> +		break;
> +	}
>  	return 0;
>  }
>  

-- 
Slawomir Stepien