| Message ID | 1408585181-10131-1-git-send-email-mark.roszko@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
Hi Wolfram, Could you take this patch into your tree? Thanks Regards Ludovic On Wed, Aug 20, 2014 at 09:39:41PM -0400, Marek Roszko wrote: > The driver was not bound checking the received length byte to ensure it was within the > the buffer size that is allocated for SMBus blocks. This resulted in buffer overflows > whenever an invalid length byte was received. > It also failed to ensure the length byte was not zero. If it received zero, it would end up > in an infinite loop as the at91_twi_read_next_byte function returned immediately without > allowing RHR to be read to clear the RXRDY interrupt. > > Tested agaisnt a SMBus compliant battery. > > Signed-off-by: Marek Roszko <mark.roszko@gmail.com> > Acked-by: Ludovic Desroches <ludovic.desroches@atmel.com> > --- > Change from v1: > fixed typo in commit message > reworded message slightly to be specifically say length byte > > drivers/i2c/busses/i2c-at91.c | 28 ++++++++++++++++++++++++---- > 1 file changed, 24 insertions(+), 4 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c > index e95f9ba..ec4ff33 100644 > --- a/drivers/i2c/busses/i2c-at91.c > +++ b/drivers/i2c/busses/i2c-at91.c > @@ -101,6 +101,7 @@ struct at91_twi_dev { > unsigned twi_cwgr_reg; > struct at91_twi_pdata *pdata; > bool use_dma; > + bool recv_len_abort; > struct at91_twi_dma dma; > }; > > @@ -267,12 +268,24 @@ static void at91_twi_read_next_byte(struct at91_twi_dev *dev) > *dev->buf = at91_twi_read(dev, AT91_TWI_RHR) & 0xff; > --dev->buf_len; > > + /* return if aborting, we only needed to read RHR to clear RXRDY*/ > + if (dev->recv_len_abort) > + return; > + > /* handle I2C_SMBUS_BLOCK_DATA */ > if (unlikely(dev->msg->flags & I2C_M_RECV_LEN)) { > - dev->msg->flags &= ~I2C_M_RECV_LEN; > - dev->buf_len += *dev->buf; > - dev->msg->len = dev->buf_len + 1; > - dev_dbg(dev->dev, "received block length %d\n", dev->buf_len); > + /* ensure length byte is a valid value */ > + if (*dev->buf <= I2C_SMBUS_BLOCK_MAX && *dev->buf > 0) { > + dev->msg->flags &= ~I2C_M_RECV_LEN; > + dev->buf_len += *dev->buf; > + dev->msg->len = dev->buf_len + 1; > + dev_dbg(dev->dev, "received block length %d\n", > + dev->buf_len); > + } else { > + /* abort and send the stop by reading one more byte */ > + dev->recv_len_abort = true; > + dev->buf_len = 1; > + } > } > > /* send stop if second but last byte has been read */ > @@ -444,6 +457,12 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev) > ret = -EIO; > goto error; > } > + if (dev->recv_len_abort) { > + dev_err(dev->dev, "invalid smbus block length recvd\n"); > + ret = -EPROTO; > + goto error; > + } > + > dev_dbg(dev->dev, "transfer complete\n"); > > return 0; > @@ -500,6 +519,7 @@ static int at91_twi_xfer(struct i2c_adapter *adap, struct i2c_msg *msg, int num) > dev->buf_len = m_start->len; > dev->buf = m_start->buf; > dev->msg = m_start; > + dev->recv_len_abort = false; > > ret = at91_do_twi_transfer(dev); > > -- > 1.7.10.4 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Ludovic, Wolfram took in the patch quietly awhile back. It's in the 3.17 kernel and backported to some of the older trees. https://github.com/torvalds/linux/commit/75b81f339c6af43f6f4a1b3eabe0603321dade65 Regards, Mark -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Thu, Oct 23, 2014 at 09:28:55AM -0400, Mark Roszko wrote: > Hi Ludovic, > > Wolfram took in the patch quietly awhile back. It's in the 3.17 kernel > and backported to some of the older trees. > https://github.com/torvalds/linux/commit/75b81f339c6af43f6f4a1b3eabe0603321dade65 Great, sorry for the noise so. It seems I have checked it from a wrong branch. Ludovic > > Regards, > Mark > -- > To unsubscribe from this list: send the line "unsubscribe linux-i2c" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Wolfram took in the patch quietly awhile back. It's in the 3.17 kernel > > and backported to some of the older trees. > > https://github.com/torvalds/linux/commit/75b81f339c6af43f6f4a1b3eabe0603321dade65 > > Great, sorry for the noise so. It seems I have checked it from a wrong > branch. Yes, usually I send out mails when applying patches. Dunno why I missed it here.
diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c index e95f9ba..ec4ff33 100644 --- a/drivers/i2c/busses/i2c-at91.c +++ b/drivers/i2c/busses/i2c-at91.c @@ -101,6 +101,7 @@ struct at91_twi_dev { unsigned twi_cwgr_reg; struct at91_twi_pdata *pdata; bool use_dma; + bool recv_len_abort; struct at91_twi_dma dma; }; @@ -267,12 +268,24 @@ static void at91_twi_read_next_byte(struct at91_twi_dev *dev) *dev->buf = at91_twi_read(dev, AT91_TWI_RHR) & 0xff; --dev->buf_len; + /* return if aborting, we only needed to read RHR to clear RXRDY*/ + if (dev->recv_len_abort) + return; + /* handle I2C_SMBUS_BLOCK_DATA */ if (unlikely(dev->msg->flags & I2C_M_RECV_LEN)) { - dev->msg->flags &= ~I2C_M_RECV_LEN; - dev->buf_len += *dev->buf; - dev->msg->len = dev->buf_len + 1; - dev_dbg(dev->dev, "received block length %d\n", dev->buf_len); + /* ensure length byte is a valid value */ + if (*dev->buf <= I2C_SMBUS_BLOCK_MAX && *dev->buf > 0) { + dev->msg->flags &= ~I2C_M_RECV_LEN; + dev->buf_len += *dev->buf; + dev->msg->len = dev->buf_len + 1; + dev_dbg(dev->dev, "received block length %d\n", + dev->buf_len); + } else { + /* abort and send the stop by reading one more byte */ + dev->recv_len_abort = true; + dev->buf_len = 1; + } } /* send stop if second but last byte has been read */ @@ -444,6 +457,12 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev) ret = -EIO; goto error; } + if (dev->recv_len_abort) { + dev_err(dev->dev, "invalid smbus block length recvd\n"); + ret = -EPROTO; + goto error; + } + dev_dbg(dev->dev, "transfer complete\n"); return 0; @@ -500,6 +519,7 @@ static int at91_twi_xfer(struct i2c_adapter *adap, struct i2c_msg *msg, int num) dev->buf_len = m_start->len; dev->buf = m_start->buf; dev->msg = m_start; + dev->recv_len_abort = false; ret = at91_do_twi_transfer(dev);