-Wfortify-source in kernel/printk/printk.c

-Wfortify-source in kernel/printk/printk.c

[Nathan Chancellor]

Hi all,

After commit 6d485ff455e ("Improve static checks for sprintf and
__builtin___sprintf_chk") in clang [1], the following warning appears
when CONFIG_PRINTK is disabled (e.g. allnoconfig):

../kernel/printk/printk.c:2416:10: warning: 'sprintf' will always
overflow; destination buffer has size 0, but format string expands
to at least 33 [-Wfortify-source]
                        len = sprintf(text,
                              ^
1 warning generated.

Specifically referring to
https://elixir.bootlin.com/linux/v5.5/source/kernel/printk/printk.c#L2416.

It isn't wrong, given that when CONFIG_PRINTK is disabled, text's length
is 0 (LOG_LINE_MAX and PREFIX_MAX are both zero). How should this
warning be dealt this? I am not familiar enough with the printk code to
say myself.

[1]: https://github.com/llvm/llvm-project/commit/6d485ff455ea2b37fef9e06e426dae6c1241b231

Cheers,
Nathan

Re: -Wfortify-source in kernel/printk/printk.c

[Sergey Senozhatsky]

On (20/01/29 19:16), Nathan Chancellor wrote:
> Hi all,
>
> After commit 6d485ff455e ("Improve static checks for sprintf and
> __builtin___sprintf_chk") in clang [1], the following warning appears
> when CONFIG_PRINTK is disabled (e.g. allnoconfig):
>
> ../kernel/printk/printk.c:2416:10: warning: 'sprintf' will always
> overflow; destination buffer has size 0, but format string expands
> to at least 33 [-Wfortify-source]
>                         len = sprintf(text,
>                               ^
> 1 warning generated.
>
> Specifically referring to
> https://elixir.bootlin.com/linux/v5.5/source/kernel/printk/printk.c#L2416.

Good catch.

> It isn't wrong, given that when CONFIG_PRINTK is disabled, text's length
> is 0 (LOG_LINE_MAX and PREFIX_MAX are both zero). How should this
> warning be dealt this? I am not familiar enough with the printk code to
> say myself.

It's not wrong.

Unless I'm missing something completely obvious: with disabled printk()
we don't have any functions that can append messages to the logbuf, hence
we can't overflow it. So the error in question should never trigger.

- Normal printk() is void, so kernel cannot append messages;
- dev_printk() is void, so drivers cannot append messages and dicts;
- devkmsg_write() is void, so user space cannot write to logbuf.

So I think we should never trigger that overflow (assuming that I
didn't miss something) message.

In any case feel free to submit a patch - switch it to snprintf().

	-ss

Re: -Wfortify-source in kernel/printk/printk.c

[Joe Perches]

On Thu, 2020-01-30 at 14:17 +0900, Sergey Senozhatsky wrote:
> On (20/01/29 19:16), Nathan Chancellor wrote:
> > Hi all,
> > 
> > After commit 6d485ff455e ("Improve static checks for sprintf and
> > __builtin___sprintf_chk") in clang [1], the following warning appears
> > when CONFIG_PRINTK is disabled (e.g. allnoconfig):
> > 
> > ../kernel/printk/printk.c:2416:10: warning: 'sprintf' will always
> > overflow; destination buffer has size 0, but format string expands
> > to at least 33 [-Wfortify-source]
> >                         len = sprintf(text,
> >                               ^
> > 1 warning generated.
> > 
> > Specifically referring to
> > https://elixir.bootlin.com/linux/v5.5/source/kernel/printk/printk.c#L2416.
> 
> Good catch.
> 
> > It isn't wrong, given that when CONFIG_PRINTK is disabled, text's length
> > is 0 (LOG_LINE_MAX and PREFIX_MAX are both zero). How should this
> > warning be dealt this? I am not familiar enough with the printk code to
> > say myself.
> 
> It's not wrong.
> 
> Unless I'm missing something completely obvious: with disabled printk()
> we don't have any functions that can append messages to the logbuf, hence
> we can't overflow it. So the error in question should never trigger.
> 
> - Normal printk() is void, so kernel cannot append messages;
> - dev_printk() is void, so drivers cannot append messages and dicts;
> - devkmsg_write() is void, so user space cannot write to logbuf.
> 
> So I think we should never trigger that overflow (assuming that I
> didn't miss something) message.
> 
> In any case feel free to submit a patch - switch it to snprintf().

and/or make the code depend on CONFIG_PRINTK

Re: -Wfortify-source in kernel/printk/printk.c

[Sergey Senozhatsky]

On (20/01/29 22:39), Joe Perches wrote:
> > > It isn't wrong, given that when CONFIG_PRINTK is disabled, text's length
> > > is 0 (LOG_LINE_MAX and PREFIX_MAX are both zero). How should this
> > > warning be dealt this? I am not familiar enough with the printk code to
> > > say myself.
> > 
> > It's not wrong.
> > 
> > Unless I'm missing something completely obvious: with disabled printk()
> > we don't have any functions that can append messages to the logbuf, hence
> > we can't overflow it. So the error in question should never trigger.
> > 
> > - Normal printk() is void, so kernel cannot append messages;
> > - dev_printk() is void, so drivers cannot append messages and dicts;
> > - devkmsg_write() is void, so user space cannot write to logbuf.
> > 
> > So I think we should never trigger that overflow (assuming that I
> > didn't miss something) message.
> > 
> > In any case feel free to submit a patch - switch it to snprintf().
> 
> and/or make the code depend on CONFIG_PRINTK

console_unlock() still needs to at least up() the console semaphore.
We don't have printk(), but the tty subsystem is still there and serial
consoles and definitely still up and running. So... maybe we can have
two versions of console_unlock().

	-ss