linux-kernel - Re: [PATCH] ohci-hcd: Fix race condition caused by ohci_urb_enqueue() and io_watchdog

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: <20180201.144911.2171407625301691891.shigeru.yoshida@windriver.com>
Date:   Thu, 1 Feb 2018 05:48:13 +0000
From:   "Yoshida, Shigeru" <Shigeru.Yoshida@...driver.com>
To:     "stern@...land.harvard.edu" <stern@...land.harvard.edu>
CC:     "Bai, Haiqing" <Haiqing.Bai@...driver.com>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] ohci-hcd: Fix race condition caused by
 ohci_urb_enqueue() and io_watchdog_func()

Hi Alan,

Thank you for your commenting.

On Wed, 31 Jan 2018 11:02:47 -0500, Alan Stern wrote:
>> To address above scenario, this patch introduces timer_running flag to
>> ohci_hcd structure.  Setting true to ohci->timer_running indicates
>> io_watchdog_func() is scheduled or is running.  ohci_urb_enqueue()
>> checks the flag when it schedules the watchdog (step 4 and 12 above),
>> so ohci->prev_frame_no is not overwritten while io_watchdog_func() is
>> running.
> 
> Instead of adding an extra flag variable, which has to be kept in sync 
> with the timer routine, how about defining a special sentinel value for 
> prev_frame_no?  For example:
> 
> #define IO_WATCHDOG_OFF		0xffffff00
> 
> Then whenever the timer isn't scheduled or running, set
> ohci->prev_frame_no to IO_WATCHDOG_OFF.  And instead of testing
> timer_pending(), compare prev_frame_no to this special value.
> 
> I think that approach will be slightly more robust.

It's reasonable since ohci->prev_frame_no is not used while the
watchdog timer is stopped.

I think we must choose an invalid frame number for the special
sentinel value, but I'm not sure which value is adequate for it.
Is 0xffffff00 an invalid frame number, otherwise how about simply
-1(0xffffffff)?

Thanks,
Shigeru