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

[Borislav Petkov]

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