linux-kernel - Re: [PATCH] uio/gen-pci: don't enable interrupts in ISR

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:	Mon, 8 Aug 2011 09:40:50 +0300
From:	"Michael S. Tsirkin" <mst@...hat.com>
To:	Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc:	Chris Wright <chrisw@...hat.com>,
	"Hans J. Koch" <hjk@...sjkoch.de>,
	Jesse Barnes <jbarnes@...tuousgeek.org>,
	Greg Kroah-Hartman <gregkh@...e.de>,
	Anthony Foiani <anthony.foiani@...il.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] uio/gen-pci: don't enable interrupts in ISR

On Fri, Aug 05, 2011 at 09:18:42PM +0200, Sebastian Andrzej Siewior wrote:
> * Michael S. Tsirkin | 2011-08-05 00:04:13 [+0300]:
> 
> >> Looking at it further here is only one spot where the lock is hold. So
> >> giving the fact that an ISR is not reentrant and is not executed on two
> >> cpus at the same time why do we need a lock here?
> >
> >I'm not sure anymore. I think the idea was to use
> >it for synchronization down the road somehow,
> >but it never materialized. Let's drop that lock completely.
> Okay. So I post antoher patch with this lock removed and cc stable.
> 
> >> So there is _no_ protection
> >> against read-modify-write of user vs kernel so even that
> >> pci_block_user_cfg_access() is kinda pointless.
> >
> >I didn't get that. pci_block_user_cfg_access is to prevent
> >sysfs access while we read modify-write the command register.
> >Isn't it effective for that?
> It probably works well enough for you because you only care the one bit
> and don't change anything else in the kernel driver.
> 
> Lets assume user land changes another bit in this register:
> 
>     user                                kernel
>  read config()                             |
>    add a bit                               |
>      |                                  interrupt
>      |                                block user land
>      |                              read + clear + write
>      |                               unblock user land
>  write config back                         | 
> 
> 
> You did not *re-read* the config field after the interrupt so kernel's
> modifications are lost. So you get two interrupts accounted while only
> one happend.

So we might get an extra interrupt. This is harmless,
and the window seems small enough for this not to affect
performance. This will also never happen if
userspace is careful to make interrupt unmasking its last action.

> It seems to me that you could drop this "user block" thing
> since you never change anything outside of this command register and it
> does not stop the race.

I don't think so: if we did, we would lose userspace modifications to
other bits such as io enable, and there's no way to guess what their
values should be.

> >> pci_block_user_cfg_access() in open() + ->irqcontrol() should fix this.
> >
> >Why block in open? We don't access the device there, do we?
> Yeah. That might not work for you since you need change other values.
> 
> Sebastian
--
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/