linux-kernel - Re: [PATCH] staging: pi433: fix race condition in pi433

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: <20180619134220.GA2396@hle-laptop.local>
Date:   Tue, 19 Jun 2018 09:42:20 -0400
From:   Hugo Lefeuvre <hle@....eu.com>
To:     Dan Carpenter <dan.carpenter@...cle.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        devel@...verdev.osuosl.org,
        Marcus Wolf <linux@...f-entwicklungen.de>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: pi433: fix race condition in pi433_open

> > It would be great to get rid of this counter, indeed. But how to do it
> > properly without breaking things ? It seems to be useful to me...
> 
> These things are refcounted so you can't unload the module while a file
> is open.  When we do an open it does a cdev_get().  When we call the
> delete_module syscall, it checks the refcount in try_stop_module() ->
> try_release_module_ref().  It will still let us force a release but
> at that point you're expecting use after frees.

Then I'd like to understand why this counter was introduced if remove
always gets executed after the last release call returned. This was
probably unclear to the original developer.

This TODO seems to be related to this misunderstanding too:

 890         /* TODO? guard against device removal before, or while,
 891          * we issue this ioctl. --> device_get()
 892          */

Device removal can't happen before or during the ioctl execution.

> > Really ? device->spi is NULL-ed in remove() so that operations on
> > remaining fds can detect remove() was already called and free remaining
> > resources:
> >  
> > 1296         /* make sure ops on existing fds can abort cleanly */
> > 1297         device->spi = NULL;
> 
> That's when we're unloading the module so there aren't any users left.

I'll submit an updated version of my patch getting rid of the counter
and addressing the remaining race conditions.

Thanks !

Regards,
 Hugo

-- 
             Hugo Lefeuvre (hle)    |    www.owl.eu.com
4096/ 9C4F C8BF A4B0 8FC5 48EB 56B8 1962 765B B9A8 BACA

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)