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

[Dmitry Torokhov]

On Mon, May 23, 2016 at 11:20:35AM +0100, Ian Abbott wrote:
> On 21/05/16 17:50, Dmitry Torokhov wrote:
> >On Mon, Mar 23, 2015 at 10:50 AM, Ian Abbott <abbotti@xxxxxxxxx> wrote:
> >>`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@xxxxxxxxx>
> >>Cc: <stable@xxxxxxxxxxxxxxx> # 4.0+
> >>---
> >>This could be backported to kernels prior to 4.0, but the total and
> >>individual lengths would need to be checked against `bufsiz` instead of
> >>`INT_MAX`.
> >>---
> >>  drivers/spi/spidev.c | 5 +++--
> >>  1 file changed, 3 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
> >>index bb6b3ab..23ad978 100644
> >>--- a/drivers/spi/spidev.c
> >>+++ b/drivers/spi/spidev.c
> >>@@ -249,9 +249,10 @@ static int spidev_message(struct spidev_data *spidev,
> >>                 total += k_tmp->len;
> >>                 /* Since the function returns the total length of transfers
> >>                  * on success, restrict the total to positive int values to
> >>-                * avoid the return value looking like an error.
> >>+                * avoid the return value looking like an error.  Also check
> >>+                * each transfer length to avoid arithmetic overflow.
> >>                  */
> >>-               if (total > INT_MAX) {
> >>+               if (total > INT_MAX || k_tmp->len > INT_MAX) {
> >
> >What if total is INT_MAX - 2 and k_tmp->len is 3? What about total is
> >INT_MAX and k_tmp->len is INT_MAX as well? I think the proper check
> 
> In your questions, I assume you are referring to the values of
> 'total' before the addition.  I'll call the values 'old_total' and

Sorry, yes, for some reason I was thinking we are checking before
performing addition. Ignore me.

> 'new_total' (with the same type as 'total', i.e. 'unsigned int').
> Note that total (and old_total, and new_total) and 'k_tmp->len' have
> range UINT_MAX, or 2*INT_MAX+1.
> 
> Before the addition, we know that old_total <= INT_MAX (otherwise
> the loop would have errored out already), but k_tmp->len can have
> any value from 0 to UINT_MAX.  After the addition, new_total can
> have any value from 0 to UINT_MAX, and might be less than old_total.
> new_total can only be less than old_total if old_total + k_tmp->len
> > UINT_MAX, and here I am referring to proper addition, not addition
> modulo UINT_MAX+1.  Rearranging, new_total will be less than
> old_total if k_tmp->len > UINT_MAX - old_total.  Since the maximum
> value of old_total is INT_MAX, the lowest possible value of
> k_tmp->len that could cause new_total to be less than old_total is
> UINT_MAX - INT_MAX, or INT_MAX+1.  That is what the second part of
> the 'if' test is detecting.
> 
> >should be:
> >
> >if (total < k_tmp->len || total > INT_MAX) {
> >         ...
> >}
> >
> 
> That also works.
> 
> -- 
> -=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@xxxxxxxxx> )=-
> -=(                          Web: http://www.mev.co.uk/  )=-

-- 
Dmitry