Coverity: vgxy61_tx_from_ep(): Memory - corruptions

Coverity: vgxy61_tx_from_ep(): Memory - corruptions

[coverity-bot]

Hello!

This is an experimental semi-automated report about issues detected by
Coverity from a scan of next-20221110 as part of the linux-next scan project:
https://scan.coverity.com/projects/linux-next-weekly-scan

You're getting this email because you were associated with the identified
lines of code (noted below) that were touched by commits:

  Thu Oct 27 14:37:38 2022 +0300
    153e4ad44d60 ("media: i2c: Add driver for ST VGXY61 camera sensor")

Coverity reported the following:

*** CID 1527258:  Memory - corruptions  (OVERRUN)
drivers/media/i2c/st-vgxy61.c:1528 in vgxy61_tx_from_ep()
1522     	 * valid for hardware stuff.
1523     	 */
1524     	for (p = 0; p < VGXY61_NB_POLARITIES; p++) {
1525     		if (phy2log[p] != ~0)
1526     			continue;
1527     		phy2log[p] = l;
vvv     CID 1527258:  Memory - corruptions  (OVERRUN)
vvv     Overrunning array "log2phy" of 5 4-byte elements at element index 5 (byte offset 23) using index "l" (which evaluates to 5).
1528     		log2phy[l] = p;
1529     		l++;
1530     	}
1531     	for (l = 0; l < l_nb + 1; l++)
1532     		polarities[l] = ep.bus.mipi_csi2.lane_polarities[l];
1533

If this is a false positive, please let us know so we can mark it as
such, or teach the Coverity rules to be smarter. If not, please make
sure fixes get into linux-next. :) For patches fixing this, please
include these lines (but double-check the "Fixes" first):

Reported-by: coverity-bot <keescook+coverity-bot@chromium.org>
Addresses-Coverity-ID: 1527258 ("Memory - corruptions")
Fixes: 153e4ad44d60 ("media: i2c: Add driver for ST VGXY61 camera sensor")

Note that l starts at 1, 2, or 4, so line 1529 could push it to 5, which
is out of bounds, etc...

Thanks for your attention!

-- 
Coverity-bot

Re: Coverity: vgxy61_tx_from_ep(): Memory - corruptions

[Benjamin MUGNIER]

Hello,

Thank you for your report.

On 11/10/22 17:27, coverity-bot wrote:
> Hello!
> 
> This is an experimental semi-automated report about issues detected by
> Coverity from a scan of next-20221110 as part of the linux-next scan project:
> https://scan.coverity.com/projects/linux-next-weekly-scan
> 
> You're getting this email because you were associated with the identified
> lines of code (noted below) that were touched by commits:
> 
>   Thu Oct 27 14:37:38 2022 +0300
>     153e4ad44d60 ("media: i2c: Add driver for ST VGXY61 camera sensor")
> 
> Coverity reported the following:
> 
> *** CID 1527258:  Memory - corruptions  (OVERRUN)
> drivers/media/i2c/st-vgxy61.c:1528 in vgxy61_tx_from_ep()
> 1522     	 * valid for hardware stuff.
> 1523     	 */
> 1524     	for (p = 0; p < VGXY61_NB_POLARITIES; p++) {
> 1525     		if (phy2log[p] != ~0)
> 1526     			continue;
> 1527     		phy2log[p] = l;
> vvv     CID 1527258:  Memory - corruptions  (OVERRUN)
> vvv     Overrunning array "log2phy" of 5 4-byte elements at element index 5 (byte offset 23) using index "l" (which evaluates to 5).
> 1528     		log2phy[l] = p;
> 1529     		l++;
> 1530     	}
> 1531     	for (l = 0; l < l_nb + 1; l++)
> 1532     		polarities[l] = ep.bus.mipi_csi2.lane_polarities[l];
> 1533
> 
> If this is a false positive, please let us know so we can mark it as
> such, or teach the Coverity rules to be smarter. If not, please make
> sure fixes get into linux-next. :) For patches fixing this, please
> include these lines (but double-check the "Fixes" first):
> 
> Reported-by: coverity-bot <keescook+coverity-bot@chromium.org>
> Addresses-Coverity-ID: 1527258 ("Memory - corruptions")
> Fixes: 153e4ad44d60 ("media: i2c: Add driver for ST VGXY61 camera sensor")
> 
> Note that l starts at 1, 2, or 4, so line 1529 could push it to 5, which
> is out of bounds, etc...

This 'for' loop is tied with the previous one.
The first loop fills log2phy with provided data, and the second one
fills slots that were not provided with hardware valid data. You can see
in the second loop:
  if (phy2log[p] != ~0)
    continue;
which prevents this loop from filling slots that were already filled,
and at the same time prevents 'l' to be incremented when this is the case.
As 'l' was incremented for each provided data by the first loop, and
incremented for each not provided data by the second loop. At the end of
the second loop l == VGXY61_NB_POLARITIES == 5 (because of the last
'l++'), but will never be used as an index for 'log2phy' because the
loop will end right after.


So if I'm not mistaken, and while hard to catch by tools this could be
understood as a false positive.

I could guard it with to make the checker happy with:
  if (p != VGXY61_NB_POLARITIES - 1) l++;
and it would be exactly the same, but I find this kind of unnecessary.


Any thoughts?

Thank you.

> 
> Thanks for your attention!
> 

-- 
Regards,

Benjamin