netdev - Re: [PATCH] [v2] hinic: avoid gcc -Wrestrict warning

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]

Date:   Wed, 24 Mar 2021 15:25:47 +0100
From:   Arnd Bergmann <arnd@...nel.org>
To:     Rasmus Villemoes <linux@...musvillemoes.dk>
Cc:     Bin Luo <luobin9@...wei.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Networking <netdev@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] [v2] hinic: avoid gcc -Wrestrict warning

On Wed, Mar 24, 2021 at 2:29 PM Rasmus Villemoes
<linux@...musvillemoes.dk> wrote:
> On 24/03/2021 14.07, Arnd Bergmann wrote:
 >
> >       if (set_settings & HILINK_LINK_SET_SPEED) {
> >               speed_level = hinic_ethtool_to_hw_speed_level(speed);
> >               err = snprintf(set_link_str, SET_LINK_STR_MAX_LEN,
> > -                            "%sspeed %d ", set_link_str, speed);
> > -             if (err <= 0 || err >= SET_LINK_STR_MAX_LEN) {
> > +                            "speed %d ", speed);
> > +             if (err >= SET_LINK_STR_MAX_LEN) {
> >                       netif_err(nic_dev, drv, netdev, "Failed to snprintf link speed, function return(%d) and dest_len(%d)\n",
> >                                 err, SET_LINK_STR_MAX_LEN);
> >                       return -EFAULT;
>
> It's not your invention of course, but this both seems needlessly harsh
> and EFAULT is a weird error to return. It's just a printk() message that
> might be truncated, and now that the format string only has a %d
> specifier, it can actually be verified statically that overflow will
> never happen (though I don't know or think gcc can do that, perhaps
> there's some locale nonsense in the standard that allows using
> utf16-encoded sanskrit runes). So probably that test should just be
> dropped, but that's a separate thing.

I thought about fixing it, but this seemed to be a rabbit hole I didn't
want to get into, as there are other harmless issues in the driver
that could be improved.

I'm fairly sure gcc can indeed warn about the overflow with
-Wformat-truncation, but the warning is disabled at the moment
because it has a ton of false positives:

kernel/workqueue.c: In function 'create_worker':
kernel/workqueue.c:1933:55: error: '%d' directive output may be
truncated writing between 1 and 10 bytes into a region of size between
3 and 13 [-Werror=format-truncation=]
 1933 |                 snprintf(id_buf, sizeof(id_buf), "u%d:%d",
pool->id, id);

        Arnd