| Message ID | 20240529-fortify_panic-v1-1-9923d5c77657@quicinc.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | x86/boot: add prototype for __fortify_panic() | expand |
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/79653cc7-6e59-4657-9c0a-76f49f49d019@quicinc.com/ [1] > Signed-off-by: Jeff Johnson <quic_jjohnson@quicinc.com> > --- > 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 <quic_jjohnson@quicinc.com> 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...
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.)
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 <quic_jjohnson@quicinc.com> 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.
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.
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.
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?
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.
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?
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...
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.
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 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>
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
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/79653cc7-6e59-4657-9c0a-76f49f49d019@quicinc.com/ [1] Signed-off-by: Jeff Johnson <quic_jjohnson@quicinc.com> --- arch/x86/boot/compressed/misc.h | 1 + 1 file changed, 1 insertion(+) --- base-commit: e0cce98fe279b64f4a7d81b7f5c3a23d80b92fbc change-id: 20240529-fortify_panic-325601efe71d