linux-kernel - Re: [PATCH V2] perf cpumap: Fix snprintf overflow check

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, 25 Mar 2020 09:19:12 -0300
From:   Arnaldo Carvalho de Melo <arnaldo.melo@...il.com>
To:     Christophe JAILLET <christophe.jaillet@...adoo.fr>
Cc:     Arnaldo Carvalho de Melo <arnaldo.melo@...il.com>,
        peterz@...radead.org, mingo@...hat.com, mark.rutland@....com,
        alexander.shishkin@...ux.intel.com, jolsa@...hat.com,
        namhyung@...nel.org, kan.liang@...ux.intel.com,
        zhe.he@...driver.com, dzickus@...hat.com, jstancek@...hat.com,
        David Laight <David.Laight@...lab.com>,
        linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org
Subject: Re: [PATCH V2] perf cpumap: Fix snprintf overflow check

Em Tue, Mar 24, 2020 at 03:44:15PM +0100, Christophe JAILLET escreveu:
> Le 24/03/2020 à 13:50, Arnaldo Carvalho de Melo a écrit :
> > Em Tue, Mar 24, 2020 at 08:03:19AM +0100, Christophe JAILLET escreveu:
> > > 'snprintf' returns the number of characters which would be generated for
> > > the given input.
> > > 
> > > If the returned value is *greater than* or equal to the buffer size, it
> > > means that the output has been truncated.
> > > 
> > > Fix the overflow test accordingling.
> >                                    y
> > 
> > You forgot to CC David and add this to your patch, which I did:
> > 
> > Suggested-by: David Laight <David.Laight@...LAB.COM>
> > 
> > Ok?
> 
> No problem for me.
> 
> The to: and cc: list are (at least should be :)) the ones given by
> get_maintainer, and I was not aware of the 'Suggested-by' tag.

Sure, that is the starting point, but if someone participates in the
discussion and provides suggestions, comments, then it is sensible to
add them to the CC list in follow up patches.
 
> BTW, thanks for fixing the typo in the description.

Part of my job as a maintainer :-)

- Arnaldo
 
> CJ
> 
> 
> > - Arnaldo
> > > Fixes: 7780c25bae59f ("perf tools: Allow ability to map cpus to nodes easily")
> > > Fixes: 92a7e1278005b ("perf cpumap: Add cpu__max_present_cpu()")
> > > Signed-off-by: Christophe JAILLET <christophe.jaillet@...adoo.fr>
> > > ---
> > > V2: keep snprintf
> > >      modifiy the tests for truncated output
> > >      Update subject and description
> > > ---
> > >   tools/perf/util/cpumap.c | 10 +++++-----
> > >   1 file changed, 5 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
> > > index 983b7388f22b..dc5c5e6fc502 100644
> > > --- a/tools/perf/util/cpumap.c
> > > +++ b/tools/perf/util/cpumap.c
> > > @@ -317,7 +317,7 @@ static void set_max_cpu_num(void)
> > >   	/* get the highest possible cpu number for a sparse allocation */
> > >   	ret = snprintf(path, PATH_MAX, "%s/devices/system/cpu/possible", mnt);
> > > -	if (ret == PATH_MAX) {
> > > +	if (ret >= PATH_MAX) {
> > >   		pr_err("sysfs path crossed PATH_MAX(%d) size\n", PATH_MAX);
> > >   		goto out;
> > >   	}
> > > @@ -328,7 +328,7 @@ static void set_max_cpu_num(void)
> > >   	/* get the highest present cpu number for a sparse allocation */
> > >   	ret = snprintf(path, PATH_MAX, "%s/devices/system/cpu/present", mnt);
> > > -	if (ret == PATH_MAX) {
> > > +	if (ret >= PATH_MAX) {
> > >   		pr_err("sysfs path crossed PATH_MAX(%d) size\n", PATH_MAX);
> > >   		goto out;
> > >   	}
> > > @@ -356,7 +356,7 @@ static void set_max_node_num(void)
> > >   	/* get the highest possible cpu number for a sparse allocation */
> > >   	ret = snprintf(path, PATH_MAX, "%s/devices/system/node/possible", mnt);
> > > -	if (ret == PATH_MAX) {
> > > +	if (ret >= PATH_MAX) {
> > >   		pr_err("sysfs path crossed PATH_MAX(%d) size\n", PATH_MAX);
> > >   		goto out;
> > >   	}
> > > @@ -441,7 +441,7 @@ int cpu__setup_cpunode_map(void)
> > >   		return 0;
> > >   	n = snprintf(path, PATH_MAX, "%s/devices/system/node", mnt);
> > > -	if (n == PATH_MAX) {
> > > +	if (n >= PATH_MAX) {
> > >   		pr_err("sysfs path crossed PATH_MAX(%d) size\n", PATH_MAX);
> > >   		return -1;
> > >   	}
> > > @@ -456,7 +456,7 @@ int cpu__setup_cpunode_map(void)
> > >   			continue;
> > >   		n = snprintf(buf, PATH_MAX, "%s/%s", path, dent1->d_name);
> > > -		if (n == PATH_MAX) {
> > > +		if (n >= PATH_MAX) {
> > >   			pr_err("sysfs path crossed PATH_MAX(%d) size\n", PATH_MAX);
> > >   			continue;
> > >   		}
> > > -- 
> > > 2.20.1
> > > 
> 

-- 

- Arnaldo