Re: [PATCH v2 01/10] EDAC/mc: Fix usage of snprintf() and dimm location setup
From: Borislav Petkov
Date: Wed Apr 22 2020 - 16:52:51 EST
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@xxxxxxxxxxx>
> ---
> 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?
---
diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index 422120793a6b..7c04ef0c3536 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -91,16 +91,23 @@ static void edac_mc_dump_channel(struct rank_info *chan)
static void edac_mc_dump_dimm(struct dimm_info *dimm)
{
- char location[80];
+ struct mem_ctl_info *mci = dimm->mci;
+ int i;
if (!dimm->nr_pages)
return;
- edac_dimm_info_location(dimm, location, sizeof(location));
+ edac_dbg(4, "%s%i: ", dimm->mci->csbased ? "rank" : "dimm", dimm->idx);
+
+ for (i = 0; i < mci->n_layers; i++)
+ edac_dbg(4, "%s %d ",
+ edac_layer_name[mci->layers[i].type],
+ dimm->location[i]);
+
+ edac_dbg(4, "mapped as virtual row %d, chan %d\n",
+ dimm->csrow, dimm->cschannel);
- edac_dbg(4, "%s%i: %smapped as virtual row %d, chan %d\n",
- dimm->mci->csbased ? "rank" : "dimm",
- dimm->idx, location, dimm->csrow, dimm->cschannel);
edac_dbg(4, " dimm = %p\n", dimm);
edac_dbg(4, " dimm->label = '%s'\n", dimm->label);
edac_dbg(4, " dimm->nr_pages = 0x%x\n", dimm->nr_pages);
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette