linux-kernel - Re: [PATCH] rc: correctly handling failed allocation

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160216105454.GA7378@gofer.mess.org>
Date:	Tue, 16 Feb 2016 10:54:54 +0000
From:	Sean Young <sean@...s.org>
To:	Insu Yun <wuninsu@...il.com>
Cc:	mchehab@....samsung.com, linux-media@...r.kernel.org,
	linux-kernel@...r.kernel.org, taesoo@...ech.edu,
	yeongjin.jang@...ech.edu, insu@...ech.edu, changwoo@...ech.edu
Subject: Re: [PATCH] rc: correctly handling failed allocation

On Mon, Feb 15, 2016 at 09:33:11PM -0500, Insu Yun wrote:
> Since rc_allocate_device() uses kmalloc,
> it can returns NULL, so need to check, 
> otherwise, NULL derefenrece can be happened.

Thanks for catching that.

> Signed-off-by: Insu Yun <wuninsu@...il.com>
> ---
>  drivers/media/rc/igorplugusb.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/media/rc/igorplugusb.c b/drivers/media/rc/igorplugusb.c
> index b36e515..df37cd5 100644
> --- a/drivers/media/rc/igorplugusb.c
> +++ b/drivers/media/rc/igorplugusb.c
> @@ -191,6 +191,8 @@ static int igorplugusb_probe(struct usb_interface *intf,
>  	usb_make_path(udev, ir->phys, sizeof(ir->phys));
>  
>  	rc = rc_allocate_device();
> +	if (!rc)
> +		goto fail;

At this point, ret is not initialized but will be used in the error path.

>  	rc->input_name = DRIVER_DESC;
>  	rc->input_phys = ir->phys;
>  	usb_to_input_id(udev, &rc->input_id);
> @@ -213,6 +215,7 @@ static int igorplugusb_probe(struct usb_interface *intf,
>  	ir->rc = rc;
>  	ret = rc_register_device(rc);
>  	if (ret) {

I'm not sure how common it is to goto into another nesting level for an
error path. Also I just noticed that the code is leaking the timer in
the error path.

It might be better to put the "fail:" at the end after the last return
for the successful case, and have a goto to it after both
rc_allocate_device() and rc_register_device() in case they fail.

> +fail:
>  		dev_err(&intf->dev, "failed to register rc device: %d", ret);
>  		rc_free_device(rc);
>  		usb_free_urb(ir->urb);
> -- 
> 1.9.1

Thanks
Sean