[v2] EDAC/thunderx: Fix some potential buffer overflow in thunderx_ocx_com_threaded_isr()

  strncat() usage in thunderx_ocx_com_threaded_isr() is wrong.
The size given to strncat() is the maximum number of bytes that can be
written, excluding the trailing NULL.

Here, the size of the 'msg' buffer is used (i.e. OCX_MESSAGE_SIZE), not
the space that is remaining.
The space for the ending NULL is also not taken into account.

in order to fix it:
   - call decode_register() before the snprintf() calls
   - use scnprintf() instead of snprintf() and compute, in the 'remaining'
     variable, the space that is still available in the 'msg' buffer
   - add a %s at the end of the format strings and append directly the
     result of decode_register() stored in 'other'
   - write directly at the right position in the 'msg' buffer when
     appending some data in the for loop.

Doing so, all usages of strncat() are removed.

Fixes: 41003396f932 ("EDAC, thunderx: Add Cavium ThunderX EDAC driver")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
This is cross-compile tested only.
Review with care.

v2: remove some other erroneous usage of strncat()
---
 drivers/edac/thunderx_edac.c | 44 ++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 24 deletions(-)
  

On Sat, Oct 21, 2023 at 07:13:51PM +0200, Christophe JAILLET wrote:
> strncat() usage in thunderx_ocx_com_threaded_isr() is wrong.
> The size given to strncat() is the maximum number of bytes that can be
> written, excluding the trailing NULL.
> 
> Here, the size of the 'msg' buffer is used (i.e. OCX_MESSAGE_SIZE), not
> the space that is remaining.
> The space for the ending NULL is also not taken into account.
> 
> in order to fix it:
>    - call decode_register() before the snprintf() calls
>    - use scnprintf() instead of snprintf() and compute, in the 'remaining'
>      variable, the space that is still available in the 'msg' buffer
>    - add a %s at the end of the format strings and append directly the
>      result of decode_register() stored in 'other'
>    - write directly at the right position in the 'msg' buffer when
>      appending some data in the for loop.
> 
> Doing so, all usages of strncat() are removed.
> 
> Fixes: 41003396f932 ("EDAC, thunderx: Add Cavium ThunderX EDAC driver")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> This is cross-compile tested only.
> Review with care.
> 
> v2: remove some other erroneous usage of strncat()
> ---
>  drivers/edac/thunderx_edac.c | 44 ++++++++++++++++--------------------
>  1 file changed, 20 insertions(+), 24 deletions(-)

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- You have marked a patch with a Fixes: tag for a commit that is in an
  older released kernel, yet you do not have a cc: stable line in the
  signed-off-by area at all, which means that the patch will not be
  applied to any older kernel releases.  To properly fix this, please
  follow the documented rules in the
  Documentation/process/stable-kernel-rules.rst file for how to resolve
  this.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot
  

On Sat, Oct 21, 2023 at 07:13:51PM +0200, Christophe JAILLET wrote:
> @@ -1127,27 +1128,26 @@ static irqreturn_t thunderx_ocx_com_threaded_isr(int irq, void *irq_id)
>  				ARRAY_SIZE(ocx->com_err_ctx));
>  		ctx = &ocx->com_err_ctx[tail];
>  
> -		snprintf(msg, OCX_MESSAGE_SIZE, "%s: OCX_COM_INT: %016llx",
> -			ocx->edac_dev->ctl_name, ctx->reg_com_int);
> -
>  		decode_register(other, OCX_OTHER_SIZE,
>  				ocx_com_errors, ctx->reg_com_int);
>  
> -		strncat(msg, other, OCX_MESSAGE_SIZE);
> +		remaining = OCX_MESSAGE_SIZE;
> +		remaining -= scnprintf(msg, remaining, "%s: OCX_COM_INT: %016llx%s",
> +				       ocx->edac_dev->ctl_name, ctx->reg_com_int,
> +				       other);
>  
>  		for (lane = 0; lane < OCX_RX_LANES; lane++)
>  			if (ctx->reg_com_int & BIT(lane)) {
> -				snprintf(other, OCX_OTHER_SIZE,
> -					 "\n\tOCX_LNE_INT[%02d]: %016llx OCX_LNE_STAT11[%02d]: %016llx",
> -					 lane, ctx->reg_lane_int[lane],
> -					 lane, ctx->reg_lane_stat11[lane]);
> -
> -				strncat(msg, other, OCX_MESSAGE_SIZE);
> -
>  				decode_register(other, OCX_OTHER_SIZE,
>  						ocx_lane_errors,
>  						ctx->reg_lane_int[lane]);
> -				strncat(msg, other, OCX_MESSAGE_SIZE);
> +
> +				remaining -= scnprintf(msg + (OCX_MESSAGE_SIZE - remaining),
> +						       remaining,

Instead of doing "remaining -=" the canonincal way is "off +=".  Then
the snprintf() becomes:

	off += scnprintf(msg + off, OCX_MESSAGE_SIZE - off, ""\n\tOCX_...

Your way works but it makes my head hurt.

regards,
dan carpenter
  

On Tue, Oct 24, 2023 at 08:35:33AM +0300, Dan Carpenter wrote:
> On Sat, Oct 21, 2023 at 07:13:51PM +0200, Christophe JAILLET wrote:
> > @@ -1127,27 +1128,26 @@ static irqreturn_t thunderx_ocx_com_threaded_isr(int irq, void *irq_id)
> >  				ARRAY_SIZE(ocx->com_err_ctx));
> >  		ctx = &ocx->com_err_ctx[tail];
> >  
> > -		snprintf(msg, OCX_MESSAGE_SIZE, "%s: OCX_COM_INT: %016llx",
> > -			ocx->edac_dev->ctl_name, ctx->reg_com_int);
> > -
> >  		decode_register(other, OCX_OTHER_SIZE,
> >  				ocx_com_errors, ctx->reg_com_int);
> >  
> > -		strncat(msg, other, OCX_MESSAGE_SIZE);
> > +		remaining = OCX_MESSAGE_SIZE;
> > +		remaining -= scnprintf(msg, remaining, "%s: OCX_COM_INT: %016llx%s",
> > +				       ocx->edac_dev->ctl_name, ctx->reg_com_int,
> > +				       other);
> >  
> >  		for (lane = 0; lane < OCX_RX_LANES; lane++)
> >  			if (ctx->reg_com_int & BIT(lane)) {
> > -				snprintf(other, OCX_OTHER_SIZE,
> > -					 "\n\tOCX_LNE_INT[%02d]: %016llx OCX_LNE_STAT11[%02d]: %016llx",
> > -					 lane, ctx->reg_lane_int[lane],
> > -					 lane, ctx->reg_lane_stat11[lane]);
> > -
> > -				strncat(msg, other, OCX_MESSAGE_SIZE);
> > -
> >  				decode_register(other, OCX_OTHER_SIZE,
> >  						ocx_lane_errors,
> >  						ctx->reg_lane_int[lane]);
> > -				strncat(msg, other, OCX_MESSAGE_SIZE);
> > +
> > +				remaining -= scnprintf(msg + (OCX_MESSAGE_SIZE - remaining),
> > +						       remaining,
> 
> Instead of doing "remaining -=" the canonincal way is "off +=".  Then
> the snprintf() becomes:
> 
> 	off += scnprintf(msg + off, OCX_MESSAGE_SIZE - off, ""\n\tOCX_...
> 
> Your way works but it makes my head hurt.

Sorry, I shouldn't have sent this email.  You're allowed to write it
however you want if you're fixing the bug.

regards,
dan carpenter
  

On Sat, Oct 21, 2023 at 07:13:51PM +0200, Christophe JAILLET wrote:
> strncat() usage in thunderx_ocx_com_threaded_isr() is wrong.
> The size given to strncat() is the maximum number of bytes that can be
> written, excluding the trailing NULL.
> 
> Here, the size of the 'msg' buffer is used (i.e. OCX_MESSAGE_SIZE), not
> the space that is remaining.
> The space for the ending NULL is also not taken into account.
> 
> in order to fix it:
>    - call decode_register() before the snprintf() calls
>    - use scnprintf() instead of snprintf() and compute, in the 'remaining'
>      variable, the space that is still available in the 'msg' buffer
>    - add a %s at the end of the format strings and append directly the
>      result of decode_register() stored in 'other'
>    - write directly at the right position in the 'msg' buffer when
>      appending some data in the for loop.
> 
> Doing so, all usages of strncat() are removed.
> 
> Fixes: 41003396f932 ("EDAC, thunderx: Add Cavium ThunderX EDAC driver")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> This is cross-compile tested only.
> Review with care.
> 
> v2: remove some other erroneous usage of strncat()
> ---
>  drivers/edac/thunderx_edac.c | 44 ++++++++++++++++--------------------
>  1 file changed, 20 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/edac/thunderx_edac.c b/drivers/edac/thunderx_edac.c
> index b9c5772da959..62e1e120178b 100644
> --- a/drivers/edac/thunderx_edac.c
> +++ b/drivers/edac/thunderx_edac.c
> @@ -1111,6 +1111,7 @@ static irqreturn_t thunderx_ocx_com_threaded_isr(int irq, void *irq_id)
>  
>  	unsigned long tail;
>  	struct ocx_com_err_ctx *ctx;
> +	size_t remaining;
>  	int lane;
>  	char *msg;
>  	char *other;
> @@ -1127,27 +1128,26 @@ static irqreturn_t thunderx_ocx_com_threaded_isr(int irq, void *irq_id)
>  				ARRAY_SIZE(ocx->com_err_ctx));
>  		ctx = &ocx->com_err_ctx[tail];
>  
> -		snprintf(msg, OCX_MESSAGE_SIZE, "%s: OCX_COM_INT: %016llx",
> -			ocx->edac_dev->ctl_name, ctx->reg_com_int);
> -
>  		decode_register(other, OCX_OTHER_SIZE,
>  				ocx_com_errors, ctx->reg_com_int);
>  
> -		strncat(msg, other, OCX_MESSAGE_SIZE);
> +		remaining = OCX_MESSAGE_SIZE;
> +		remaining -= scnprintf(msg, remaining, "%s: OCX_COM_INT: %016llx%s",
> +				       ocx->edac_dev->ctl_name, ctx->reg_com_int,
> +				       other);

As the replacements get longer, I would encourage you to use seq_buf
instead -- it does all the length math internally. For example:

	seq_buf s;

        msg = kmalloc(OCX_MESSAGE_SIZE, GFP_KERNEL);
	seq_buf_init(&s, msg, OCX_MESSAGE_SIZE);


	seq_buf_printf(&s, "%s: OCX_COM_INT: %016llx%s",
		       ocx->edac_dev->ctl_name,
		       ctx->reg_com_int, other);

	...
  		for (lane = 0; lane < OCX_RX_LANES; lane++)
  			if (ctx->reg_com_int & BIT(lane)) {
				...

				seq_buf_printf(&s, "\n\tOCX_LNE_INT[%02d]: %016llx OCX_LNE_STAT11[%02d]: %016llx%s",
	                                       lane, ctx->reg_lane_int[lane],
	                                       lane, ctx->reg_lane_stat11[lane],
	                                       other);
		}

	...
	seq_buf_terminate(&s);


etc...

Though I think the seq_buf API could use some improvement: it should
always maintainer a %NUL-terminated string so the final
seq_buf_terminate() isn't needed...
  

On Tue, Oct 24, 2023 at 04:39:36PM -0700, Kees Cook wrote:
> As the replacements get longer, I would encourage you to use seq_buf
> instead -- it does all the length math internally. For example:

There's some ongoing work to make seq_buf easier to use:
https://lore.kernel.org/lkml/20231026170722.work.638-kees@kernel.org/

Perhaps we can add an "alloc" and "free" pair too, to handle this case:

>	msg = kmalloc(OCX_MESSAGE_SIZE, GFP_KERNEL);
> 	seq_buf_init(&s, msg, OCX_MESSAGE_SIZE);

But perhaps it's overkill...