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

[Ian Abbott]

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 '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/  )=-