linux-kernel - Re: [PATCH] iio: buffer: remove null-checks for 'indio

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: <950bed7e142e7aa1cbd74fe15e560fe7d584af95.camel@analog.com>
Date:   Sun, 12 Apr 2020 14:31:14 +0000
From:   "Ardelean, Alexandru" <alexandru.Ardelean@...log.com>
To:     "jic23@...nel.org" <jic23@...nel.org>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "lars@...afoo.de" <lars@...afoo.de>
Subject: Re: [PATCH] iio: buffer: remove null-checks for 'indio_dev->info'

On Sun, 2020-04-12 at 14:30 +0100, Jonathan Cameron wrote:
> [External]
> 
> On Tue, 7 Apr 2020 17:59:18 +0300
> Alexandru Ardelean <alexandru.ardelean@...log.com> wrote:
> 
> > Checking for 'indio_dev->info' is an impossible condition, since an IIO
> > device should NOT be able to register without that information.
> > The iio_device_register() function won't allow an IIO device to register if
> > 'indio_dev->info' is NULL.
> > 
> > If that information somehow becomes NULL, then we're likely busted anyway
> > and we should crash the system, if we haven't already.
> > 
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@...log.com>
> I'm glad there was a comment in there to remind me of what was going on here.
> 
> This is the result of an ancient set from (I think) Lars hardening IIO
> against forced removal.
> 
> The indio_dev->info == NULL is deliberately set to true by the IIO core
> during device remove in order to deal with any inflight data.
> 
> Reference counting should ensure the device doesn't go away but we need
> to avoid actually doing anything if this occurs.  That pointer was a
> convenient option to avoid having to add an explicit flag or 'going away'.
> 
> Now, it's possible we don't need this any more due to other changes but
> I certainly don't want to remove it without that being very thoroughly
> verified!
> 

Agreed.
Thanks for the info.

Will think about this a bit later.

Thanks
Alex


> Thanks,
> 
> Jonathan
> 
> > ---
> >  drivers/iio/industrialio-buffer.c | 19 +------------------
> >  1 file changed, 1 insertion(+), 18 deletions(-)
> > 
> > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-
> > buffer.c
> > index e6fa1a4e135d..c96071bfada8 100644
> > --- a/drivers/iio/industrialio-buffer.c
> > +++ b/drivers/iio/industrialio-buffer.c
> > @@ -54,10 +54,6 @@ static bool iio_buffer_ready(struct iio_dev *indio_dev,
> > struct iio_buffer *buf,
> >  	size_t avail;
> >  	int flushed = 0;
> >  
> > -	/* wakeup if the device was unregistered */
> This comment makes it clear this isn't as 'obvious' as it might at first seem
> ;)
> 
> > -	if (!indio_dev->info)
> > -		return true;
> > -
> >  	/* drain the buffer if it was disabled */
> >  	if (!iio_buffer_is_active(buf)) {
> >  		to_wait = min_t(size_t, to_wait, 1);
> > @@ -109,9 +105,6 @@ ssize_t iio_buffer_read_outer(struct file *filp, char
> > __user *buf,
> >  	size_t to_wait;
> >  	int ret = 0;
> >  
> > -	if (!indio_dev->info)
> > -		return -ENODEV;
> > -
> >  	if (!rb || !rb->access->read)
> >  		return -EINVAL;
> >  
> > @@ -131,11 +124,6 @@ ssize_t iio_buffer_read_outer(struct file *filp, char
> > __user *buf,
> >  
> >  	add_wait_queue(&rb->pollq, &wait);
> >  	do {
> > -		if (!indio_dev->info) {
> > -			ret = -ENODEV;
> > -			break;
> > -		}
> > -
> >  		if (!iio_buffer_ready(indio_dev, rb, to_wait, n / datum_size)) {
> >  			if (signal_pending(current)) {
> >  				ret = -ERESTARTSYS;
> > @@ -171,7 +159,7 @@ __poll_t iio_buffer_poll(struct file *filp,
> >  	struct iio_dev *indio_dev = filp->private_data;
> >  	struct iio_buffer *rb = indio_dev->buffer;
> >  
> > -	if (!indio_dev->info || rb == NULL)
> > +	if (rb == NULL)
> >  		return 0;
> >  
> >  	poll_wait(filp, &rb->pollq, wait);
> > @@ -1100,11 +1088,6 @@ int iio_update_buffers(struct iio_dev *indio_dev,
> >  		goto out_unlock;
> >  	}
> >  
> > -	if (indio_dev->info == NULL) {
> > -		ret = -ENODEV;
> > -		goto out_unlock;
> > -	}
> > -
> >  	ret = __iio_update_buffers(indio_dev, insert_buffer, remove_buffer);
> >  
> >  out_unlock: