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

[Robert Richter <rrichter@marvell.com>]

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@marvell.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