netdev - Re: [PATCH net v2 1/3] s390/ism: Fix locking for forwarding of IRQs and events to clients

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] [day] [month] [year] [list]

Date: Mon, 10 Jul 2023 08:45:21 +0100
From: Simon Horman <simon.horman@...igine.com>
To: Niklas Schnelle <schnelle@...ux.ibm.com>
Cc: Paolo Abeni <pabeni@...hat.com>,
	Alexandra Winter <wintera@...ux.ibm.com>,
	Wenjia Zhang <wenjia@...ux.ibm.com>,
	Heiko Carstens <hca@...ux.ibm.com>,
	Vasily Gorbik <gor@...ux.ibm.com>,
	Alexander Gordeev <agordeev@...ux.ibm.com>,
	Christian Borntraeger <borntraeger@...ux.ibm.com>,
	Sven Schnelle <svens@...ux.ibm.com>,
	"David S. Miller" <davem@...emloft.net>,
	Stefan Raspl <raspl@...ux.ibm.com>,
	Jan Karcher <jaka@...ux.ibm.com>, linux-s390@...r.kernel.org,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net v2 1/3] s390/ism: Fix locking for forwarding of IRQs
 and events to clients

On Mon, Jul 10, 2023 at 09:28:20AM +0200, Niklas Schnelle wrote:
> On Sat, 2023-07-08 at 14:36 +0100, Simon Horman wrote:
> > On Fri, Jul 07, 2023 at 12:43:57PM +0200, Niklas Schnelle wrote:
> > > The clients array references all registered clients and is protected by
> > > the clients_lock. Besides its use as general list of clients the clients
> > > array is accessed in ism_handle_irq() to forward ISM device events to
> > > clients.
> > > 
> > > While the clients_lock is taken in the IRQ handler when calling
> > > handle_event() it is however incorrectly not held during the
> > > client->handle_irq() call and for the preceding clients[] access leaving
> > > it unprotected against concurrent client (un-)registration.
> > > 
> > > Furthermore the accesses to ism->sba_client_arr[] in ism_register_dmb()
> > > and ism_unregister_dmb() are not protected by any lock. This is
> > > especially problematic as the client ID from the ism->sba_client_arr[]
> > > is not checked against NO_CLIENT and neither is the client pointer
> > > checked.
> > > 
> > > Instead of expanding the use of the clients_lock further add a separate
> > > array in struct ism_dev which references clients subscribed to the
> > > device's events and IRQs. This array is protected by ism->lock which is
> > > already taken in ism_handle_irq() and can be taken outside the IRQ
> > > handler when adding/removing subscribers or the accessing
> > > ism->sba_client_arr[]. This also means that the clients_lock is no
> > > longer taken in IRQ context.
> > > 
> > > Fixes: 89e7d2ba61b7 ("net/ism: Add new API for client registration")
> > > Signed-off-by: Niklas Schnelle <schnelle@...ux.ibm.com>
> > 
> > ...
> > 
> > > @@ -71,6 +80,7 @@ int ism_register_client(struct ism_client *client)
> > >  		list_for_each_entry(ism, &ism_dev_list.list, list) {
> > >  			ism->priv[i] = NULL;
> > >  			client->add(ism);
> > > +			ism_setup_forwarding(client, ism);
> > >  		}
> > >  	}
> > >  	mutex_unlock(&ism_dev_list.mutex);
> > 
> > ...
> > 
> > > @@ -92,6 +102,9 @@ int ism_unregister_client(struct ism_client *client)
> > >  		max_client--;
> > >  	spin_unlock_irqrestore(&clients_lock, flags);
> > >  	list_for_each_entry(ism, &ism_dev_list.list, list) {
> > > +		spin_lock_irqsave(&ism->lock, flags);
> > 
> > Hi Niklas,
> > 
> > The lock is taken here.
> > 
> > > +		/* Stop forwarding IRQs and events */
> > > +		ism->subs[client->id] = NULL;
> > >  		for (int i = 0; i < ISM_NR_DMBS; ++i) {
> > >  			if (ism->sba_client_arr[i] == client->id) {
> > >  				pr_err("%s: attempt to unregister client '%s'"
> > > @@ -101,6 +114,7 @@ int ism_unregister_client(struct ism_client *client)
> > >  				goto out;
> > 
> > But it does not appear to be released
> > (by the call to spin_unlock_irqrestore() below)
> > if goto out is called here.
> 
> Good catch. Yes I screwed this up while splitting the patch up. The
> spin_unlock_irqrestore() is there after patch 3 but should have been
> added in patch 1. As far as I can see all 3 patches have already been
> applied, otherwise I'd send a v3. Thankfully even in the in between
> state this error case can really onlt happen due to driver bugs so
> maybe it's okay?

Hi Niklas,

I also saw the patches have been accepted after I sent my previous email.
So, given that the problem is resolved by another patch in the series,
I think the situation is as good as it is going to get.