Re: spi: spidev: only use up TX/RX bounce buffer space when needed

[Ian Abbott <abbotti-GUHe90Wt2aFaa/9Udqfwiw@public.gmane.org>]

On 23/03/15 13:01, Dan Carpenter wrote:
> Hi Ian,
>
> The patch 9a12bff7c346: "spi: spidev: only use up TX/RX bounce buffer
> space when needed" from Feb 16, 2015, has a potential integer overflow
> issue.
>
> drivers/spi/spidev.c
>     241          total = 0;
>     242          tx_total = 0;
>     243          rx_total = 0;
>     244          for (n = n_xfers, k_tmp = k_xfers, u_tmp = u_xfers;
>     245                          n;
>     246                          n--, k_tmp++, u_tmp++) {
>     247                  k_tmp->len = u_tmp->len;
>     248
>     249                  total += k_tmp->len;
>                          ^^^^^^^^^^^^^^^^^^^
> This is a potential integer overflow but the impact is not serious.

It's possible, although the previous version also had the same problem. 
  It's quite serious as the SPI core may read past the end of the 
pre-allocated rx buffer and write past the end of the pre-allocated tx 
buffer.

>
>     250                  /* Since the function returns the total length of transfers
>     251                   * on success, restrict the total to positive int values to
>     252                   * avoid the return value looking like an error.
>     253                   */
>     254                  if (total > INT_MAX) {

Changing that to `if (total > INT_MAX || k_tmp->len > INT_MAX)` would 
fix the overflow.  I'll send a patch later today.

>     255                          status = -EMSGSIZE;
>     256                          goto done;
>     257                  }
>     258
>     259                  if (u_tmp->rx_buf) {
>     260                          /* this transfer needs space in RX bounce buffer */
>     261                          rx_total += k_tmp->len;
>                                  ^^^^^^^^^^^^^^^^^^^^^^
> This one can maybe result in an info leak?  I'm not sure.

This is reserving space in the pre-allocated rx buffer which should be 
fine as long as the previous integer overflow is fixed.  After the call 
to spi_sync(), there is a possibility of copying old data from the SPI 
device to the user if the SPI message was not fully transferred.  If 
that's a problem, I think the safest fix would be to clear the 
pre-allocated rx buffer (at least the first rx_total bytes) before 
calling spi_sync().  Alternatively, the code that copies the rx data 
back to the user could stop copying when the returned message length is 
exceeded, and either clear the remaining user rx buffer space or leave 
it unchanged.  I guess that's open to discussion.

>
>     262                          if (rx_total > bufsiz) {
>     263                                  status = -EMSGSIZE;
>     264                                  goto done;
>     265                          }
>     266                          k_tmp->rx_buf = rx_buf;
>     267                          if (!access_ok(VERIFY_WRITE, (u8 __user *)
>     268                                                  (uintptr_t) u_tmp->rx_buf,
>     269                                                  u_tmp->len))
>     270                                  goto done;
>     271                          rx_buf += k_tmp->len;
>     272                  }
>
> regards,
> dan carpenter
>

Best regards,
Ian Abbott

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti-GUHe90Wt2aFaa/9Udqfwiw@public.gmane.org> )=-
-=(                          Web: http://www.mev.co.uk/  )=-
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html