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
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html