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