[v2,01/10] EDAC/mc: Fix usage of snprintf() and dimm location setup
diff mbox series

Message ID 20200422115814.22205-2-rrichter@marvell.com
State New
Headers show
Series
  • EDAC/mc/ghes: Fixes, cleanup and reworks
Related show

Commit Message

Robert Richter April 22, 2020, 11:58 a.m. UTC
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(-)

Comments

Borislav Petkov April 22, 2020, 8:52 p.m. UTC | #1
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);
Robert Richter May 19, 2020, 9:27 a.m. UTC | #2
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

Patch
diff mbox series

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 */