re: staging: iio: accel: Use __be16 instead of u16

re: staging: iio: accel: Use __be16 instead of u16

[Dan Carpenter]

Hello Aybuke Ozdemir,

The patch 03c6eaa37ad7: "staging: iio: accel: Use __be16 instead of
u16" from Sep 28, 2014, leads to the following static checker warning:

	drivers/staging/iio/accel/sca3000_ring.c:120 sca3000_read_first_n_hw_rb()
	warn: potential pointer math issue ('rx' is a 16 bit pointer)

drivers/staging/iio/accel/sca3000_ring.c
   107           */
   108          if (count > num_available * bytes_per_sample)
   109                  num_read = num_available*bytes_per_sample;
   110          else
   111                  num_read = count;
   112  
   113          ret = sca3000_read_data(st,
   114                                  SCA3000_REG_ADDR_RING_OUT,
   115                                  &rx, num_read);
   116          if (ret)
   117                  goto error_ret;
   118  
   119          for (i = 0; i < num_read; i++)
   120                  *(((u16 *)rx) + i) = be16_to_cpup((__be16 *)rx + i);
                          ^^^^^^^^^^^^^^^

We're writing beyond the end of the array here because of the pointer
math issue.  The fix is probably to say:

		for (i = 0; i < num_read / sizeof(u16); i++)
			*(((u16 *)rx) + i) = be16_to_cpup((__be16 *)rx + i);

   121  
   122          if (copy_to_user(buf, rx, num_read))
   123                  ret = -EFAULT;


regards,
dan carpenter

Re: staging: iio: accel: Use __be16 instead of u16

[Dan Carpenter]

Ping!

regards,
dan carpenter

On Wed, Jul 15, 2015 at 10:36:51PM +0300, Dan Carpenter wrote:
> Hello Aybuke Ozdemir,
> 
> The patch 03c6eaa37ad7: "staging: iio: accel: Use __be16 instead of
> u16" from Sep 28, 2014, leads to the following static checker warning:
> 
> 	drivers/staging/iio/accel/sca3000_ring.c:120 sca3000_read_first_n_hw_rb()
> 	warn: potential pointer math issue ('rx' is a 16 bit pointer)
> 
> drivers/staging/iio/accel/sca3000_ring.c
>    107           */
>    108          if (count > num_available * bytes_per_sample)
>    109                  num_read = num_available*bytes_per_sample;
>    110          else
>    111                  num_read = count;
>    112  
>    113          ret = sca3000_read_data(st,
>    114                                  SCA3000_REG_ADDR_RING_OUT,
>    115                                  &rx, num_read);
>    116          if (ret)
>    117                  goto error_ret;
>    118  
>    119          for (i = 0; i < num_read; i++)
>    120                  *(((u16 *)rx) + i) = be16_to_cpup((__be16 *)rx + i);
>                           ^^^^^^^^^^^^^^^
> 
> We're writing beyond the end of the array here because of the pointer
> math issue.  The fix is probably to say:
> 
> 		for (i = 0; i < num_read / sizeof(u16); i++)
> 			*(((u16 *)rx) + i) = be16_to_cpup((__be16 *)rx + i);
> 
>    121  
>    122          if (copy_to_user(buf, rx, num_read))
>    123                  ret = -EFAULT;
> 
> 
> regards,
> dan carpenter

Re: staging: iio: accel: Use __be16 instead of u16

[Daniel Baluta]

Hi Dan,

On Wed, Jul 15, 2015 at 10:36 PM, Dan Carpenter
<dan.carpenter@oracle.com> wrote:
> Hello Aybuke Ozdemir,
>
> The patch 03c6eaa37ad7: "staging: iio: accel: Use __be16 instead of
> u16" from Sep 28, 2014, leads to the following static checker warning:
>
>         drivers/staging/iio/accel/sca3000_ring.c:120 sca3000_read_first_n_hw_rb()
>         warn: potential pointer math issue ('rx' is a 16 bit pointer)
>
I think the issue was there before this patch.

> drivers/staging/iio/accel/sca3000_ring.c
>    107           */
>    108          if (count > num_available * bytes_per_sample)
>    109                  num_read = num_available*bytes_per_sample;
>    110          else
>    111                  num_read = count;
>    112
>    113          ret = sca3000_read_data(st,
>    114                                  SCA3000_REG_ADDR_RING_OUT,
>    115                                  &rx, num_read);
>    116          if (ret)
>    117                  goto error_ret;
>    118
>    119          for (i = 0; i < num_read; i++)
>    120                  *(((u16 *)rx) + i) = be16_to_cpup((__be16 *)rx + i);
>                           ^^^^^^^^^^^^^^^
>
> We're writing beyond the end of the array here because of the pointer
> math issue.  The fix is probably to say:
>
>                 for (i = 0; i < num_read / sizeof(u16); i++)
>                         *(((u16 *)rx) + i) = be16_to_cpup((__be16 *)rx + i);
>
>    121
>    122          if (copy_to_user(buf, rx, num_read))
>    123                  ret = -EFAULT;

Looks good to me. Please send a formal patch.

thanks,
Daniel.

Re: staging: iio: accel: Use __be16 instead of u16

[Dan Carpenter]

On Fri, Aug 07, 2015 at 01:59:40PM +0300, Daniel Baluta wrote:
> >    119          for (i = 0; i < num_read; i++)
> >    120                  *(((u16 *)rx) + i) = be16_to_cpup((__be16 *)rx + i);
> >                           ^^^^^^^^^^^^^^^
> >
> > We're writing beyond the end of the array here because of the pointer
> > math issue.  The fix is probably to say:
> >
> >                 for (i = 0; i < num_read / sizeof(u16); i++)
> >                         *(((u16 *)rx) + i) = be16_to_cpup((__be16 *)rx + i);
> >
> >    121
> >    122          if (copy_to_user(buf, rx, num_read))
> >    123                  ret = -EFAULT;
> 
> Looks good to me. Please send a formal patch.

It's weird that no one has noticed this bug in testing because we end
up corrupting memory every time this function is called.

regards,
dan carpenter

Re: staging: iio: accel: Use __be16 instead of u16

[Lars-Peter Clausen]

On 08/07/2015 01:27 PM, Dan Carpenter wrote:
> On Fri, Aug 07, 2015 at 01:59:40PM +0300, Daniel Baluta wrote:
>>>    119          for (i = 0; i < num_read; i++)
>>>    120                  *(((u16 *)rx) + i) = be16_to_cpup((__be16 *)rx + i);
>>>                           ^^^^^^^^^^^^^^^
>>>
>>> We're writing beyond the end of the array here because of the pointer
>>> math issue.  The fix is probably to say:
>>>
>>>                 for (i = 0; i < num_read / sizeof(u16); i++)
>>>                         *(((u16 *)rx) + i) = be16_to_cpup((__be16 *)rx + i);
>>>
>>>    121
>>>    122          if (copy_to_user(buf, rx, num_read))
>>>    123                  ret = -EFAULT;
>>
>> Looks good to me. Please send a formal patch.
> 
> It's weird that no one has noticed this bug in testing because we end
> up corrupting memory every time this function is called.

The buffers are probably smaller than the minimum allocation size so there
is a bit of unused space after the buffer and overwriting it will not result
in a corruption of used memory and nobody ever noticed.

But in any way we should just drop the endianness conversion in kernelspace
and advertise the data as big-endian to userspace.

- Lars