linux-kernel - Re: [PATCH v2 01/10] EDAC/mc: Fix usage of snprintf() and dimm location setup

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20200519092748.zalds3reudd22nky@rric.localdomain>
Date:   Tue, 19 May 2020 11:27:48 +0200
From:   Robert Richter <rrichter@...vell.com>
To:     Borislav Petkov <bp@...en8.de>
CC:     Mauro Carvalho Chehab <mchehab@...nel.org>,
        Tony Luck <tony.luck@...el.com>,
        James Morse <james.morse@....com>,
        Aristeu Rozanski <aris@...hat.com>,
        Matthias Brugger <mbrugger@...e.com>,
        <linux-edac@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 01/10] EDAC/mc: Fix usage of snprintf() and dimm
 location setup

On 22.04.20 22:52:43, Borislav Petkov wrote:
> On Wed, Apr 22, 2020 at 01:58:05PM +0200, Robert Richter wrote:
> > The setup of the dimm->location may be incomplete in case writing to
> > dimm->label fails due to small buffer size. Fix this by iterating
> > through all existing layers.
> > 
> > Also, the return value of snprintf() can be higher than the number of
> > bytes written to the buffer in case it is to small. Fix usage of
> > snprintf() by either porting it to scnprintf() or fixing the handling
> > of the return code.
> > 
> > It is very unlikely the buffer is too small in practice, but fixing it
> > anyway.
> > 
> > Signed-off-by: Robert Richter <rrichter@...vell.com>
> > ---
> >  drivers/edac/edac_mc.c | 20 ++++++++++----------
> >  1 file changed, 10 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
> > index 75ede27bdf6a..107d7c4de933 100644
> > --- a/drivers/edac/edac_mc.c
> > +++ b/drivers/edac/edac_mc.c
> > @@ -130,11 +130,11 @@ unsigned int edac_dimm_info_location(struct dimm_info *dimm, char *buf,
> >  		n = snprintf(p, len, "%s %d ",
> >  			      edac_layer_name[mci->layers[i].type],
> >  			      dimm->location[i]);
> > +		if (len <= n)
> > +			return count + len - 1;
> >  		p += n;
> >  		len -= n;
> >  		count += n;
> > -		if (!len)
> > -			break;
> >  	}
> >  
> >  	return count;
> > @@ -397,19 +397,19 @@ static int edac_mc_alloc_dimms(struct mem_ctl_info *mci)
> >  		 */
> >  		len = sizeof(dimm->label);
> >  		p = dimm->label;
> > -		n = snprintf(p, len, "mc#%u", mci->mc_idx);
> > +		n = scnprintf(p, len, "mc#%u", mci->mc_idx);
> >  		p += n;
> >  		len -= n;
> > +
> >  		for (layer = 0; layer < mci->n_layers; layer++) {
> > -			n = snprintf(p, len, "%s#%u",
> > -				     edac_layer_name[mci->layers[layer].type],
> > -				     pos[layer]);
> 
> The edac_layer_name[]'s are single words of a couple of letters and the
> pos is a number. The buffer we pass in is at least 80 chars and in one
> place even a PAGE_SIZE.
> 
> But in general, this is just silly with the buffers on stack and
> printing into them.
> 
> It would be much better to opencode that loop in
> edac_dimm_info_location() and simply dump those layer names at the call
> sites. And then kill that silly edac_dimm_info_location() function. See
> below for example.
> 
> And then since two call sites do edac_dbg(), you can put that in a
> function edac_dbg_dump_dimm_location() or so and call it and not care
> about any buffer lengths and s*printf's and so on.
> 
> Right?

The aim of this patch is just to fix snprintf() users. Anything else
would involve a larger cleanup. It is not only about edac_dbg(), there
are other users of edac_layer_name[] which implement similar things
that need to be looked at.

So I am dropping this patch from the series.

Thanks,

-Robert