netdev - Re: [PATCH net-next] iavf: Avoid a memory allocation in iavf_print_link

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]

Date: Wed, 4 Oct 2023 11:04:38 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Jesse Brandeburg <jesse.brandeburg@...el.com>
Cc: Christophe JAILLET <christophe.jaillet@...adoo.fr>,
	Tony Nguyen <anthony.l.nguyen@...el.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org,
	intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org
Subject: Re: [PATCH net-next] iavf: Avoid a memory allocation in
 iavf_print_link_message()

On Tue, Oct 03, 2023 at 04:01:18PM -0700, Jesse Brandeburg wrote:
> On 10/3/2023 1:33 PM, Christophe JAILLET wrote:
> > kasprintf() is much better.
> 
> cool! I just sent the patches and cc'd you earlier today.
> 
> > > 
> > > your patch still shows these errors
> > 
> > I built-tested the patch before sending, so this is strange.
> > 
> > However, I got a similar feedback from Greg KH and the "kernel test
> > robot" for another similar patch.
> > 
> > What version of gcc do you use?
> > I use 12.3.0, and I suspect that the value range algorithm or how the
> > diagnostic is done has been improved in recent gcc.
> 
> Fedora gcc 12.3.1, with W=1 flag
> 
> gcc version 12.3.1 20230508 (Red Hat 12.3.1-1) (GCC)
> 
> [linux]$ make W=1 M=drivers/net/ethernet/intel/iavf
>   CC [M]  drivers/net/ethernet/intel/iavf/iavf_main.o
>   CC [M]  drivers/net/ethernet/intel/iavf/iavf_ethtool.o
>   CC [M]  drivers/net/ethernet/intel/iavf/iavf_virtchnl.o
>   CC [M]  drivers/net/ethernet/intel/iavf/iavf_fdir.o
>   CC [M]  drivers/net/ethernet/intel/iavf/iavf_adv_rss.o
>   CC [M]  drivers/net/ethernet/intel/iavf/iavf_txrx.o
>   CC [M]  drivers/net/ethernet/intel/iavf/iavf_common.o
>   CC [M]  drivers/net/ethernet/intel/iavf/iavf_adminq.o
>   CC [M]  drivers/net/ethernet/intel/iavf/iavf_client.o
> drivers/net/ethernet/intel/iavf/iavf_virtchnl.c: In function
> ‘iavf_virtchnl_completion’:
> drivers/net/ethernet/intel/iavf/iavf_virtchnl.c:1446:60: warning: ‘%s’
> directive output may be truncated writing 4 bytes into a region of size
> between 1 and 11 [-Wformat-truncation=]
>  1446 |                 snprintf(speed, IAVF_MAX_SPEED_STRLEN, "%d %s",
>       |                                                            ^~
>  1447 |                          link_speed_mbps, "Mbps");
>       |                                           ~~~~~~

GCC is kind of crap at static analysis, right?  Smatch would know that
this at most 11 characters long.  It's kind of laziness for GCC to print
this warning.  If you complained to me about a false positive like this
in Smatch I would at least think about various ways to silence it.

But I probably wouldn't write a check for this anyway because I don't
view truncating strings as a note worthy bug...

Smatch also gets stuff wrong, but in that case I just always encourage
people to mark the warning as old news and move on.  Only new warnings
are interesting.

I feel like as we incorporate more and more static analysis into our
processes we're going to have to give up on trying to keep every static
checker happy.

regards,
dan carpenter