netdev - Re: [PATCH V1 net 1/3] net: ena: Prevent reset after device destruction

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:   Thu, 13 Aug 2020 15:51:46 +0300
From:   Shay Agroskin <shayagr@...zon.com>
To:     Jakub Kicinski <kuba@...nel.org>
CC:     <davem@...emloft.net>, <netdev@...r.kernel.org>, <dwmw@...zon.com>,
        <zorik@...zon.com>, <matua@...zon.com>, <saeedb@...zon.com>,
        <msw@...zon.com>, <aliguori@...zon.com>, <nafea@...zon.com>,
        <gtzalik@...zon.com>, <netanel@...zon.com>, <alisaidi@...zon.com>,
        <benh@...zon.com>, <akiyano@...zon.com>, <sameehj@...zon.com>,
        <ndagan@...zon.com>
Subject: Re: [PATCH V1 net 1/3] net: ena: Prevent reset after device destruction

Jakub Kicinski <kuba@...nel.org> writes:

> On Wed, 12 Aug 2020 13:10:57 +0300 Shay Agroskin wrote:
>> This patch also removes the destruction of the timer and reset 
>> services
>> from ena_remove() since the timer is destroyed by the 
>> destruction
>> routine and the reset work is handled by this patch.
>
> You'd still have a use after free if the work runs after the 
> device is
> removed. I think cancel_work_sync() gotta stay.

Hi, thank you for reviewing the patch. Short answer: I verified 
that the ENA_FLAG_TRIGGER_RESET flag cannot be set after 
ena_destroy_device() finishes its execution.

Long answer:
The ena_destroy_device() function is called with rtnl_lock() held, 
so it cannot run in parallel with the reset function. Also the 
destroy function clears the bit ENA_FLAG_TRIGGER_RESET without 
which the reset function just exits without doing anything.

A problem can then only happen when some routine sets the 
ENA_FLAG_TRIGGER_RESET bit before the reset function is executed, 
the following describes all functions from which this bit can be 
set:

- check_* functions: these function are called from the timer 
  routine which is destroyed in ena_destroy_device(), so by the 
  time the rtnl_lock() released, the bit is cleared

- napi related functions (io_poll, xdp_io_poll, validate_rx_req_id 
  etc.): the napi is de-registered in ena_destroy_device(), so 
  none of these functions is called after destroying the device.

- xmit functions (ena_xmit_common, ena_tx_timeout): the device is 
  brought down and all its RX/TX resources are freed before 
  releasing the lock.

These are all the occurrences I found. Without this bit set, the 
reset function would fail the 'if' check in this patch, and exit 
without doing anything. Destroying the reset function explicitly 
won't help since by the time we do it, the function can already be 
executed.