| 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
| ||
|
Date: Sun, 04 Feb 2018 21:22:47 +0000
From: Jonathan Cameron <jic23@...23.retrosnub.co.uk>
To: Shreeya Patel <shreeya.patel23498@...il.com>,
Jonathan Cameron <jic23@...nel.org>
CC: lars@...afoo.de, Michael.Hennerich@...log.com, knaack.h@....de,
pmeerw@...erw.net, gregkh@...uxfoundation.org,
linux-iio@...r.kernel.org, devel@...verdev.osuosl.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4] Staging: iio: ade7758: Expand buf_lock to cover both buffer and state protection
On 4 February 2018 21:03:05 GMT, Shreeya Patel <shreeya.patel23498@...il.com> wrote:
>On Sun, 2018-02-04 at 11:10 +0000, Jonathan Cameron wrote:
>> On Sat, 3 Feb 2018 21:01:32 +0530
>> Shreeya Patel <shreeya.patel23498@...il.com> wrote:
>>
>> >
>> > iio_dev->mlock is to be used only by the IIO core for protecting
>> > device mode changes between INDIO_DIRECT and INDIO_BUFFER.
>> >
>> > This patch replaces the use of mlock with the already established
>> > buf_lock mutex.
>> >
>> > Introducing 'unlocked' forms of read and write registers. The
>> > read/write frequency functions now require buf_lock to be held.
>> > That's not obvious so avoid this but moving the locking inside
>> > the functions where it is then clear that they are taking the
>> > unlocked forms of the register read/write.
>> >
>> > It isn't readily apparent that write frequency function requires
>> > the locks to be taken, so move it inside the function to where it
>> > is required to protect.
>> >
>> > Signed-off-by: Shreeya Patel <shreeya.patel23498@...il.com>
>> Hi Shreeya,
>>
>Hello sir,
>
>> Unfortunately this introduces a new bug - you end up unlocking
>> a mutex that you never locked in one of the error paths.
>> See inline.
>I'll make this change.
>>
>> We are also still taking the mlock around the read of the
>> frequency which doesn't make any sense given there is
>> no reason to protect that against state changes.
>> Arguably fixing that could be a separate patch as it never
>> made much sense, but it should probably be in this same series
>> at least. I would have no real problem with it being in it this
>> same patch as long as the description above mentions it.
>>
>> Thanks,
>>
>> Jonathan
>>
>> >
>> > ---
>> >
>> > Changes in v2
>> > -Add static keyword to newly introduced functions and remove some
>> > added comments which are not required.
>> >
>> > Changes in v3
>> > -Remove some useless mlocks and send it as another patch.
>> > Also make the necessary change in the current patch associated
>> > with
>> > the new patch with commit id 88eba33. Make commit message more
>> > appropriate.
>> >
>> > Changes in v4
>> > -Write frequency function do not require lock so move it inside
>> > the function to where it is required to protect.
>> >
>> >
>> > drivers/staging/iio/meter/ade7758.h | 2 +-
>> > drivers/staging/iio/meter/ade7758_core.c | 49
>> > ++++++++++++++++++++++++--------
>> > 2 files changed, 38 insertions(+), 13 deletions(-)
>> >
>> > diff --git a/drivers/staging/iio/meter/ade7758.h
>> > b/drivers/staging/iio/meter/ade7758.h
>> > index 6ae78d8..2de81b5 100644
>> > --- a/drivers/staging/iio/meter/ade7758.h
>> > +++ b/drivers/staging/iio/meter/ade7758.h
>> > @@ -111,7 +111,7 @@
>> > * @trig: data ready trigger registered with iio
>> > * @tx: transmit buffer
>> > * @rx: receive buffer
>> > - * @buf_lock: mutex to protect tx and rx
>> > + * @buf_lock: mutex to protect tx, rx, read and
>> > write frequency
>> > **/
>> > struct ade7758_state {
>> > struct spi_device *us;
>> > diff --git a/drivers/staging/iio/meter/ade7758_core.c
>> > b/drivers/staging/iio/meter/ade7758_core.c
>> > index 227dbfc..ff19d46 100644
>> > --- a/drivers/staging/iio/meter/ade7758_core.c
>> > +++ b/drivers/staging/iio/meter/ade7758_core.c
>> > @@ -24,17 +24,25 @@
>> > #include "meter.h"
>> > #include "ade7758.h"
>> >
>> > -int ade7758_spi_write_reg_8(struct device *dev, u8 reg_address, u8
>> > val)
>> > +static int __ade7758_spi_write_reg_8(struct device *dev, u8
>> > reg_address, u8 val)
>> > {
>> > - int ret;
>> > struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>> > struct ade7758_state *st = iio_priv(indio_dev);
>> >
>> > - mutex_lock(&st->buf_lock);
>> > st->tx[0] = ADE7758_WRITE_REG(reg_address);
>> > st->tx[1] = val;
>> >
>> > - ret = spi_write(st->us, st->tx, 2);
>> > + return spi_write(st->us, st->tx, 2);
>> > +}
>> > +
>> > +int ade7758_spi_write_reg_8(struct device *dev, u8 reg_address, u8
>> > val)
>> > +{
>> > + int ret;
>> > + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>> > + struct ade7758_state *st = iio_priv(indio_dev);
>> > +
>> > + mutex_lock(&st->buf_lock);
>> > + ret = __ade7758_spi_write_reg_8(dev, reg_address, val);
>> > mutex_unlock(&st->buf_lock);
>> >
>> > return ret;
>> > @@ -91,7 +99,7 @@ static int ade7758_spi_write_reg_24(struct device
>> > *dev, u8 reg_address,
>> > return ret;
>> > }
>> >
>> > -int ade7758_spi_read_reg_8(struct device *dev, u8 reg_address, u8
>> > *val)
>> > +static int __ade7758_spi_read_reg_8(struct device *dev, u8
>> > reg_address, u8 *val)
>> > {
>> > struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>> > struct ade7758_state *st = iio_priv(indio_dev);
>> > @@ -111,7 +119,6 @@ int ade7758_spi_read_reg_8(struct device *dev,
>> > u8 reg_address, u8 *val)
>> > },
>> > };
>> >
>> > - mutex_lock(&st->buf_lock);
>> > st->tx[0] = ADE7758_READ_REG(reg_address);
>> > st->tx[1] = 0;
>> >
>> > @@ -124,7 +131,19 @@ int ade7758_spi_read_reg_8(struct device *dev,
>> > u8 reg_address, u8 *val)
>> > *val = st->rx[0];
>> >
>> > error_ret:
>> > + return ret;
>> > +}
>> > +
>> > +int ade7758_spi_read_reg_8(struct device *dev, u8 reg_address, u8
>> > *val)
>> > +{
>> > + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>> > + struct ade7758_state *st = iio_priv(indio_dev);
>> > + int ret;
>> > +
>> > + mutex_lock(&st->buf_lock);
>> > + ret = __ade7758_spi_read_reg_8(dev, reg_address, val);
>> > mutex_unlock(&st->buf_lock);
>> > +
>> > return ret;
>> > }
>> >
>> > @@ -480,10 +499,12 @@ static int ade7758_read_samp_freq(struct
>> > device *dev, int *val)
>> > return 0;
>> > }
>> >
>> > -static int ade7758_write_samp_freq(struct device *dev, int val)
>> > +static int ade7758_write_samp_freq(struct iio_dev *indio_dev, int
>> > val)
>> > {
>> > int ret;
>> > u8 reg, t;
>> > + struct ade7758_state *st = iio_priv(indio_dev);
>> > + struct device *dev = &indio_dev->dev;
>> >
>> > switch (val) {
>> > case 26040:
>> > @@ -503,16 +524,20 @@ static int ade7758_write_samp_freq(struct
>> > device *dev, int val)
>> > goto out;
>> This goto out results in an unlock but the lock hasn't been locked
>> for a few more lines...
>>
>> Change this to a direct return here rather than a goto to fix this.
>> return -EINVAL;
>>
>> >
>> > }
>> >
>> > - ret = ade7758_spi_read_reg_8(dev, ADE7758_WAVMODE, ®);
>> > + mutex_lock(&st->buf_lock);
>> > +
>> > + ret = __ade7758_spi_read_reg_8(dev, ADE7758_WAVMODE,
>> > ®);
>> > if (ret)
>> > goto out;
>
>Here, can I move the above lock after the calling of the read register
>but before the if(ret) statement?
>With this we can avoid locks over the read cases.
>Mostly, this shouldn't create any problem but yet I thought of first
>confirming it from you.
Don't do that. We need to protect against another caller doing that read before the write is done.
>
>Thank you
>
>> >
>> > reg &= ~(5 << 3);
>> > reg |= t << 5;
>> >
>> > - ret = ade7758_spi_write_reg_8(dev, ADE7758_WAVMODE, reg);
>> > + ret = __ade7758_spi_write_reg_8(dev, ADE7758_WAVMODE,
>> > reg);
>> >
>> > out:
>> > + mutex_unlock(&st->buf_lock);
>> > +
>> > return ret;
>> > }
>> >
>> > @@ -545,9 +570,9 @@ static int ade7758_write_raw(struct iio_dev
>> > *indio_dev,
>> > case IIO_CHAN_INFO_SAMP_FREQ:
>> > if (val2)
>> > return -EINVAL;
>> > - mutex_lock(&indio_dev->mlock);
>> > - ret = ade7758_write_samp_freq(&indio_dev->dev,
>> > val);
>> > - mutex_unlock(&indio_dev->mlock);
>> > +
>> > + ret = ade7758_write_samp_freq(indio_dev, val);
>> > +
>> > return ret;
>> > default:
>> > return -EINVAL;
>--
>To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>the body of a message to majordomo@...r.kernel.org
>More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
Powered by blists - more mailing lists