Re: [PATCH 2.6.22 to 4.0] spi: spidev: fix possible arithmetic overflow for multi-transfer message

Re: [PATCH 2.6.22 to 4.0] spi: spidev: fix possible arithmetic overflow for multi-transfer message

[Ben Hutchings]

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

On Fri, 2015-04-17 at 17:02 +0100, Ian Abbott wrote:
> commit f20fbaad7620 ("spi: spidev: fix possible arithmetic overflow for multi-transfer message")

Queued up for 3.2, thanks.

Ben.

> `spidev_message()` sums the lengths of the individual SPI transfers to
> determine the overall SPI message length.  It restricts the total
> length, returning an error if too long, but it does not check for
> arithmetic overflow.  For example, if the SPI message consisted of two
> transfers and the first has a length of 10 and the second has a length
> of (__u32)(-1), the total length would be seen as 9, even though the
> second transfer is actually very long.  If the second transfer specifies
> a null `rx_buf` and a non-null `tx_buf`, the `copy_from_user()` could
> overrun the spidev's pre-allocated tx buffer before it reaches an
> invalid user memory address.  Fix it by checking that neither the total
> nor the individual transfer lengths exceed the maximum allowed value.
> 
> Thanks to Dan Carpenter for reporting the potential integer overflow.
> 
> Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
> ---
> Note: original commit compares the lengths to INT_MAX instead of bufsiz
> due to changes in earlier commits.
> ---
>  drivers/spi/spidev.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
> index 4eb7a98..7bf5186 100644
> --- a/drivers/spi/spidev.c
> +++ b/drivers/spi/spidev.c
> @@ -245,7 +245,10 @@ static int spidev_message(struct spidev_data *spidev,
>  		k_tmp->len = u_tmp->len;
>  
>  		total += k_tmp->len;
> -		if (total > bufsiz) {
> +		/* Check total length of transfers.  Also check each
> +		 * transfer length to avoid arithmetic overflow.
> +		 */
> +		if (total > bufsiz || k_tmp->len > bufsiz) {
>  			status = -EMSGSIZE;
>  			goto done;
>  		}

-- 
Ben Hutchings
If you seem to know what you are doing, you'll be given more to do.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH 2.6.22 to 4.0] spi: spidev: fix possible arithmetic overflow for multi-transfer message

[Zefan Li]

On 2015/4/18 0:02, Ian Abbott wrote:
> commit f20fbaad7620 ("spi: spidev: fix possible arithmetic overflow for multi-transfer message")
> 

Queued up for 3.4. Thanks!

> `spidev_message()` sums the lengths of the individual SPI transfers to
> determine the overall SPI message length.  It restricts the total
> length, returning an error if too long, but it does not check for
> arithmetic overflow.  For example, if the SPI message consisted of two
> transfers and the first has a length of 10 and the second has a length
> of (__u32)(-1), the total length would be seen as 9, even though the
> second transfer is actually very long.  If the second transfer specifies
> a null `rx_buf` and a non-null `tx_buf`, the `copy_from_user()` could
> overrun the spidev's pre-allocated tx buffer before it reaches an
> invalid user memory address.  Fix it by checking that neither the total
> nor the individual transfer lengths exceed the maximum allowed value.
> 
> Thanks to Dan Carpenter for reporting the potential integer overflow.
> 
> Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
> ---
> Note: original commit compares the lengths to INT_MAX instead of bufsiz
> due to changes in earlier commits.
> ---
>  drivers/spi/spidev.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
> index 4eb7a98..7bf5186 100644
> --- a/drivers/spi/spidev.c
> +++ b/drivers/spi/spidev.c
> @@ -245,7 +245,10 @@ static int spidev_message(struct spidev_data *spidev,
>  		k_tmp->len = u_tmp->len;
>  
>  		total += k_tmp->len;
> -		if (total > bufsiz) {
> +		/* Check total length of transfers.  Also check each
> +		 * transfer length to avoid arithmetic overflow.
> +		 */
> +		if (total > bufsiz || k_tmp->len > bufsiz) {
>  			status = -EMSGSIZE;
>  			goto done;
>  		}
>