[PATCH v2] i2c:at91: add bound checking on SMBus block length bytes

[PATCH v2] i2c:at91: add bound checking on SMBus block length bytes

[Marek Roszko]

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Acked-by: Ludovic Desroches <ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
---
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

[PATCH v2] i2c:at91: add bound checking on SMBus block length bytes

[Marek Roszko]

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

Re: [PATCH v2] i2c:at91: add bound checking on SMBus block length bytes

[Ludovic Desroches]

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Acked-by: Ludovic Desroches <ludovic.desroches-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
> ---
> 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-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2] i2c:at91: add bound checking on SMBus block length bytes

[Ludovic Desroches]

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 at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] i2c:at91: add bound checking on SMBus block length bytes

[Mark Roszko]

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

[PATCH v2] i2c:at91: add bound checking on SMBus block length bytes

[Mark Roszko]

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

Re: [PATCH v2] i2c:at91: add bound checking on SMBus block length bytes

[Ludovic Desroches]

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] i2c:at91: add bound checking on SMBus block length bytes

[Ludovic Desroches]

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 at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] i2c:at91: add bound checking on SMBus block length bytes

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 389 bytes --]


> > 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.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH v2] i2c:at91: add bound checking on SMBus block length bytes

[Wolfram Sang]

> > 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.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141027/9cec37aa/attachment.sig>