| Message ID | 20200422115814.22205-2-rrichter@marvell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | EDAC/mc/ghes: Fixes, cleanup and reworks | expand |
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? --- 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);
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
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]); + dimm->location[layer] = pos[layer]; + if (len <= 1) + continue; + n = scnprintf(p, len, "%s#%u", + edac_layer_name[mci->layers[layer].type], + pos[layer]); p += n; len -= n; - dimm->location[layer] = pos[layer]; - - if (len <= 0) - break; } /* Link it to the csrows old API data */
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(-)