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]

Message-ID: <20110805191842.GA4224@linutronix.de>
Date:	Fri, 5 Aug 2011 21:18:42 +0200
From:	Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To:	"Michael S. Tsirkin" <mst@...hat.com>
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

* 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. 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.

>> 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/