As discussed in [1] add a prototype for __fortify_panic() to fix the
'make W=1 C=1' warning:
arch/x86/boot/compressed/misc.c:535:6: warning: symbol '__fortify_panic' was not declared. Should it be static?
Link: https://lore.kernel.org/all/[email protected]/ [1]
Signed-off-by: Jeff Johnson <[email protected]>
---
arch/x86/boot/compressed/misc.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
index b353a7be380c..3a56138484a9 100644
--- a/arch/x86/boot/compressed/misc.h
+++ b/arch/x86/boot/compressed/misc.h
@@ -68,6 +68,7 @@ void __putdec(unsigned long value);
#define error_putstr(__x) __putstr(__x)
#define error_puthex(__x) __puthex(__x)
#define error_putdec(__x) __putdec(__x)
+void __fortify_panic(const u8 reason, size_t avail, size_t size);
#ifdef CONFIG_X86_VERBOSE_BOOTUP
---
base-commit: e0cce98fe279b64f4a7d81b7f5c3a23d80b92fbc
change-id: 20240529-fortify_panic-325601efe71d
On 29.05.24 г. 21:09 ч., Jeff Johnson wrote:
> As discussed in [1] add a prototype for __fortify_panic() to fix the
> 'make W=1 C=1' warning:
>
> arch/x86/boot/compressed/misc.c:535:6: warning: symbol '__fortify_panic' was not declared. Should it be static?
Actually doesn't it make sense to have this defined under ../string.h ?
Actually given that we don't have any string fortification under the
boot/ why have the fortify _* functions at all ?
>
> Link: https://lore.kernel.org/all/[email protected]/ [1]
> Signed-off-by: Jeff Johnson <[email protected]>
> ---
> arch/x86/boot/compressed/misc.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
> index b353a7be380c..3a56138484a9 100644
> --- a/arch/x86/boot/compressed/misc.h
> +++ b/arch/x86/boot/compressed/misc.h
> @@ -68,6 +68,7 @@ void __putdec(unsigned long value);
> #define error_putstr(__x) __putstr(__x)
> #define error_puthex(__x) __puthex(__x)
> #define error_putdec(__x) __putdec(__x)
> +void __fortify_panic(const u8 reason, size_t avail, size_t size);
>
> #ifdef CONFIG_X86_VERBOSE_BOOTUP
>
>
> ---
> base-commit: e0cce98fe279b64f4a7d81b7f5c3a23d80b92fbc
> change-id: 20240529-fortify_panic-325601efe71d
>
>
On 5/30/2024 8:42 AM, Nikolay Borisov wrote:
>
>
> On 29.05.24 г. 21:09 ч., Jeff Johnson wrote:
>> As discussed in [1] add a prototype for __fortify_panic() to fix the
>> 'make W=1 C=1' warning:
>>
>> arch/x86/boot/compressed/misc.c:535:6: warning: symbol '__fortify_panic' was not declared. Should it be static?
>
> Actually doesn't it make sense to have this defined under ../string.h ?
> Actually given that we don't have any string fortification under the
> boot/ why have the fortify _* functions at all ?
I'll let Kees answer these questions since I just took guidance from him :)
/jeff
On May 30, 2024 6:23:36 PM GMT+02:00, Jeff Johnson <[email protected]> wrote:
>On 5/30/2024 8:42 AM, Nikolay Borisov wrote:
>>
>>
>> On 29.05.24 г. 21:09 ч., Jeff Johnson wrote:
>>> As discussed in [1] add a prototype for __fortify_panic() to fix the
>>> 'make W=1 C=1' warning:
>>>
>>> arch/x86/boot/compressed/misc.c:535:6: warning: symbol '__fortify_panic' was not declared. Should it be static?
>>
>> Actually doesn't it make sense to have this defined under ../string.h ?
>> Actually given that we don't have any string fortification under the
>> boot/ why have the fortify _* functions at all ?
>
>I'll let Kees answer these questions since I just took guidance from him :)
The more important question is how does the decompressor build even know of this symbol? And then make it forget it again instead of adding silly prototypes...
--
Sent from a small device: formatting sucks and brevity is inevitable.
On Thu, May 30, 2024 at 09:23:36AM -0700, Jeff Johnson wrote:
> On 5/30/2024 8:42 AM, Nikolay Borisov wrote:
> >
> >
> > On 29.05.24 г. 21:09 ч., Jeff Johnson wrote:
> >> As discussed in [1] add a prototype for __fortify_panic() to fix the
> >> 'make W=1 C=1' warning:
> >>
> >> arch/x86/boot/compressed/misc.c:535:6: warning: symbol '__fortify_panic' was not declared. Should it be static?
> >
> > Actually doesn't it make sense to have this defined under ../string.h ?
> > Actually given that we don't have any string fortification under the
> > boot/ why have the fortify _* functions at all ?
>
> I'll let Kees answer these questions since I just took guidance from him :)
Ah-ha, I see what's happening. When not built with
CONFIG_FORTIFY_SOURCE, fortify-string.h isn't included. But since misc.c
has the function definition, we get a warning that the function
declaration was never seen. This is likely the better solution:
diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index b70e4a21c15f..3f21a5e218f8 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -532,7 +532,9 @@ asmlinkage __visible void *extract_kernel(void *rmode, unsigned char *output)
return output + entry_offset;
}
+#ifdef CONFIG_FORTIFY_SOURCE
void __fortify_panic(const u8 reason, size_t avail, size_t size)
{
error("detected buffer overflow");
}
+#endif
Jeff, can you test this? (I still haven't been able to reproduce the
warning.)
--
Kees Cook
On Thu, May 30, 2024 at 06:46:39PM +0200, Borislav Petkov wrote:
> On May 30, 2024 6:23:36 PM GMT+02:00, Jeff Johnson <[email protected]> wrote:
> >On 5/30/2024 8:42 AM, Nikolay Borisov wrote:
> >>
> >>
> >> On 29.05.24 г. 21:09 ч., Jeff Johnson wrote:
> >>> As discussed in [1] add a prototype for __fortify_panic() to fix the
> >>> 'make W=1 C=1' warning:
> >>>
> >>> arch/x86/boot/compressed/misc.c:535:6: warning: symbol '__fortify_panic' was not declared. Should it be static?
> >>
> >> Actually doesn't it make sense to have this defined under ../string.h ?
> >> Actually given that we don't have any string fortification under the
> >> boot/ why have the fortify _* functions at all ?
> >
> >I'll let Kees answer these questions since I just took guidance from him :)
>
> The more important question is how does the decompressor build even know of this symbol? And then make it forget it again instead of adding silly prototypes...
Under CONFIG_FORTIFY_SOURCE, the boot code *does* still uses
fortify-string.h. It lets us both catch mistakes we can discover at
compile and will catch egregious runtime mistakes, though the reporting
is much simpler in the boot code.
--
Kees Cook
On 5/31/2024 9:28 AM, Kees Cook wrote:
> On Thu, May 30, 2024 at 09:23:36AM -0700, Jeff Johnson wrote:
>> On 5/30/2024 8:42 AM, Nikolay Borisov wrote:
>>>
>>>
>>> On 29.05.24 г. 21:09 ч., Jeff Johnson wrote:
>>>> As discussed in [1] add a prototype for __fortify_panic() to fix the
>>>> 'make W=1 C=1' warning:
>>>>
>>>> arch/x86/boot/compressed/misc.c:535:6: warning: symbol '__fortify_panic' was not declared. Should it be static?
>>>
>>> Actually doesn't it make sense to have this defined under ../string.h ?
>>> Actually given that we don't have any string fortification under the
>>> boot/ why have the fortify _* functions at all ?
>>
>> I'll let Kees answer these questions since I just took guidance from him :)
>
> Ah-ha, I see what's happening. When not built with
> CONFIG_FORTIFY_SOURCE, fortify-string.h isn't included. But since misc.c
> has the function definition, we get a warning that the function
> declaration was never seen. This is likely the better solution:
>
>
> diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
> index b70e4a21c15f..3f21a5e218f8 100644
> --- a/arch/x86/boot/compressed/misc.c
> +++ b/arch/x86/boot/compressed/misc.c
> @@ -532,7 +532,9 @@ asmlinkage __visible void *extract_kernel(void *rmode, unsigned char *output)
> return output + entry_offset;
> }
>
> +#ifdef CONFIG_FORTIFY_SOURCE
> void __fortify_panic(const u8 reason, size_t avail, size_t size)
> {
> error("detected buffer overflow");
> }
> +#endif
>
>
> Jeff, can you test this? (I still haven't been able to reproduce the
> warning.)
Adding Dan since this comes during:
CHECK arch/x86/boot/compressed/misc.c
What version of smatch are you using? I'm using v0.5.0-8639-gff1cc4d453ff
In the build where I'm seeing this issue I have:
CONFIG_ARCH_HAS_FORTIFY_SOURCE=y
CONFIG_FORTIFY_SOURCE=y
CONFIG_FORTIFY_KUNIT_TEST=m
So that conditional compilation won't make a difference.
Also note that misc.c doesn't include the standard include/linux/string.h but
instead includes the stripped down arch/x86/boot/string.h, so fortify-string.h
isn't included.
This seems to come back around to the question that Nikolay asked, which part
of the boot code actually needs this?
/jeff
On Fri, May 31, 2024 at 11:28:58AM -0700, Jeff Johnson wrote:
> On 5/31/2024 9:28 AM, Kees Cook wrote:
> > On Thu, May 30, 2024 at 09:23:36AM -0700, Jeff Johnson wrote:
> >> On 5/30/2024 8:42 AM, Nikolay Borisov wrote:
> >>>
> >>>
> >>> On 29.05.24 \u0433. 21:09 \u0447., Jeff Johnson wrote:
> >>>> As discussed in [1] add a prototype for __fortify_panic() to fix the
> >>>> 'make W=1 C=1' warning:
> >>>>
> >>>> arch/x86/boot/compressed/misc.c:535:6: warning: symbol '__fortify_panic' was not declared. Should it be static?
> >>>
> >>> Actually doesn't it make sense to have this defined under ../string.h ?
> >>> Actually given that we don't have any string fortification under the
> >>> boot/ why have the fortify _* functions at all ?
> >>
> >> I'll let Kees answer these questions since I just took guidance from him :)
> >
> > Ah-ha, I see what's happening. When not built with
> > CONFIG_FORTIFY_SOURCE, fortify-string.h isn't included. But since misc.c
> > has the function definition, we get a warning that the function
> > declaration was never seen. This is likely the better solution:
> >
> >
> > diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
> > index b70e4a21c15f..3f21a5e218f8 100644
> > --- a/arch/x86/boot/compressed/misc.c
> > +++ b/arch/x86/boot/compressed/misc.c
> > @@ -532,7 +532,9 @@ asmlinkage __visible void *extract_kernel(void *rmode, unsigned char *output)
> > return output + entry_offset;
> > }
> >
> > +#ifdef CONFIG_FORTIFY_SOURCE
> > void __fortify_panic(const u8 reason, size_t avail, size_t size)
> > {
> > error("detected buffer overflow");
> > }
> > +#endif
> >
> >
> > Jeff, can you test this? (I still haven't been able to reproduce the
> > warning.)
>
> Adding Dan since this comes during:
> CHECK arch/x86/boot/compressed/misc.c
>
> What version of smatch are you using? I'm using v0.5.0-8639-gff1cc4d453ff
The "warning: symbol '__fortify_panic' was not declared. Should it be
static?" warning is from Sparse, not Smatch. So probably that's why you
can't reproduce it.
regards,
dan carpenter
On Fri, May 31, 2024 at 09:53:28AM -0700, Kees Cook wrote:
> Under CONFIG_FORTIFY_SOURCE, the boot code *does* still uses
> fortify-string.h. It lets us both catch mistakes we can discover at
> compile and will catch egregious runtime mistakes, though the reporting
> is much simpler in the boot code.
From where I'm standing, we're not catching anything in the
decompressor:
$ objdump -D arch/x86/boot/compressed/vmlinux | grep __fortify_panic
0000000001bec250 <__fortify_panic>:
$
Sure, in vmlinux proper (allmodconfig) we do:
objdump -D vmlinux | grep __fortify_panic | wc -l
1417
but not in the decompressor which is special anyway.
So we can just as well disable CONFIG_FORTIFY_SOURCE in the decompressor
and not do silly prototypes.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Fri, May 31, 2024 at 09:08:16PM +0200, Borislav Petkov wrote:
> On Fri, May 31, 2024 at 09:53:28AM -0700, Kees Cook wrote:
> > Under CONFIG_FORTIFY_SOURCE, the boot code *does* still uses
> > fortify-string.h. It lets us both catch mistakes we can discover at
> > compile and will catch egregious runtime mistakes, though the reporting
> > is much simpler in the boot code.
>
> From where I'm standing, we're not catching anything in the
> decompressor:
>
> $ objdump -D arch/x86/boot/compressed/vmlinux | grep __fortify_panic
> 0000000001bec250 <__fortify_panic>:
> $
>
> Sure, in vmlinux proper (allmodconfig) we do:
>
> objdump -D vmlinux | grep __fortify_panic | wc -l
> 1417
>
> but not in the decompressor which is special anyway.
>
> So we can just as well disable CONFIG_FORTIFY_SOURCE in the decompressor
> and not do silly prototypes.
Please do not do this. It still benefits from compile-time sanity
checking.
--
Kees Cook
On Fri, May 31, 2024 at 10:49:47PM +0200, Borislav Petkov wrote:
> On Fri, May 31, 2024 at 01:46:37PM -0700, Kees Cook wrote:
> > Please do not do this. It still benefits from compile-time sanity
> > checking.
>
> Care to elaborate how exactly it benefits?
Because when new code gets added that accidentally does improper string
handling, fortify will yell about it at compile time. e.g, if someone
typos something like:
#define BUF_LEN_FOO 16
...
#define BUF_LEN_BAR 10
struct foo {
...
char buf[BUF_LEN_FOO];
...
};
...
void process_stuff(struct foo *p)
{
...
char local_copy[BUF_LEN_BAR];
...
strcpy(local_copy, p->buf);
...
}
or refactors and forgets to change some name, etc. It's all for catching
bugs before they happen, etc. And when source string lengths aren't
known, the runtime checking can kick in too. It happens x86 boot doesn't
have any of those (good!) so __fortify_panic() goes unused there. But
that's a larger topic covered by stuff like
CONFIG_LD_DEAD_CODE_DATA_ELIMINATION, etc.
--
Kees Cook
On Fri, May 31, 2024 at 11:20:09PM +0200, Borislav Petkov wrote:
> So I get an allergic reaction everytime we wag the dog - i.e., fix the
> code because some tool or option can't handle it even if it is
> a perfectly fine code. In that case it is an unused symbol.
>
> And frankly, I'd prefer the silly warning to denote that fortify doesn't
> need to do any checking there vs shutting it up just because.
If we want to declare that x86 boot will never perform string handling
on strings with unknown lengths, we could just delete the boot/
implementation of __fortify_panic(), and make it a hard failure if such
cases are introduced in the future. This hasn't been a particularly
friendly solution in the past, though, as the fortify routines do tend
to grow additional coverage over time, so there may be future cases that
do trip the runtime checking...
--
Kees Cook
On 5/31/2024 2:45 PM, Borislav Petkov wrote:
> On Fri, May 31, 2024 at 02:34:07PM -0700, Kees Cook wrote:
>> On Fri, May 31, 2024 at 11:20:09PM +0200, Borislav Petkov wrote:
>>> So I get an allergic reaction everytime we wag the dog - i.e., fix the
>>> code because some tool or option can't handle it even if it is
>>> a perfectly fine code. In that case it is an unused symbol.
>>>
>>> And frankly, I'd prefer the silly warning to denote that fortify doesn't
>>> need to do any checking there vs shutting it up just because.
>>
>> If we want to declare that x86 boot will never perform string handling
>> on strings with unknown lengths, we could just delete the boot/
>> implementation of __fortify_panic(), and make it a hard failure if such
>> cases are introduced in the future. This hasn't been a particularly
>> friendly solution in the past, though, as the fortify routines do tend
>> to grow additional coverage over time, so there may be future cases that
>> do trip the runtime checking...
>
> Yes, and we should not do anything right now either.
>
> As said, I'd prefer the warning which actually says that fortify
> routines are not used, which in itself is useful information vs shutting
> it up.
>
I'm ok with whatever you want to do. I was just following the example from ARM
where they have a prototype in arch/arm/boot/compressed/misc.h to match the
implementation in arch/arm/boot/compressed/misc.c
/jeff
On 31.05.24 г. 19:28 ч., Kees Cook wrote:
> On Thu, May 30, 2024 at 09:23:36AM -0700, Jeff Johnson wrote:
>> On 5/30/2024 8:42 AM, Nikolay Borisov wrote:
>>>
>>>
>>> On 29.05.24 г. 21:09 ч., Jeff Johnson wrote:
>>>> As discussed in [1] add a prototype for __fortify_panic() to fix the
>>>> 'make W=1 C=1' warning:
>>>>
>>>> arch/x86/boot/compressed/misc.c:535:6: warning: symbol '__fortify_panic' was not declared. Should it be static?
>>>
>>> Actually doesn't it make sense to have this defined under ../string.h ?
>>> Actually given that we don't have any string fortification under the
>>> boot/ why have the fortify _* functions at all ?
>>
>> I'll let Kees answer these questions since I just took guidance from him :)
>
> Ah-ha, I see what's happening. When not built with
> CONFIG_FORTIFY_SOURCE, fortify-string.h isn't included. But since misc.c
> has the function definition, we get a warning that the function
> declaration was never seen. This is likely the better solution:
fortify-strings.h is used in include/linux/string.h. However all the
files in the decompressor are using a local copy of string.h and not the
kernel-wide. When pre-processing misc.c with FORTIFY_SOURCE enabled
here's the status:
$ grep -i fortify arch/x86/boot/compressed/misc.i
void __fortify_panic(const u8 reason, size_t avail, size_t size)
It seems the decompressor is not using fortify-string at all because
it's not using the global string.h ?
>
>
> diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
> index b70e4a21c15f..3f21a5e218f8 100644
> --- a/arch/x86/boot/compressed/misc.c
> +++ b/arch/x86/boot/compressed/misc.c
> @@ -532,7 +532,9 @@ asmlinkage __visible void *extract_kernel(void *rmode, unsigned char *output)
> return output + entry_offset;
> }
>
> +#ifdef CONFIG_FORTIFY_SOURCE
> void __fortify_panic(const u8 reason, size_t avail, size_t size)
> {
> error("detected buffer overflow");
> }
> +#endif
>
>
> Jeff, can you test this? (I still haven't been able to reproduce the
> warning.)
>
On Fri, May 31, 2024 at 02:06:48PM -0700, Kees Cook wrote:
> ...
> or refactors and forgets to change some name, etc. It's all for catching
> bugs before they happen, etc. And when source string lengths aren't
> known, the runtime checking can kick in too.
Aha, thanks for explaining.
> It happens x86 boot doesn't have any of those (good!) so
> __fortify_panic() goes unused there. But
Exactly!
> that's a larger topic covered by stuff like
> CONFIG_LD_DEAD_CODE_DATA_ELIMINATION, etc.
"... This option is not well tested yet, so use at your own risk."
Oh well.
So I get an allergic reaction everytime we wag the dog - i.e., fix the
code because some tool or option can't handle it even if it is
a perfectly fine code. In that case it is an unused symbol.
And frankly, I'd prefer the silly warning to denote that fortify doesn't
need to do any checking there vs shutting it up just because.
So can we aim our efforts at real bugs please?
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Fri, May 31, 2024 at 01:46:37PM -0700, Kees Cook wrote:
> Please do not do this. It still benefits from compile-time sanity
> checking.
Care to elaborate how exactly it benefits?
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Fri, May 31, 2024 at 02:34:07PM -0700, Kees Cook wrote:
> On Fri, May 31, 2024 at 11:20:09PM +0200, Borislav Petkov wrote:
> > So I get an allergic reaction everytime we wag the dog - i.e., fix the
> > code because some tool or option can't handle it even if it is
> > a perfectly fine code. In that case it is an unused symbol.
> >
> > And frankly, I'd prefer the silly warning to denote that fortify doesn't
> > need to do any checking there vs shutting it up just because.
>
> If we want to declare that x86 boot will never perform string handling
> on strings with unknown lengths, we could just delete the boot/
> implementation of __fortify_panic(), and make it a hard failure if such
> cases are introduced in the future. This hasn't been a particularly
> friendly solution in the past, though, as the fortify routines do tend
> to grow additional coverage over time, so there may be future cases that
> do trip the runtime checking...
Yes, and we should not do anything right now either.
As said, I'd prefer the warning which actually says that fortify
routines are not used, which in itself is useful information vs shutting
it up.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 1.06.24 г. 10:27 ч., Nikolay Borisov wrote:
>
>
> On 31.05.24 г. 19:28 ч., Kees Cook wrote:
>> On Thu, May 30, 2024 at 09:23:36AM -0700, Jeff Johnson wrote:
>>> On 5/30/2024 8:42 AM, Nikolay Borisov wrote:
>>>>
>>>>
>>>> On 29.05.24 г. 21:09 ч., Jeff Johnson wrote:
>>>>> As discussed in [1] add a prototype for __fortify_panic() to fix the
>>>>> 'make W=1 C=1' warning:
>>>>>
>>>>> arch/x86/boot/compressed/misc.c:535:6: warning: symbol
>>>>> '__fortify_panic' was not declared. Should it be static?
>>>>
>>>> Actually doesn't it make sense to have this defined under ../string.h ?
>>>> Actually given that we don't have any string fortification under the
>>>> boot/ why have the fortify _* functions at all ?
>>>
>>> I'll let Kees answer these questions since I just took guidance from
>>> him :)
>>
>> Ah-ha, I see what's happening. When not built with
>> CONFIG_FORTIFY_SOURCE, fortify-string.h isn't included. But since misc.c
>> has the function definition, we get a warning that the function
>> declaration was never seen. This is likely the better solution:
>
> fortify-strings.h is used in include/linux/string.h. However all the
> files in the decompressor are using a local copy of string.h and not the
> kernel-wide. When pre-processing misc.c with FORTIFY_SOURCE enabled
> here's the status:
>
> $ grep -i fortify arch/x86/boot/compressed/misc.i
> void __fortify_panic(const u8 reason, size_t avail, size_t size)
>
> It seems the decompressor is not using fortify-string at all because
> it's not using the global string.h ?
Kees, care to comment about my observation? Have I missed anything?
Reading the following articles :
https://www.redhat.com/en/blog/enhance-application-security-fortifysource
it seems that fortification comes from using the system header string.h
(in our case that'd be include/linux/string.h) which is not being used
by the decompressor at all so simply removing the function definition
should be the correct fix, no ?
<snip>