* re: staging: iio: accel: Use __be16 instead of u16
@ 2015-07-15 19:36 Dan Carpenter
2015-08-07 10:47 ` Dan Carpenter
2015-08-07 10:59 ` Daniel Baluta
0 siblings, 2 replies; 5+ messages in thread
From: Dan Carpenter @ 2015-07-15 19:36 UTC (permalink / raw)
To: aybuke.147; +Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald, linux-iio
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: staging: iio: accel: Use __be16 instead of u16
2015-07-15 19:36 staging: iio: accel: Use __be16 instead of u16 Dan Carpenter
@ 2015-08-07 10:47 ` Dan Carpenter
2015-08-07 10:59 ` Daniel Baluta
1 sibling, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2015-08-07 10:47 UTC (permalink / raw)
To: aybuke.147; +Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald, linux-iio
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: staging: iio: accel: Use __be16 instead of u16
2015-07-15 19:36 staging: iio: accel: Use __be16 instead of u16 Dan Carpenter
2015-08-07 10:47 ` Dan Carpenter
@ 2015-08-07 10:59 ` Daniel Baluta
2015-08-07 11:27 ` Dan Carpenter
1 sibling, 1 reply; 5+ messages in thread
From: Daniel Baluta @ 2015-08-07 10:59 UTC (permalink / raw)
To: Dan Carpenter
Cc: aybüke özdemir, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald, linux-iio@vger.kernel.org
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.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: staging: iio: accel: Use __be16 instead of u16
2015-08-07 10:59 ` Daniel Baluta
@ 2015-08-07 11:27 ` Dan Carpenter
2015-08-07 11:38 ` Lars-Peter Clausen
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2015-08-07 11:27 UTC (permalink / raw)
To: Daniel Baluta
Cc: aybüke özdemir, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald, linux-iio@vger.kernel.org
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: staging: iio: accel: Use __be16 instead of u16
2015-08-07 11:27 ` Dan Carpenter
@ 2015-08-07 11:38 ` Lars-Peter Clausen
0 siblings, 0 replies; 5+ messages in thread
From: Lars-Peter Clausen @ 2015-08-07 11:38 UTC (permalink / raw)
To: Dan Carpenter, Daniel Baluta
Cc: aybüke özdemir, Hartmut Knaack, Peter Meerwald,
linux-iio@vger.kernel.org
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-08-07 11:38 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-07-15 19:36 staging: iio: accel: Use __be16 instead of u16 Dan Carpenter
2015-08-07 10:47 ` Dan Carpenter
2015-08-07 10:59 ` Daniel Baluta
2015-08-07 11:27 ` Dan Carpenter
2015-08-07 11:38 ` Lars-Peter Clausen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).