linux-kernel - Re: [PATCH] [media] usbtv: Add error handling for usb_submit_urb in usbtv_audio

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <e92659c0-34c1-457f-b917-0c59bdc8e081@xs4all.nl>
Date:   Wed, 6 Dec 2023 10:58:11 +0100
From:   Hans Verkuil <hverkuil@...all.nl>
To:     Haoran Liu <liuhaoran14@....com>, mchehab@...nel.org
Cc:     linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] [media] usbtv: Add error handling for usb_submit_urb in
 usbtv_audio_start

Hi Haoran,

On 30/11/2023 04:37, Haoran Liu wrote:
> This patch introduces improved error handling for the usb_submit_urb call
> in the usbtv_audio_start function. Prior to this change, the function did
> not handle the scenario where usb_submit_urb could fail, potentially
> leading to inconsistent state and unreliable audio streaming.
> 
> Although the error addressed by this patch may not occur in the current
> environment, I still suggest implementing these error handling routines
> if the function is not highly time-sensitive. As the environment evolves
> or the code gets reused in different contexts, there's a possibility that
> these errors might occur. Addressing them now can prevent potential
> debugging efforts in the future, which could be quite resource-intensive.
> 
> Signed-off-by: Haoran Liu <liuhaoran14@....com>
> ---
>  drivers/media/usb/usbtv/usbtv-audio.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/usbtv/usbtv-audio.c b/drivers/media/usb/usbtv/usbtv-audio.c
> index 333bd305a4f9..81d6d54fd12c 100644
> --- a/drivers/media/usb/usbtv/usbtv-audio.c
> +++ b/drivers/media/usb/usbtv/usbtv-audio.c
> @@ -172,6 +172,7 @@ static void usbtv_audio_urb_received(struct urb *urb)
>  static int usbtv_audio_start(struct usbtv *chip)
>  {
>  	unsigned int pipe;
> +	int err;
>  	static const u16 setup[][2] = {
>  		/* These seem to enable the device. */
>  		{ USBTV_BASE + 0x0008, 0x0001 },
> @@ -216,7 +217,15 @@ static int usbtv_audio_start(struct usbtv *chip)
>  	usbtv_set_regs(chip, setup, ARRAY_SIZE(setup));
>  
>  	usb_clear_halt(chip->udev, pipe);
> -	usb_submit_urb(chip->snd_bulk_urb, GFP_ATOMIC);
> +	err = usb_submit_urb(chip->snd_bulk_urb, GFP_ATOMIC);
> +	if (err) {
> +		dev_err(&chip->udev->dev,
> +			"usb_submit_urb failed: %d\n", err);
> +		kfree(chip->snd_bulk_urb->transfer_buffer);
> +		usb_free_urb(chip->snd_bulk_urb);
> +		chip->snd_bulk_urb = NULL;
> +		return err;
> +	}

This can be improved a bit. Add a new label in the clean up path to
do the kfree, and just go to to that label.

In the error clean up part you now need to return 'err' at the end,
so make sure you also initialize 'err' to -ENOMEM to ensure the
correct error value is returned if usb_alloc_urb fails.

It's a bit cleaner that way and avoids duplicate cleanup paths.

Regards,

	Hans

>  
>  	return 0;
>