| Message ID | f056286a-1db9-b88c-6d36-a3358190b9c9@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] i2c: i801: fix potential race in i801_block_transaction_byte_by_byte | expand |
On Sat, 02 Sep 2023 22:10:52 +0200, Heiner Kallweit wrote: > Currently we set SMBHSTCNT_LAST_BYTE only after the host has started > receiving the last byte. If we get e.g. preempted before setting > SMBHSTCNT_LAST_BYTE, the host may be finished with receiving the byte > before SMBHSTCNT_LAST_BYTE is set. > Therefore change the code to set SMBHSTCNT_LAST_BYTE before writing > SMBHSTSTS_BYTE_DONE for the byte before the last byte. Now the code > is also consistent with what we do in i801_isr_byte_done(). > > Reported-by: Jean Delvare <jdelvare@suse.com> Note for Wolfram: checkpatch says we should insert here: Closes: https://lore.kernel.org/linux-i2c/20230828152747.09444625@endymion.delvare/ > Reviewed-by: Jean Delvare <jdelvare@suse.de> > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > v2: > - remove incorrect Fixes tag > --- > drivers/i2c/busses/i2c-i801.c | 19 +++++++++---------- > 1 file changed, 9 insertions(+), 10 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c > index 7a0ccc584..8acf09539 100644 > --- a/drivers/i2c/busses/i2c-i801.c > +++ b/drivers/i2c/busses/i2c-i801.c > @@ -679,15 +679,11 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv, > return result ? priv->status : -ETIMEDOUT; > } > > - for (i = 1; i <= len; i++) { > - if (i == len && read_write == I2C_SMBUS_READ) > - smbcmd |= SMBHSTCNT_LAST_BYTE; > - outb_p(smbcmd, SMBHSTCNT(priv)); > - > - if (i == 1) > - outb_p(inb(SMBHSTCNT(priv)) | SMBHSTCNT_START, > - SMBHSTCNT(priv)); > + if (len == 1 && read_write == I2C_SMBUS_READ) > + smbcmd |= SMBHSTCNT_LAST_BYTE; > + outb_p(smbcmd | SMBHSTCNT_START, SMBHSTCNT(priv)); > > + for (i = 1; i <= len; i++) { > status = i801_wait_byte_done(priv); > if (status) > return status; > @@ -710,9 +706,12 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv, > data->block[0] = len; > } > > - /* Retrieve/store value in SMBBLKDAT */ > - if (read_write == I2C_SMBUS_READ) > + if (read_write == I2C_SMBUS_READ) { > data->block[i] = inb_p(SMBBLKDAT(priv)); > + if (i == len - 1) > + outb_p(smbcmd | SMBHSTCNT_LAST_BYTE, SMBHSTCNT(priv)); > + } > + > if (read_write == I2C_SMBUS_WRITE && i+1 <= len) > outb_p(data->block[i+1], SMBBLKDAT(priv)); >
Hi Jean, On Tue, Sep 05, 2023 at 10:12:43AM +0200, Jean Delvare wrote: > On Sat, 02 Sep 2023 22:10:52 +0200, Heiner Kallweit wrote: > > Currently we set SMBHSTCNT_LAST_BYTE only after the host has started > > receiving the last byte. If we get e.g. preempted before setting > > SMBHSTCNT_LAST_BYTE, the host may be finished with receiving the byte > > before SMBHSTCNT_LAST_BYTE is set. > > Therefore change the code to set SMBHSTCNT_LAST_BYTE before writing > > SMBHSTSTS_BYTE_DONE for the byte before the last byte. Now the code > > is also consistent with what we do in i801_isr_byte_done(). > > > > Reported-by: Jean Delvare <jdelvare@suse.com> > > Note for Wolfram: checkpatch says we should insert here: > > Closes: https://lore.kernel.org/linux-i2c/20230828152747.09444625@endymion.delvare/ does this also need a Fixes: tag? I tried to check it, but there was an intricate jungle of commits in these lines. Anyway, you can add: Acked-by: Andi Shyti <andi.shyti@kernel.org> Thanks, Andi
On 05.09.2023 11:11, Andi Shyti wrote: > Hi Jean, > > On Tue, Sep 05, 2023 at 10:12:43AM +0200, Jean Delvare wrote: >> On Sat, 02 Sep 2023 22:10:52 +0200, Heiner Kallweit wrote: >>> Currently we set SMBHSTCNT_LAST_BYTE only after the host has started >>> receiving the last byte. If we get e.g. preempted before setting >>> SMBHSTCNT_LAST_BYTE, the host may be finished with receiving the byte >>> before SMBHSTCNT_LAST_BYTE is set. >>> Therefore change the code to set SMBHSTCNT_LAST_BYTE before writing >>> SMBHSTSTS_BYTE_DONE for the byte before the last byte. Now the code >>> is also consistent with what we do in i801_isr_byte_done(). >>> >>> Reported-by: Jean Delvare <jdelvare@suse.com> >> >> Note for Wolfram: checkpatch says we should insert here: >> >> Closes: https://lore.kernel.org/linux-i2c/20230828152747.09444625@endymion.delvare/ > > does this also need a Fixes: tag? I tried to check it, but there > was an intricate jungle of commits in these lines. > Quoting Jean from previous communication about this patch: As far as I see, the race condition already existed when the kernel switched to git, so there's no point in having a Fixes statement. > Anyway, you can add: > > Acked-by: Andi Shyti <andi.shyti@kernel.org> > > Thanks, > Andi
Hi Heiner, On Tue, Sep 05, 2023 at 01:35:10PM +0200, Heiner Kallweit wrote: > On 05.09.2023 11:11, Andi Shyti wrote: > > Hi Jean, > > > > On Tue, Sep 05, 2023 at 10:12:43AM +0200, Jean Delvare wrote: > >> On Sat, 02 Sep 2023 22:10:52 +0200, Heiner Kallweit wrote: > >>> Currently we set SMBHSTCNT_LAST_BYTE only after the host has started > >>> receiving the last byte. If we get e.g. preempted before setting > >>> SMBHSTCNT_LAST_BYTE, the host may be finished with receiving the byte > >>> before SMBHSTCNT_LAST_BYTE is set. > >>> Therefore change the code to set SMBHSTCNT_LAST_BYTE before writing > >>> SMBHSTSTS_BYTE_DONE for the byte before the last byte. Now the code > >>> is also consistent with what we do in i801_isr_byte_done(). > >>> > >>> Reported-by: Jean Delvare <jdelvare@suse.com> > >> > >> Note for Wolfram: checkpatch says we should insert here: > >> > >> Closes: https://lore.kernel.org/linux-i2c/20230828152747.09444625@endymion.delvare/ > > > > does this also need a Fixes: tag? I tried to check it, but there > > was an intricate jungle of commits in these lines. > > > Quoting Jean from previous communication about this patch: > As far as I see, the race condition already existed when the kernel > switched to git, so there's no point in having a Fixes statement. true... I forgot about this comment. Anyway I think that this should, then, go to all the stable kernels and I believe that without the Fixes tag this will never be picked up. Maybe Greg can advise here. Would you mind resending this patch Cc'eing the stable kernel and adding a note after the '---'? Andi > > Anyway, you can add: > > > > Acked-by: Andi Shyti <andi.shyti@kernel.org> > > > > Thanks, > > Andi >
On 06.09.2023 00:59, Andi Shyti wrote: > Hi Heiner, > > On Tue, Sep 05, 2023 at 01:35:10PM +0200, Heiner Kallweit wrote: >> On 05.09.2023 11:11, Andi Shyti wrote: >>> Hi Jean, >>> >>> On Tue, Sep 05, 2023 at 10:12:43AM +0200, Jean Delvare wrote: >>>> On Sat, 02 Sep 2023 22:10:52 +0200, Heiner Kallweit wrote: >>>>> Currently we set SMBHSTCNT_LAST_BYTE only after the host has started >>>>> receiving the last byte. If we get e.g. preempted before setting >>>>> SMBHSTCNT_LAST_BYTE, the host may be finished with receiving the byte >>>>> before SMBHSTCNT_LAST_BYTE is set. >>>>> Therefore change the code to set SMBHSTCNT_LAST_BYTE before writing >>>>> SMBHSTSTS_BYTE_DONE for the byte before the last byte. Now the code >>>>> is also consistent with what we do in i801_isr_byte_done(). >>>>> >>>>> Reported-by: Jean Delvare <jdelvare@suse.com> >>>> >>>> Note for Wolfram: checkpatch says we should insert here: >>>> >>>> Closes: https://lore.kernel.org/linux-i2c/20230828152747.09444625@endymion.delvare/ >>> >>> does this also need a Fixes: tag? I tried to check it, but there >>> was an intricate jungle of commits in these lines. >>> >> Quoting Jean from previous communication about this patch: >> As far as I see, the race condition already existed when the kernel >> switched to git, so there's no point in having a Fixes statement. > > true... I forgot about this comment. > > Anyway I think that this should, then, go to all the stable > kernels and I believe that without the Fixes tag this will never > be picked up. > Then we may have to set the Fixes tag to the following? 1da177e4c3f4 ("Linux-2.6.12-rc2") Plus a comment that the issue existed before already. > Maybe Greg can advise here. > I *think* Greg (or a bot of him) would complain when receiving something for stable w/o a Fixes tag. > Would you mind resending this patch Cc'eing the stable kernel and > adding a note after the '---'? > OK > Andi > >>> Anyway, you can add: >>> >>> Acked-by: Andi Shyti <andi.shyti@kernel.org> >>> >>> Thanks, >>> Andi >>
On Wed, 6 Sep 2023 00:59:22 +0200, Andi Shyti wrote: > On Tue, Sep 05, 2023 at 01:35:10PM +0200, Heiner Kallweit wrote: > > On 05.09.2023 11:11, Andi Shyti wrote: > > > On Tue, Sep 05, 2023 at 10:12:43AM +0200, Jean Delvare wrote: > > >> On Sat, 02 Sep 2023 22:10:52 +0200, Heiner Kallweit wrote: > > >>> Currently we set SMBHSTCNT_LAST_BYTE only after the host has started > > >>> receiving the last byte. If we get e.g. preempted before setting > > >>> SMBHSTCNT_LAST_BYTE, the host may be finished with receiving the byte > > >>> before SMBHSTCNT_LAST_BYTE is set. > > >>> Therefore change the code to set SMBHSTCNT_LAST_BYTE before writing > > >>> SMBHSTSTS_BYTE_DONE for the byte before the last byte. Now the code > > >>> is also consistent with what we do in i801_isr_byte_done(). > > >>> > > >>> Reported-by: Jean Delvare <jdelvare@suse.com> > > >> > > >> Note for Wolfram: checkpatch says we should insert here: > > >> > > >> Closes: https://lore.kernel.org/linux-i2c/20230828152747.09444625@endymion.delvare/ > > > > > > does this also need a Fixes: tag? I tried to check it, but there > > > was an intricate jungle of commits in these lines. > > > > > Quoting Jean from previous communication about this patch: > > As far as I see, the race condition already existed when the kernel > > switched to git, so there's no point in having a Fixes statement. > > true... I forgot about this comment. > > Anyway I think that this should, then, go to all the stable > kernels and I believe that without the Fixes tag this will never > be picked up. > > Maybe Greg can advise here. > > Would you mind resending this patch Cc'eing the stable kernel and > adding a note after the '---'? Again, no. This is a theoretical bug, that was discovered by code inspection. It's been present in the driver for over 20 years and we have no evidence that anyone ever hit it. Furthermore, it only happens if the driver is in polling mode, which is uncommon, and only for block transactions, and only when the block buffer isn't used. And then the only negative effect of the bug is to shift the internal register pointer by 1. So it would only ever be a problem if someone mixes block reads and immediate byte reads on the same device. That's a very uncommon usage model. That's definitely not "a real bug that bothers people". Not every fix needs to go to stable. In this case, our engineering time is better used elsewhere.
On Wed, Sep 06, 2023 at 08:05:59AM +0200, Heiner Kallweit wrote: > On 06.09.2023 00:59, Andi Shyti wrote: > > Hi Heiner, > > > > On Tue, Sep 05, 2023 at 01:35:10PM +0200, Heiner Kallweit wrote: > >> On 05.09.2023 11:11, Andi Shyti wrote: > >>> Hi Jean, > >>> > >>> On Tue, Sep 05, 2023 at 10:12:43AM +0200, Jean Delvare wrote: > >>>> On Sat, 02 Sep 2023 22:10:52 +0200, Heiner Kallweit wrote: > >>>>> Currently we set SMBHSTCNT_LAST_BYTE only after the host has started > >>>>> receiving the last byte. If we get e.g. preempted before setting > >>>>> SMBHSTCNT_LAST_BYTE, the host may be finished with receiving the byte > >>>>> before SMBHSTCNT_LAST_BYTE is set. > >>>>> Therefore change the code to set SMBHSTCNT_LAST_BYTE before writing > >>>>> SMBHSTSTS_BYTE_DONE for the byte before the last byte. Now the code > >>>>> is also consistent with what we do in i801_isr_byte_done(). > >>>>> > >>>>> Reported-by: Jean Delvare <jdelvare@suse.com> > >>>> > >>>> Note for Wolfram: checkpatch says we should insert here: > >>>> > >>>> Closes: https://lore.kernel.org/linux-i2c/20230828152747.09444625@endymion.delvare/ > >>> > >>> does this also need a Fixes: tag? I tried to check it, but there > >>> was an intricate jungle of commits in these lines. > >>> > >> Quoting Jean from previous communication about this patch: > >> As far as I see, the race condition already existed when the kernel > >> switched to git, so there's no point in having a Fixes statement. > > > > true... I forgot about this comment. > > > > Anyway I think that this should, then, go to all the stable > > kernels and I believe that without the Fixes tag this will never > > be picked up. > > > > Then we may have to set the Fixes tag to the following? > 1da177e4c3f4 ("Linux-2.6.12-rc2") > Plus a comment that the issue existed before already. > > > Maybe Greg can advise here. > > > I *think* Greg (or a bot of him) would complain when receiving > something for stable w/o a Fixes tag. No, fixes tags are not required, but they are nice. For the full set of rules on how to do this, please see: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index 7a0ccc584..8acf09539 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -679,15 +679,11 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv, return result ? priv->status : -ETIMEDOUT; } - for (i = 1; i <= len; i++) { - if (i == len && read_write == I2C_SMBUS_READ) - smbcmd |= SMBHSTCNT_LAST_BYTE; - outb_p(smbcmd, SMBHSTCNT(priv)); - - if (i == 1) - outb_p(inb(SMBHSTCNT(priv)) | SMBHSTCNT_START, - SMBHSTCNT(priv)); + if (len == 1 && read_write == I2C_SMBUS_READ) + smbcmd |= SMBHSTCNT_LAST_BYTE; + outb_p(smbcmd | SMBHSTCNT_START, SMBHSTCNT(priv)); + for (i = 1; i <= len; i++) { status = i801_wait_byte_done(priv); if (status) return status; @@ -710,9 +706,12 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv, data->block[0] = len; } - /* Retrieve/store value in SMBBLKDAT */ - if (read_write == I2C_SMBUS_READ) + if (read_write == I2C_SMBUS_READ) { data->block[i] = inb_p(SMBBLKDAT(priv)); + if (i == len - 1) + outb_p(smbcmd | SMBHSTCNT_LAST_BYTE, SMBHSTCNT(priv)); + } + if (read_write == I2C_SMBUS_WRITE && i+1 <= len) outb_p(data->block[i+1], SMBBLKDAT(priv));