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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAMRc=MfrQNc1idCD8fBbOx5bWzCU6f-Ryefu-eoxfzOaA8=_Fg@mail.gmail.com>
Date: Tue, 8 Oct 2024 09:13:27 +0200
From: Bartosz Golaszewski <brgl@...ev.pl>
To: Christophe JAILLET <christophe.jaillet@...adoo.fr>
Cc: Ulf Hansson <ulf.hansson@...aro.org>, linux-mmc@...r.kernel.org, 
	linux-kernel@...r.kernel.org, 
	Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
Subject: Re: [PATCH] mmc: mmc_spi: fix snprintf() output buffer size

On Mon, Oct 7, 2024 at 8:09 PM Christophe JAILLET
<christophe.jaillet@...adoo.fr> wrote:
>
> Le 07/10/2024 à 13:45, Bartosz Golaszewski a écrit :
> > From: Bartosz Golaszewski <bartosz.golaszewski@...aro.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@...aro.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