linux-kernel - Re: [PATCH v17 2/9] counter: Add character device interface

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]

Message-ID: <YWxCtGcJ8s4gjgZj@piout.net>
Date:   Sun, 17 Oct 2021 17:35:16 +0200
From:   Alexandre Belloni <alexandre.belloni@...tlin.com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     William Breathitt Gray <vilhelm.gray@...il.com>, jic23@...nel.org,
        linux-stm32@...md-mailman.stormreply.com, kernel@...gutronix.de,
        a.fatoum@...gutronix.de, kamel.bouhara@...tlin.com,
        gwendal@...omium.org, david@...hnology.com,
        linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, syednwaris@...il.com,
        patrick.havelange@...ensium.com, fabrice.gasnier@...com,
        mcoquelin.stm32@...il.com, alexandre.torgue@...com,
        o.rempel@...gutronix.de, jarkko.nikula@...ux.intel.com,
        Dan Carpenter <dan.carpenter@...cle.com>
Subject: Re: [PATCH v17 2/9] counter: Add character device interface

On 17/10/2021 16:40:14+0200, Greg KH wrote:
> On Sun, Oct 17, 2021 at 04:02:42PM +0200, Alexandre Belloni wrote:
> > On 17/10/2021 15:50:11+0200, Greg KH wrote:
> > > Note, review of this now that it has been submitted in a pull request to
> > > me, sorry I missed this previously...
> > > 
> > > On Wed, Sep 29, 2021 at 12:15:59PM +0900, William Breathitt Gray wrote:
> > > > +static int counter_chrdev_open(struct inode *inode, struct file *filp)
> > > > +{
> > > > +	struct counter_device *const counter = container_of(inode->i_cdev,
> > > > +							    typeof(*counter),
> > > > +							    chrdev);
> > > > +
> > > > +	/* Ensure chrdev is not opened more than 1 at a time */
> > > > +	if (!atomic_add_unless(&counter->chrdev_lock, 1, 1))
> > > > +		return -EBUSY;
> > > 
> > > I understand the feeling that you wish to stop userspace from doing
> > > this, but really, it does not work.  Eventhough you are doing this
> > > correctly (you should see all the other attempts at doing this), you are
> > > not preventing userspace from having multiple processes access this
> > > device node at the same time, so please, don't even attempt to stop this
> > > from happening.
> > > 
> > > So you can drop the atomic "lock" you have here, it's not needed at all.
> > > 
> > 
> > Could you elaborate a bit here because we've had a similar thing in the
> > RTC subsystem:
> > 
> > https://elixir.bootlin.com/linux/latest/source/drivers/rtc/dev.c#L28
> 
> Yeah, that too will not work :(  Note, it does stop open from being
> called from different processes, but think of the following sequence of
> userspace calls:
> 	open()
> 	fork/exec()
> 	both processes access the file descriptor
> 
> or passing a fd across a socket?
> 
> Or duplicating the file descriptor and sending it to a different task
> (like across a socket or many other IPC ways)?
> 
> Once userspace has a file descriptor, all bets are off as to where it
> goes and what it does with it.  There's no need to try to save userspace
> from itself by preventing multiple opens when really, it doesn't stop
> anyone who really wants to do this.
> 
> If userspace does do multiple read/writes from different threads /
> processes / whatever on the same file descriptor, it gets to keep the
> pieces of the mess it causes.  It's not the kernel's job to try to
> "protect" userspace from itself here.
> 
> Look at serial/tty connections as one example of this always being the
> case.
> 
> Does that help?
> 

Thanks for the explanation, this is now clear to me.

> > And it would mean I can remove rtc->flags completely.
> 
> I think you can do that.
> 
> thanks,
> 
> greg k-h

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com