linux-kernel - Re: [PATCH] brcmfmac: unlink URB when request timed out

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: <1415610506.16488.20.camel@linux-0dmf.site>
Date:	Mon, 10 Nov 2014 10:08:26 +0100
From:	Oliver Neukum <oneukum@...e.de>
To:	Mathy Vanhoef <vanhoefm@...il.com>
Cc:	brudley@...adcom.com, arend@...adcom.com, frankyl@...adcom.com,
	meuleman@...adcom.com, linville@...driver.com,
	pieterpg@...adcom.com, linux-wireless@...r.kernel.org,
	brcm80211-dev-list@...adcom.com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] brcmfmac: unlink URB when request timed out

On Sun, 2014-11-09 at 13:10 -0500, Mathy Vanhoef wrote:
> From: Mathy Vanhoef <vanhoefm@...il.com>
> 
> Unlink the submitted URB in brcmf_usb_dl_cmd if the request timed out. This
> assures the URB is never submitted twice, preventing a driver crash.

Hi,

I am afrad this patch is no good. The diagnosis is good,
but the fix introduces serious problems.

> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/usb.c b/drivers/net/wireless/brcm80211/brcmfmac/usb.c
> index 5265aa7..1bc7858 100644
> --- a/drivers/net/wireless/brcm80211/brcmfmac/usb.c
> +++ b/drivers/net/wireless/brcm80211/brcmfmac/usb.c
> @@ -738,10 +738,12 @@ static int brcmf_usb_dl_cmd(struct brcmf_usbdev_info *devinfo, u8 cmd,
>  		goto finalize;
>  	}
>  
> -	if (!brcmf_usb_ioctl_resp_wait(devinfo))
> +	if (!brcmf_usb_ioctl_resp_wait(devinfo)) {
> +		usb_unlink_urb(devinfo->ctl_urb);

This is the asynchronous unlink. You have no guarantee it is finished
after this point.

>  		ret = -ETIMEDOUT;
> -	else
> +	} else {
>  		memcpy(buffer, tmpbuf, buflen);
> +	}
>  
>  finalize:
>  	kfree(tmpbuf);

Which means that you are freeing memory that may still be used by DMA
at this time.
In addition you have no guarantee that the unlink is indeed finished
by the time the URB is reused.
If you wish to take this approach you better forget about this URB
and allocate a new one and free the buffer from the callback.

	Regards
		Oliver

-- 
Oliver Neukum <oneukum@...e.de>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/