linux-kernel - Re: [PATCH] phy: sun4i-usb: Fix a W=1 compilation failure

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230905151301.591fa238@donnerap.manchester.arm.com>
Date:   Tue, 5 Sep 2023 15:13:01 +0100
From:   Andre Przywara <andre.przywara@....com>
To:     Dan Carpenter <dan.carpenter@...aro.org>
Cc:     Christophe JAILLET <christophe.jaillet@...adoo.fr>,
        Vinod Koul <vkoul@...nel.org>,
        Kishon Vijay Abraham I <kishon@...nel.org>,
        Chen-Yu Tsai <wens@...e.org>,
        Jernej Skrabec <jernej.skrabec@...il.com>,
        Samuel Holland <samuel@...lland.org>,
        linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org,
        linux-phy@...ts.infradead.org,
        linux-arm-kernel@...ts.infradead.org, linux-sunxi@...ts.linux.dev
Subject: Re: [PATCH] phy: sun4i-usb: Fix a W=1 compilation failure

On Tue, 5 Sep 2023 13:32:08 +0300
Dan Carpenter <dan.carpenter@...aro.org> wrote:

Hi,

> On Mon, Sep 04, 2023 at 12:58:55AM +0100, Andre Przywara wrote:
> > On Sun,  3 Sep 2023 12:11:06 +0200
> > Christophe JAILLET <christophe.jaillet@...adoo.fr> wrote:
> >   
> > > With gcc 12.3.0, when this file is built, we get errors such as:
> > > 
> > > drivers/phy/allwinner/phy-sun4i-usb.c: In function ‘sun4i_usb_phy_probe’:
> > > drivers/phy/allwinner/phy-sun4i-usb.c:790:52: error: ‘_vbus’ directive output may be truncated writing 5 bytes into a region of size between 2 and 12 [-Werror=format-truncation=]
> > >   790 |                 snprintf(name, sizeof(name), "usb%d_vbus", i);
> > >       |                                                    ^~~~~
> > > drivers/phy/allwinner/phy-sun4i-usb.c:790:17: note: ‘snprintf’ output between 10 and 20 bytes into a destination of size 16
> > >   790 |                 snprintf(name, sizeof(name), "usb%d_vbus", i);
> > >       |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > 
> > > Because of the possible value of 'i', this can't be an issue in real world  
> > 
> > Would using "u8 i;" help? After all currently there are only 4 PHYs
> > max, and in general this isn't expected to be more than a "handful", so
> > 8 bits should be plenty. An unsigned is better anyway.  
> 
> Generally unsigned types are trickier and cause bugs.  I've blogged
> about this before.  The title is a probably more negative than it should
> have been.
> 
> https://staticthinking.wordpress.com/2022/06/01/unsigned-int-i-is-stupid/
> 
> My blog mentions u8 i.  I would say avoid that unless forced by an API.

Fair enough, the reason I suggested u8 was to allow us using "%u" in the
snprintf, so any static checker would not try to account for a potential
'-' character. Because not doing so would spoil that approach for the
"usb%d_hsic_12M" string further down.

> > It leaves a bit of a bitter taste, though, as we shouldn't do this kind
> > type tweaking, especially not to work around the compiler trying to be
> > clever, but then not seeing the whole picture (that "i" is bounded by
> > compile time constants not exceeding "4").  
> 
> Yeah.  There is always the option of just ignoring the static checker
> when it tells you to write bad code.

Agreed on that, though I find those diagnostics useful, and just ignoring
or masking them might come back and haunt us later.

So I still think we should fix this, one way or the other.

But I feel this goes quite far into bikeshedding territory, so we should
probably just go with name[32].

Cheers,
Andre.

> You don't have to even look at the *whole* picture to know that GCC is
> wrong.  The BIT(i) would overflow if i is more than 31.
> 
> regards,
> dan carpenter
>