linux-kernel - Re: [PATCH] spi: spidev: add exclusive bus access lock via ioctls

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, 14 Aug 2017 19:38:57 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Vikram N <vicky773@...il.com>
Cc:     linux-spi@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] spi: spidev: add exclusive bus access lock via ioctls

On Sat, Aug 12, 2017 at 05:24:03PM +0530, Vikram N wrote:

>  	else
> -		status = spi_sync(spi, message);
> +		status = spidev->bus_locked ? spi_sync_locked(spi, message) :
> +				spi_sync(spi, message);

Please don't abuse the ternery operator, people need to be able to read
the code.

> +	case SPI_IOC_BUS_LOCK:
> +		spi_bus_lock(spi->master);
> +		spidev->bus_locked = true;
> +		break;
> +
> +	case SPI_IOC_BUS_UNLOCK:
> +		spi_bus_unlock(spi->master);
> +		spidev->bus_locked = false;
> +		break;

I'm not super convinced that this API is a good idea in general - it
seems extremely niche to be using multiple userspace programs that don't
need to coordinate at all except for a single lock (which they will all
need to use to avoid just bouncing off with errors).  That all seems
very narrow.

I'm also worried that even if there is such a use case this code is very
fragile as it stands.  If an application crashes then nothing will free
a lock it holds and any application can through simple error drop locks
that are supposed to be held by other applications.  This isn't going to
be terribly robust.

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