mmc: mmc_spi: fix snprintf() output buffer size

[PATCH] mmc: mmc_spi: fix snprintf() output buffer size

Posted by Bartosz Golaszewski 1 month, 3 weeks ago

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

GCC 13 complains about the truncated output of snprintf():

drivers/mmc/host/mmc_spi.c: In function ‘mmc_spi_response_get’:
drivers/mmc/host/mmc_spi.c:227:64: error: ‘snprintf’ output may be truncated before the last format character [-Werror=format-truncation=]
  227 |         snprintf(tag, sizeof(tag), "  ... CMD%d response SPI_%s",
      |                                                                ^
drivers/mmc/host/mmc_spi.c:227:9: note: ‘snprintf’ output between 26 and 43 bytes into a destination of size 32
  227 |         snprintf(tag, sizeof(tag), "  ... CMD%d response SPI_%s",
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  228 |                 cmd->opcode, maptype(cmd));

Increase the size of the target buffer.

Fixes: 15a0580ced08 ("mmc_spi host driver")
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/mmc/host/mmc_spi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
index 8fee7052f2ef..fa1d1a1b3142 100644
--- a/drivers/mmc/host/mmc_spi.c
+++ b/drivers/mmc/host/mmc_spi.c
@@ -222,7 +222,7 @@ static int mmc_spi_response_get(struct mmc_spi_host *host,
 	u8 	leftover = 0;
 	unsigned short rotator;
 	int 	i;
-	char	tag[32];
+	char	tag[43];
 
 	snprintf(tag, sizeof(tag), "  ... CMD%d response SPI_%s",
 		cmd->opcode, maptype(cmd));
-- 
2.43.0

Re: [PATCH] mmc: mmc_spi: fix snprintf() output buffer size

Posted by Christophe JAILLET 1 month, 3 weeks ago

Le 07/10/2024 à 13:45, Bartosz Golaszewski a écrit :
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> GCC 13 complains about the truncated output of snprintf():
> 
> drivers/mmc/host/mmc_spi.c: In function ‘mmc_spi_response_get’:
> drivers/mmc/host/mmc_spi.c:227:64: error: ‘snprintf’ output may be truncated before the last format character [-Werror=format-truncation=]
>    227 |         snprintf(tag, sizeof(tag), "  ... CMD%d response SPI_%s",
>        |                                                                ^
> drivers/mmc/host/mmc_spi.c:227:9: note: ‘snprintf’ output between 26 and 43 bytes into a destination of size 32
>    227 |         snprintf(tag, sizeof(tag), "  ... CMD%d response SPI_%s",
>        |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>    228 |                 cmd->opcode, maptype(cmd));
> 
> Increase the size of the target buffer.
> 
> Fixes: 15a0580ced08 ("mmc_spi host driver")
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>   drivers/mmc/host/mmc_spi.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
> index 8fee7052f2ef..fa1d1a1b3142 100644
> --- a/drivers/mmc/host/mmc_spi.c
> +++ b/drivers/mmc/host/mmc_spi.c
> @@ -222,7 +222,7 @@ static int mmc_spi_response_get(struct mmc_spi_host *host,
>   	u8 	leftover = 0;
>   	unsigned short rotator;
>   	int 	i;
> -	char	tag[32];
> +	char	tag[43];
>   
>   	snprintf(tag, sizeof(tag), "  ... CMD%d response SPI_%s",
>   		cmd->opcode, maptype(cmd));

'tag' is only used in a dev_dbg() at the end of the function.

Maybe "  ... CMD%d response SPI_%s" could me moved directly within the 
dev_dbg(). This would save a few bytes on the stack, save a snprintf() 
in the normal path and fix the warning without the need of magic number.

just my 2c.

CJ

Re: [PATCH] mmc: mmc_spi: fix snprintf() output buffer size

Posted by Bartosz Golaszewski 1 month, 2 weeks ago

On Mon, Oct 7, 2024 at 8:09 PM Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:
>
> Le 07/10/2024 à 13:45, Bartosz Golaszewski a écrit :
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > GCC 13 complains about the truncated output of snprintf():
> >
> > drivers/mmc/host/mmc_spi.c: In function ‘mmc_spi_response_get’:
> > drivers/mmc/host/mmc_spi.c:227:64: error: ‘snprintf’ output may be truncated before the last format character [-Werror=format-truncation=]
> >    227 |         snprintf(tag, sizeof(tag), "  ... CMD%d response SPI_%s",
> >        |                                                                ^
> > drivers/mmc/host/mmc_spi.c:227:9: note: ‘snprintf’ output between 26 and 43 bytes into a destination of size 32
> >    227 |         snprintf(tag, sizeof(tag), "  ... CMD%d response SPI_%s",
> >        |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >    228 |                 cmd->opcode, maptype(cmd));
> >
> > Increase the size of the target buffer.
> >
> > Fixes: 15a0580ced08 ("mmc_spi host driver")
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > ---
> >   drivers/mmc/host/mmc_spi.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
> > index 8fee7052f2ef..fa1d1a1b3142 100644
> > --- a/drivers/mmc/host/mmc_spi.c
> > +++ b/drivers/mmc/host/mmc_spi.c
> > @@ -222,7 +222,7 @@ static int mmc_spi_response_get(struct mmc_spi_host *host,
> >       u8      leftover = 0;
> >       unsigned short rotator;
> >       int     i;
> > -     char    tag[32];
> > +     char    tag[43];
> >
> >       snprintf(tag, sizeof(tag), "  ... CMD%d response SPI_%s",
> >               cmd->opcode, maptype(cmd));
>
> 'tag' is only used in a dev_dbg() at the end of the function.
>
> Maybe "  ... CMD%d response SPI_%s" could me moved directly within the
> dev_dbg(). This would save a few bytes on the stack, save a snprintf()
> in the normal path and fix the warning without the need of magic number.
>
> just my 2c.
>
> CJ

I would be hesitant to change this logic here. The cmd struct is
manipulated pretty extensively later in the function which leads me to
believe that this snprintf() here was done on purpose.

Bart

Re: [PATCH] mmc: mmc_spi: fix snprintf() output buffer size

Posted by Ulf Hansson 1 month, 2 weeks ago

On Tue, 8 Oct 2024 at 09:13, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> On Mon, Oct 7, 2024 at 8:09 PM Christophe JAILLET
> <christophe.jaillet@wanadoo.fr> wrote:
> >
> > Le 07/10/2024 à 13:45, Bartosz Golaszewski a écrit :
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > GCC 13 complains about the truncated output of snprintf():
> > >
> > > drivers/mmc/host/mmc_spi.c: In function ‘mmc_spi_response_get’:
> > > drivers/mmc/host/mmc_spi.c:227:64: error: ‘snprintf’ output may be truncated before the last format character [-Werror=format-truncation=]
> > >    227 |         snprintf(tag, sizeof(tag), "  ... CMD%d response SPI_%s",
> > >        |                                                                ^
> > > drivers/mmc/host/mmc_spi.c:227:9: note: ‘snprintf’ output between 26 and 43 bytes into a destination of size 32
> > >    227 |         snprintf(tag, sizeof(tag), "  ... CMD%d response SPI_%s",
> > >        |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >    228 |                 cmd->opcode, maptype(cmd));
> > >
> > > Increase the size of the target buffer.
> > >
> > > Fixes: 15a0580ced08 ("mmc_spi host driver")
> > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > ---
> > >   drivers/mmc/host/mmc_spi.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/mmc/host/mmc_spi.c b/drivers/mmc/host/mmc_spi.c
> > > index 8fee7052f2ef..fa1d1a1b3142 100644
> > > --- a/drivers/mmc/host/mmc_spi.c
> > > +++ b/drivers/mmc/host/mmc_spi.c
> > > @@ -222,7 +222,7 @@ static int mmc_spi_response_get(struct mmc_spi_host *host,
> > >       u8      leftover = 0;
> > >       unsigned short rotator;
> > >       int     i;
> > > -     char    tag[32];
> > > +     char    tag[43];
> > >
> > >       snprintf(tag, sizeof(tag), "  ... CMD%d response SPI_%s",
> > >               cmd->opcode, maptype(cmd));
> >
> > 'tag' is only used in a dev_dbg() at the end of the function.
> >
> > Maybe "  ... CMD%d response SPI_%s" could me moved directly within the
> > dev_dbg(). This would save a few bytes on the stack, save a snprintf()
> > in the normal path and fix the warning without the need of magic number.
> >
> > just my 2c.
> >
> > CJ
>
> I would be hesitant to change this logic here. The cmd struct is
> manipulated pretty extensively later in the function which leads me to
> believe that this snprintf() here was done on purpose.

The cmd->opcode and cmd->flags aren't really something a host driver
should need to change. It's the core that sets them to inform the host
driver about the command.

I looked closer and it seems to be correct in this case too.

Kind regards
Uffe