Re: [syzbot] KASAN: out-of-bounds Read in i801_isr

Andy Shevchenko
2021-05-24T10:42:15Z

On Mon, May 24, 2021 at 11:16:52AM +0200, Jean Delvare wrote:
> On Fri, 21 May 2021 15:48:20 +0200, Jean Delvare wrote:
> > Looking at the ICH9 datasheet, I see the following description for the
> > KILL bit (which is what we try to use to reset the SMBus controller):
> >
> > "Kills the current host transaction taking place, sets the FAILED
> > status bit, and asserts the interrupt (or SMI#)."
> >
> > At the time the recovery code was written, i2c-i801 was a polling-only
> > driver, interrupts were not supported, so asserting the interrupt had
> > no effect. Now that the driver does support interrupts, this would call
> > i801_isr(), right?
>
> I modified the i2c-i801 driver to simulate a timeout once in a while,
> and I can confirm that i801_isr() gets called when the KILL bit is set.
>
> > So my theory is that our attempt to kill a timed-out byte-by-byte block
> > transaction triggers an interrupt, which calls in i801_isr() with the
> > SMBHSTSTS_BYTE_DONE bit set. This in turn causes i801_isr_byte_done()
> > to be called while we are absolutely not ready nor even supposed to
> > process the next data byte.
>
> I can also report that I'm reproducing the bug reported by syzbot when
> this happens, by running decode-dimms with my modified i2c-i801 driver.
> I have 4 SPD EEPROMs instantiated for my DIMMs, so decode-dimms
> triggers a lot of calls to i2c_smbus_read_i2c_block_data().
>
> > I guess we should clear SMBHSTSTS_BYTE_DONE before issuing a
> > SMBHSTCNT_KILL. Alternatively we could add a check at the beginning of
> > i801_isr() to bail out immediately if SMBHSTCNT_KILL is set. While
> > possibly more robust, this approach has the drawback of increasing the
> > processing time of all interrupts, even standard/legitimate ones. So
> > maybe just clearing SMBHSTSTS_BYTE_DONE is more reasonable. Something
> > like:
> >
> > --- linux-5.11.orig/drivers/i2c/busses/i2c-i801.c
> > +++ linux-5.11/drivers/i2c/busses/i2c-i801.c
> > @@ -393,6 +393,8 @@ static int i801_check_post(struct i801_p
> > dev_err(&priv->pci_dev->dev, "Transaction timeout\n");
> > /* try to stop the current command */
> > dev_dbg(&priv->pci_dev->dev, "Terminating the current operation\n");
> > + /* Clear BYTE_DONE so as to not confuse i801_isr() */
> > + outb_p(SMBHSTSTS_BYTE_DONE, SMBHSTSTS(priv));
> > outb_p(inb_p(SMBHSTCNT(priv)) | SMBHSTCNT_KILL,
> > SMBHSTCNT(priv));
> > usleep_range(1000, 2000);
>
> I ended up with a different approach. Clearing BYTE_DONE would possibly
> avoid the problem immediately at hand, however the interrupt would
> still be triggered, even for other (not byte-by-byte) transactions,
> causing us to fiddle with the status register. I don't think it's
> needed, and it could have unintended side effects.
>
> So instead I tried the alternative approach of checking the KILL bit in
> i801_isr() and returning immediately if it is set. While it does work,
> I noticed that i801_isr() is in fact called in a loop for the whole
> duration of the usleep_range(1000, 2000) before we clear the KILL bit.
> While better than an out-of-bounds memory access, an interrupt flood is
> still not ideal.
>
> > I must say I wonder why SMBHSTCNT_KILL generates an interrupt in the
> > first place, I can't see who would need this.
>
> Which made me think... The interrupt is being generated because we
> *ask* for it. It doesn't have to be generated if we aren't interested
> in it. So the fix would be very simple: don't bother preserving the
> other bits in HST_CNT, they will be set properly again for the next
> transaction anyway, only set the KILL bit to 1. In particular don't
> preserve the INTREN bit, so no interrupt will be generated.
>
> @@ -395,11 +401,9 @@ static int i801_check_post(struct i801_p
> dev_err(&priv->pci_dev->dev, "Transaction timeout\n");
> /* try to stop the current command */
> dev_dbg(&priv->pci_dev->dev, "Terminating the current operation\n");
> - outb_p(inb_p(SMBHSTCNT(priv)) | SMBHSTCNT_KILL,
> - SMBHSTCNT(priv));
> + outb_p(SMBHSTCNT_KILL, SMBHSTCNT(priv));
> usleep_range(1000, 2000);
> - outb_p(inb_p(SMBHSTCNT(priv)) & (~SMBHSTCNT_KILL),
> - SMBHSTCNT(priv));
> + outb_p(0, SMBHSTCNT(priv));
>
> /* Check if it worked */
> status = inb_p(SMBHSTSTS(priv));
>
> This works for me. If there are no objections I'll post a proper patch.

No objection from me, thanks for conducting a research and developing the fix!
Feel free to add mine
Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
to whatever solution you consider the best.

--
With Best Regards,
Andy Shevchenko