linux-kernel - Re: [PATCH] drivers: usb: gadget: udc: remove pointer dereference after free

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]

Date:   Wed, 08 Feb 2017 21:37:19 +0200
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     "Gustavo A. R. Silva" <garsilva@...eddedor.com>,
        gregkh@...uxfoundation.org, balbi@...nel.org, bhelgaas@...gle.com,
        heikki.krogerus@...ux.intel.com, mail@...oabal.eu,
        mina86@...a86.com
Cc:     linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drivers: usb: gadget: udc: remove pointer dereference
 after free

On Wed, 2017-02-08 at 13:15 -0600, Gustavo A. R. Silva wrote:
> Remove pointer dereference and write after free.

It's wrong description. There is no write after free. The memory is
still in pool and one may access it. Though the access is *formally*
illegal.

Code itself looks interesting.

> 
> Addresses-Coverity-ID: 1091173
> Signed-off-by: Gustavo A. R. Silva <garsilva@...eddedor.com>
> ---
>  drivers/usb/gadget/udc/pch_udc.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/udc/pch_udc.c
> b/drivers/usb/gadget/udc/pch_udc.c
> index a97da64..8a365aa 100644
> --- a/drivers/usb/gadget/udc/pch_udc.c
> +++ b/drivers/usb/gadget/udc/pch_udc.c
> @@ -1523,7 +1523,6 @@ static void pch_udc_free_dma_chain(struct
> pch_udc_dev *dev,
>  		td = phys_to_virt(addr);
>  		addr2 = (dma_addr_t)td->next;
>  		pci_pool_free(dev->data_requests, td, addr);
> -		td->next = 0x00;

I think the better fix is to move this line before pci_pool_free() call.
I dunno those td->next = 0x00; make any sense there.

Is it done under some lock / serialization?

>  		addr = addr2;
>  	}
>  	req->chain_len = 1;

-- 
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Intel Finland Oy