Re: [PATCH v3] media: gspca: Handle SENSOR_HV7131R

[Hans Verkuil <hverkuil+cisco@kernel.org>]

Hi Philipp,

On 4/20/26 15:54, Philipp Matthias Hahn wrote:
> I found an old USB webcam 0c45:602d Microdia VideoCAM ExpressII. `vlc`
> triggered an OOPS as soon as I opened the device:
> 
>> BUG: kernel NULL pointer dereference, address: 0000000000000068
>> #PF: supervisor read access in kernel mode
>> #PF: error_code(0x0000) - not-present page
>> PGD 0 P4D 0
>> Oops: Oops: 0000 [#1] PREEMPT SMP NOPTI
>> CPU: 8 UID: 1000 PID: 19655 Comm: vlc Tainted: G O 7.0.0+ #1 Debian
>> Tainted: [O]=OOT_MODULE
>> RIP: 0010:do_autogain+0x7d/0x100 [gspca_sonixb]
>> Code: 74 21 0f af 90 c4 00 00 00 89 d0 48 63 d2 48 69 d2 09 04 02 81
>> 48 c1 ea 20 01 c2 c1 f8 1f c1 fa 06 29 c2 48 8b 83 48 06 00 00 <48>
>> 81 78 68 f3 01 00 00 7f 34 48 89 df e8 41 f1 3b 00 85 c0 74 07
> 
> (The out-of-tree-module is v4l2-loopback.)
> 
> Adding addition debug information I found out, that the cam is based on
> an SENSOR_HV7131R:
> 
>> gspca_main: sonixb-2.14.0 probing 0c45:602d
>> sonixb 1-3:1.0: sd_config(sensor=01 bridge=00)
> 
> In case of an SENSOR_HV7131R `gspca_dev->exposure` is not setup.
> Enabling auto-gain will result in the above OOPS.
> 
> 1. Check for `gspca_dev->exposure != NULL` before dereferencing it.
> 
> Even after that there's 2nd OOPS:
> 
>> BUG: kernel NULL pointer dereference, address: 0000000000000034
>> #PF: supervisor read access in kernel mode
>> #PF: error_code(0x0000) - not-present page
>> PGD 0 P4D 0
>> Oops: Oops: 0000 [#1] SMP NOPTI
>> CPU: 1 UID: 1000 PID: 709 Comm: vlc Tainted: G E 7.0.0+ #6 Debian
>> Tainted: [E]=UNSIGNED_MODULE
>> RIP: 0010:v4l2_ctrl_g_ctrl+0x36/0x80 [videodev]
>> Code: 20 65 48 8b 05 63 17 b1 c8 48 89 44 24 18 31 c0 c7 44 24 14 00
>> 00 00 00 48 c7 44 24 04 00 00 00 00 48 c7 44 24 0c 00 00 00 00 <f6>
>> 47 34 20 74 2e 48 8d 74 24 04 c7 44 24 10 00 00 00 00 e8 72 fd
> 
> This is caused by v4l2_ctrl_g_ctrl() dereferencing gspca_dev->autogain,
> which remains NULL as gspca_dev->exposure is NULL.
> 
> Check for `gspca_dev->autogain != NULL` before dereferencing it via
> gspca_expo_autogain().

I see no sign of adding that check in gspca_expo_autogain(): I think you
missed that in your patch.

> 
> Signed-off-by: Philipp Matthias Hahn <pmhahn@pmhahn.de>
> ---
>  drivers/media/usb/gspca/sonixb.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/usb/gspca/sonixb.c b/drivers/media/usb/gspca/sonixb.c
> index 4d655e2da9cb..09725d62d904 100644
> --- a/drivers/media/usb/gspca/sonixb.c
> +++ b/drivers/media/usb/gspca/sonixb.c
> @@ -900,11 +900,11 @@ static void do_autogain(struct gspca_dev *gspca_dev)
>  	if (sd->brightness)
>  		desired_avg_lum = sd->brightness->val * desired_avg_lum / 127;
>  
> -	if (gspca_dev->exposure->maximum < 500) {
> +	if (gspca_dev->exposure && gspca_dev->exposure->maximum < 500) {
>  		if (gspca_coarse_grained_expo_autogain(gspca_dev, avg_lum,
>  				desired_avg_lum, deadzone))
>  			sd->autogain_ignore_frames = AUTOGAIN_IGNORE_FRAMES;
> -	} else {
> +	} else if (gspca_dev->autogain) {
>  		int gain_knee = (s32)gspca_dev->gain->maximum * 9 / 10;
>  		if (gspca_expo_autogain(gspca_dev, avg_lum, desired_avg_lum,
>  				deadzone, gain_knee, sd->exposure_knee))
> @@ -927,6 +927,9 @@ static int sd_config(struct gspca_dev *gspca_dev,
>  	sd->sensor = id->driver_info >> 8;
>  	sd->bridge = id->driver_info & 0xff;
>  
> +	dev_info(gspca_dev->v4l2_dev.dev, "%s(sensor=%02x bridge=%02x)\n",
> +		 __func__, sd->sensor, sd->bridge);
> +
>  	cam = &gspca_dev->cam;
>  	if (!(sensor_data[sd->sensor].flags & F_SIF)) {
>  		cam->cam_mode = vga_mode;
> @@ -958,7 +961,7 @@ static int sd_s_ctrl(struct v4l2_ctrl *ctrl)
>  
>  	gspca_dev->usb_err = 0;
>  
> -	if (ctrl->id == V4L2_CID_AUTOGAIN && ctrl->is_new && ctrl->val) {
> +	if (ctrl->id == V4L2_CID_AUTOGAIN && ctrl->is_new && ctrl->val && gspca_dev->exposure) {

Is this really needed? We should only be able to get here if the AUTOGAIN
control was added, and that's only added if the EXPOSURE control is also
present.

Does it still crash if you drop this change?

>  		/* when switching to autogain set defaults to make sure
>  		   we are on a valid point of the autogain gain /
>  		   exposure knee graph, and give this change time to
> @@ -976,7 +979,8 @@ static int sd_s_ctrl(struct v4l2_ctrl *ctrl)
>  		setbrightness(gspca_dev);
>  		break;
>  	case V4L2_CID_AUTOGAIN:
> -		if (gspca_dev->exposure->is_new || (ctrl->is_new && ctrl->val))
> +		if ((gspca_dev->exposure && gspca_dev->exposure->is_new) ||
> +		    (ctrl->is_new && ctrl->val))

Same here.

>  			setexposure(gspca_dev);
>  		if (gspca_dev->gain->is_new || (ctrl->is_new && ctrl->val))
>  			setgain(gspca_dev);

Regards,

	Hans