linux-kernel - Re: Coverity: vgxy61_tx_from_ep(): Memory

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ad2009b0-01be-76d9-ba02-e38a9ba87894@foss.st.com>
Date:   Mon, 14 Nov 2022 14:12:15 +0100
From:   Benjamin MUGNIER <benjamin.mugnier@...s.st.com>
To:     coverity-bot <keescook@...omium.org>
CC:     <linux-kernel@...r.kernel.org>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Jean-Michel Hautbois <jeanmichel.hautbois@...asonboard.com>,
        Shawn Tu <shawnx.tu@...el.com>, <linux-media@...r.kernel.org>,
        Sylvain Petinot <sylvain.petinot@...s.st.com>,
        Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>,
        Hans Verkuil <hverkuil@...all.nl>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        "Gustavo A. R. Silva" <gustavo@...eddedor.com>,
        <linux-next@...r.kernel.org>, <linux-hardening@...r.kernel.org>
Subject: Re: Coverity: vgxy61_tx_from_ep(): Memory - corruptions

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@...omium.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