Re: [PATCH] locking/refcounts, x86/asm: Use unique .text section for refcount exceptions
On Sat, Sep 2, 2017 at 3:29 AM, Ingo Molnarwrote: > > * Kees Cook wrote: > >> Using .text.unlikely for refcount exceptions isn't safe because gcc may >> move entire functions into .text.unlikely (e.g. in6_dev_get()), which >> would cause any uses of a protected refcount_t function to stay inline >> with the function, triggering the protection unconditionally: >> >> .section.text.unlikely,"ax",@progbits >> .type in6_dev_get, @function >> in6_dev_getx: >> .LFB4673: >> .loc 2 4128 0 >> .cfi_startproc >> ... >> lock; incl 480(%rbx) >> js 111f >> .pushsection .text.unlikely >> 111:lea 480(%rbx), %rcx >> 112:.byte 0x0f, 0xff >> .popsection >> 113: >> >> This creates a unique .text section and adds an additional test to the >> exception handler to WARN in the case of having none of OF, SF, nor ZF >> set so we can see things like this more easily in the future. >> >> Reported-by: Mike Galbraith >> Fixes: 7a46ec0e2f48 ("locking/refcounts, x86/asm: Implement fast refcount >> overflow protection") >> Signed-off-by: Kees Cook >> --- >> arch/x86/Kconfig | 2 +- >> arch/x86/include/asm/refcount.h | 2 +- >> arch/x86/mm/extable.c | 7 ++- >> include/asm-generic/vmlinux.lds.h | 1 + >> 4 files changed, 9 insertions(+), 3 deletions(-) > > Could you please split this into two patches: one that fixes the .unlikely > bug, > the other that re-enables the optimized version? > > Should there be any other problem with refcounts this would make any bisection > result more clear-cut. Certainly! -Kees -- Kees Cook Pixel Security
Re: [PATCH] locking/refcounts, x86/asm: Use unique .text section for refcount exceptions
On Sat, Sep 2, 2017 at 3:29 AM, Ingo Molnar wrote: > > * Kees Cook wrote: > >> Using .text.unlikely for refcount exceptions isn't safe because gcc may >> move entire functions into .text.unlikely (e.g. in6_dev_get()), which >> would cause any uses of a protected refcount_t function to stay inline >> with the function, triggering the protection unconditionally: >> >> .section.text.unlikely,"ax",@progbits >> .type in6_dev_get, @function >> in6_dev_getx: >> .LFB4673: >> .loc 2 4128 0 >> .cfi_startproc >> ... >> lock; incl 480(%rbx) >> js 111f >> .pushsection .text.unlikely >> 111:lea 480(%rbx), %rcx >> 112:.byte 0x0f, 0xff >> .popsection >> 113: >> >> This creates a unique .text section and adds an additional test to the >> exception handler to WARN in the case of having none of OF, SF, nor ZF >> set so we can see things like this more easily in the future. >> >> Reported-by: Mike Galbraith >> Fixes: 7a46ec0e2f48 ("locking/refcounts, x86/asm: Implement fast refcount >> overflow protection") >> Signed-off-by: Kees Cook >> --- >> arch/x86/Kconfig | 2 +- >> arch/x86/include/asm/refcount.h | 2 +- >> arch/x86/mm/extable.c | 7 ++- >> include/asm-generic/vmlinux.lds.h | 1 + >> 4 files changed, 9 insertions(+), 3 deletions(-) > > Could you please split this into two patches: one that fixes the .unlikely > bug, > the other that re-enables the optimized version? > > Should there be any other problem with refcounts this would make any bisection > result more clear-cut. Certainly! -Kees -- Kees Cook Pixel Security
Re: [PATCH] locking/refcounts, x86/asm: Use unique .text section for refcount exceptions
* Kees Cookwrote: > Using .text.unlikely for refcount exceptions isn't safe because gcc may > move entire functions into .text.unlikely (e.g. in6_dev_get()), which > would cause any uses of a protected refcount_t function to stay inline > with the function, triggering the protection unconditionally: > > .section.text.unlikely,"ax",@progbits > .type in6_dev_get, @function > in6_dev_getx: > .LFB4673: > .loc 2 4128 0 > .cfi_startproc > ... > lock; incl 480(%rbx) > js 111f > .pushsection .text.unlikely > 111:lea 480(%rbx), %rcx > 112:.byte 0x0f, 0xff > .popsection > 113: > > This creates a unique .text section and adds an additional test to the > exception handler to WARN in the case of having none of OF, SF, nor ZF > set so we can see things like this more easily in the future. > > Reported-by: Mike Galbraith > Fixes: 7a46ec0e2f48 ("locking/refcounts, x86/asm: Implement fast refcount > overflow protection") > Signed-off-by: Kees Cook > --- > arch/x86/Kconfig | 2 +- > arch/x86/include/asm/refcount.h | 2 +- > arch/x86/mm/extable.c | 7 ++- > include/asm-generic/vmlinux.lds.h | 1 + > 4 files changed, 9 insertions(+), 3 deletions(-) Could you please split this into two patches: one that fixes the .unlikely bug, the other that re-enables the optimized version? Should there be any other problem with refcounts this would make any bisection result more clear-cut. Thanks, Ingo
Re: [PATCH] locking/refcounts, x86/asm: Use unique .text section for refcount exceptions
* Kees Cook wrote: > Using .text.unlikely for refcount exceptions isn't safe because gcc may > move entire functions into .text.unlikely (e.g. in6_dev_get()), which > would cause any uses of a protected refcount_t function to stay inline > with the function, triggering the protection unconditionally: > > .section.text.unlikely,"ax",@progbits > .type in6_dev_get, @function > in6_dev_getx: > .LFB4673: > .loc 2 4128 0 > .cfi_startproc > ... > lock; incl 480(%rbx) > js 111f > .pushsection .text.unlikely > 111:lea 480(%rbx), %rcx > 112:.byte 0x0f, 0xff > .popsection > 113: > > This creates a unique .text section and adds an additional test to the > exception handler to WARN in the case of having none of OF, SF, nor ZF > set so we can see things like this more easily in the future. > > Reported-by: Mike Galbraith > Fixes: 7a46ec0e2f48 ("locking/refcounts, x86/asm: Implement fast refcount > overflow protection") > Signed-off-by: Kees Cook > --- > arch/x86/Kconfig | 2 +- > arch/x86/include/asm/refcount.h | 2 +- > arch/x86/mm/extable.c | 7 ++- > include/asm-generic/vmlinux.lds.h | 1 + > 4 files changed, 9 insertions(+), 3 deletions(-) Could you please split this into two patches: one that fixes the .unlikely bug, the other that re-enables the optimized version? Should there be any other problem with refcounts this would make any bisection result more clear-cut. Thanks, Ingo
Re: [PATCH] locking/refcounts, x86/asm: Use unique .text section for refcount exceptions
On 2 September 2017 at 03:58, Kees Cookwrote: > On Fri, Sep 1, 2017 at 2:43 PM, Ard Biesheuvel > wrote: >> On 1 September 2017 at 21:22, Kees Cook wrote: >>> Using .text.unlikely for refcount exceptions isn't safe because gcc may >>> move entire functions into .text.unlikely (e.g. in6_dev_get()), which >>> would cause any uses of a protected refcount_t function to stay inline >>> with the function, triggering the protection unconditionally: >>> >>> .section.text.unlikely,"ax",@progbits >>> .type in6_dev_get, @function >>> in6_dev_getx: >>> .LFB4673: >>> .loc 2 4128 0 >>> .cfi_startproc >>> ... >>> lock; incl 480(%rbx) >>> js 111f >>> .pushsection .text.unlikely >>> 111:lea 480(%rbx), %rcx >>> 112:.byte 0x0f, 0xff >>> .popsection >>> 113: >>> >>> This creates a unique .text section and adds an additional test to the >>> exception handler to WARN in the case of having none of OF, SF, nor ZF >>> set so we can see things like this more easily in the future. >>> >>> Reported-by: Mike Galbraith >>> Fixes: 7a46ec0e2f48 ("locking/refcounts, x86/asm: Implement fast refcount >>> overflow protection") >>> Signed-off-by: Kees Cook >>> --- >>> arch/x86/Kconfig | 2 +- >>> arch/x86/include/asm/refcount.h | 2 +- >>> arch/x86/mm/extable.c | 7 ++- >>> include/asm-generic/vmlinux.lds.h | 1 + >>> 4 files changed, 9 insertions(+), 3 deletions(-) >>> >>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig >>> index eaa8ff41f424..c6acdcdb3fc6 100644 >>> --- a/arch/x86/Kconfig >>> +++ b/arch/x86/Kconfig >>> @@ -56,7 +56,7 @@ config X86 >>> select ARCH_HAS_MMIO_FLUSH >>> select ARCH_HAS_PMEM_APIif X86_64 >>> # Causing hangs/crashes, see the commit that added this change for >>> details. >>> - select ARCH_HAS_REFCOUNTif BROKEN >>> + select ARCH_HAS_REFCOUNT >>> select ARCH_HAS_UACCESS_FLUSHCACHE if X86_64 >>> select ARCH_HAS_SET_MEMORY >>> select ARCH_HAS_SG_CHAIN >>> diff --git a/arch/x86/include/asm/refcount.h >>> b/arch/x86/include/asm/refcount.h >>> index ff871210b9f2..4e44250e7d0d 100644 >>> --- a/arch/x86/include/asm/refcount.h >>> +++ b/arch/x86/include/asm/refcount.h >>> @@ -15,7 +15,7 @@ >>> * back to the regular execution flow in .text. >>> */ >>> #define _REFCOUNT_EXCEPTION\ >>> - ".pushsection .text.unlikely\n" \ >>> + ".pushsection .text..refcount\n"\ >> >> You could also use a .subsection here: those are always emitted out of >> line, but into the same section. > > I considered it (especially after looking to see how you handled it in > the arm64 port) but decided against it in favor of collecting them all > at the end with .text.unlikely. > OK. On arm64, I needed this because the out of line sequence refers back to the inline code, which resulted in section mismatches if the refcount API is used from __init code. If that problem does not exist on x86, I guess it doesn't matter.
Re: [PATCH] locking/refcounts, x86/asm: Use unique .text section for refcount exceptions
On 2 September 2017 at 03:58, Kees Cook wrote: > On Fri, Sep 1, 2017 at 2:43 PM, Ard Biesheuvel > wrote: >> On 1 September 2017 at 21:22, Kees Cook wrote: >>> Using .text.unlikely for refcount exceptions isn't safe because gcc may >>> move entire functions into .text.unlikely (e.g. in6_dev_get()), which >>> would cause any uses of a protected refcount_t function to stay inline >>> with the function, triggering the protection unconditionally: >>> >>> .section.text.unlikely,"ax",@progbits >>> .type in6_dev_get, @function >>> in6_dev_getx: >>> .LFB4673: >>> .loc 2 4128 0 >>> .cfi_startproc >>> ... >>> lock; incl 480(%rbx) >>> js 111f >>> .pushsection .text.unlikely >>> 111:lea 480(%rbx), %rcx >>> 112:.byte 0x0f, 0xff >>> .popsection >>> 113: >>> >>> This creates a unique .text section and adds an additional test to the >>> exception handler to WARN in the case of having none of OF, SF, nor ZF >>> set so we can see things like this more easily in the future. >>> >>> Reported-by: Mike Galbraith >>> Fixes: 7a46ec0e2f48 ("locking/refcounts, x86/asm: Implement fast refcount >>> overflow protection") >>> Signed-off-by: Kees Cook >>> --- >>> arch/x86/Kconfig | 2 +- >>> arch/x86/include/asm/refcount.h | 2 +- >>> arch/x86/mm/extable.c | 7 ++- >>> include/asm-generic/vmlinux.lds.h | 1 + >>> 4 files changed, 9 insertions(+), 3 deletions(-) >>> >>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig >>> index eaa8ff41f424..c6acdcdb3fc6 100644 >>> --- a/arch/x86/Kconfig >>> +++ b/arch/x86/Kconfig >>> @@ -56,7 +56,7 @@ config X86 >>> select ARCH_HAS_MMIO_FLUSH >>> select ARCH_HAS_PMEM_APIif X86_64 >>> # Causing hangs/crashes, see the commit that added this change for >>> details. >>> - select ARCH_HAS_REFCOUNTif BROKEN >>> + select ARCH_HAS_REFCOUNT >>> select ARCH_HAS_UACCESS_FLUSHCACHE if X86_64 >>> select ARCH_HAS_SET_MEMORY >>> select ARCH_HAS_SG_CHAIN >>> diff --git a/arch/x86/include/asm/refcount.h >>> b/arch/x86/include/asm/refcount.h >>> index ff871210b9f2..4e44250e7d0d 100644 >>> --- a/arch/x86/include/asm/refcount.h >>> +++ b/arch/x86/include/asm/refcount.h >>> @@ -15,7 +15,7 @@ >>> * back to the regular execution flow in .text. >>> */ >>> #define _REFCOUNT_EXCEPTION\ >>> - ".pushsection .text.unlikely\n" \ >>> + ".pushsection .text..refcount\n"\ >> >> You could also use a .subsection here: those are always emitted out of >> line, but into the same section. > > I considered it (especially after looking to see how you handled it in > the arm64 port) but decided against it in favor of collecting them all > at the end with .text.unlikely. > OK. On arm64, I needed this because the out of line sequence refers back to the inline code, which resulted in section mismatches if the refcount API is used from __init code. If that problem does not exist on x86, I guess it doesn't matter.
Re: [PATCH] locking/refcounts, x86/asm: Use unique .text section for refcount exceptions
On Fri, Sep 1, 2017 at 9:03 PM, Mike Galbraithwrote: > On Fri, 2017-09-01 at 13:22 -0700, Kees Cook wrote: >> Using .text.unlikely for refcount exceptions isn't safe because gcc may >> move entire functions into .text.unlikely (e.g. in6_dev_get()), which >> would cause any uses of a protected refcount_t function to stay inline >> with the function, triggering the protection unconditionally: >> >> .section.text.unlikely,"ax",@progbits >> .type in6_dev_get, @function >> in6_dev_getx: >> .LFB4673: >> .loc 2 4128 0 >> .cfi_startproc >> ... >> lock; incl 480(%rbx) >> js 111f >> .pushsection .text.unlikely >> 111:lea 480(%rbx), %rcx >> 112:.byte 0x0f, 0xff >> .popsection >> 113: >> >> This creates a unique .text section and adds an additional test to the >> exception handler to WARN in the case of having none of OF, SF, nor ZF >> set so we can see things like this more easily in the future. > > Closure: gcc-4.8.5 now builds a functional kernel as well, so that > aspect of this bug was just a larger a dose of the same toxin. Okay, excellent. Thanks for checking! > > Question below. > > diff --git a/arch/x86/include/asm/refcount.h > b/arch/x86/include/asm/refcount.h >> index ff871210b9f2..4e44250e7d0d 100644 >> --- a/arch/x86/include/asm/refcount.h >> +++ b/arch/x86/include/asm/refcount.h >> @@ -15,7 +15,7 @@ >> * back to the regular execution flow in .text. >> */ >> #define _REFCOUNT_EXCEPTION \ >> - ".pushsection .text.unlikely\n" \ >> + ".pushsection .text..refcount\n"\ > > Why two dots? (.text.refcount_ex?) A dot keeps it out of the TEXT_MAIN macro namespace (see cb87481ee89db in -next, which is function names: [a-zA-Z0-9_]) to avoid collisions and so it can be put at the end with text.unlikely to keep the cold code together. -Kees -- Kees Cook Pixel Security
Re: [PATCH] locking/refcounts, x86/asm: Use unique .text section for refcount exceptions
On Fri, Sep 1, 2017 at 9:03 PM, Mike Galbraith wrote: > On Fri, 2017-09-01 at 13:22 -0700, Kees Cook wrote: >> Using .text.unlikely for refcount exceptions isn't safe because gcc may >> move entire functions into .text.unlikely (e.g. in6_dev_get()), which >> would cause any uses of a protected refcount_t function to stay inline >> with the function, triggering the protection unconditionally: >> >> .section.text.unlikely,"ax",@progbits >> .type in6_dev_get, @function >> in6_dev_getx: >> .LFB4673: >> .loc 2 4128 0 >> .cfi_startproc >> ... >> lock; incl 480(%rbx) >> js 111f >> .pushsection .text.unlikely >> 111:lea 480(%rbx), %rcx >> 112:.byte 0x0f, 0xff >> .popsection >> 113: >> >> This creates a unique .text section and adds an additional test to the >> exception handler to WARN in the case of having none of OF, SF, nor ZF >> set so we can see things like this more easily in the future. > > Closure: gcc-4.8.5 now builds a functional kernel as well, so that > aspect of this bug was just a larger a dose of the same toxin. Okay, excellent. Thanks for checking! > > Question below. > > diff --git a/arch/x86/include/asm/refcount.h > b/arch/x86/include/asm/refcount.h >> index ff871210b9f2..4e44250e7d0d 100644 >> --- a/arch/x86/include/asm/refcount.h >> +++ b/arch/x86/include/asm/refcount.h >> @@ -15,7 +15,7 @@ >> * back to the regular execution flow in .text. >> */ >> #define _REFCOUNT_EXCEPTION \ >> - ".pushsection .text.unlikely\n" \ >> + ".pushsection .text..refcount\n"\ > > Why two dots? (.text.refcount_ex?) A dot keeps it out of the TEXT_MAIN macro namespace (see cb87481ee89db in -next, which is function names: [a-zA-Z0-9_]) to avoid collisions and so it can be put at the end with text.unlikely to keep the cold code together. -Kees -- Kees Cook Pixel Security
Re: [PATCH] locking/refcounts, x86/asm: Use unique .text section for refcount exceptions
On Fri, 2017-09-01 at 13:22 -0700, Kees Cook wrote: > Using .text.unlikely for refcount exceptions isn't safe because gcc may > move entire functions into .text.unlikely (e.g. in6_dev_get()), which > would cause any uses of a protected refcount_t function to stay inline > with the function, triggering the protection unconditionally: > > .section.text.unlikely,"ax",@progbits > .type in6_dev_get, @function > in6_dev_getx: > .LFB4673: > .loc 2 4128 0 > .cfi_startproc > ... > lock; incl 480(%rbx) > js 111f > .pushsection .text.unlikely > 111:lea 480(%rbx), %rcx > 112:.byte 0x0f, 0xff > .popsection > 113: > > This creates a unique .text section and adds an additional test to the > exception handler to WARN in the case of having none of OF, SF, nor ZF > set so we can see things like this more easily in the future. Closure: gcc-4.8.5 now builds a functional kernel as well, so that aspect of this bug was just a larger a dose of the same toxin. Question below. diff --git a/arch/x86/include/asm/refcount.h b/arch/x86/include/asm/refcount.h > index ff871210b9f2..4e44250e7d0d 100644 > --- a/arch/x86/include/asm/refcount.h > +++ b/arch/x86/include/asm/refcount.h > @@ -15,7 +15,7 @@ > * back to the regular execution flow in .text. > */ > #define _REFCOUNT_EXCEPTION \ > - ".pushsection .text.unlikely\n" \ > + ".pushsection .text..refcount\n"\ Why two dots? (.text.refcount_ex?) -Mike
Re: [PATCH] locking/refcounts, x86/asm: Use unique .text section for refcount exceptions
On Fri, 2017-09-01 at 13:22 -0700, Kees Cook wrote: > Using .text.unlikely for refcount exceptions isn't safe because gcc may > move entire functions into .text.unlikely (e.g. in6_dev_get()), which > would cause any uses of a protected refcount_t function to stay inline > with the function, triggering the protection unconditionally: > > .section.text.unlikely,"ax",@progbits > .type in6_dev_get, @function > in6_dev_getx: > .LFB4673: > .loc 2 4128 0 > .cfi_startproc > ... > lock; incl 480(%rbx) > js 111f > .pushsection .text.unlikely > 111:lea 480(%rbx), %rcx > 112:.byte 0x0f, 0xff > .popsection > 113: > > This creates a unique .text section and adds an additional test to the > exception handler to WARN in the case of having none of OF, SF, nor ZF > set so we can see things like this more easily in the future. Closure: gcc-4.8.5 now builds a functional kernel as well, so that aspect of this bug was just a larger a dose of the same toxin. Question below. diff --git a/arch/x86/include/asm/refcount.h b/arch/x86/include/asm/refcount.h > index ff871210b9f2..4e44250e7d0d 100644 > --- a/arch/x86/include/asm/refcount.h > +++ b/arch/x86/include/asm/refcount.h > @@ -15,7 +15,7 @@ > * back to the regular execution flow in .text. > */ > #define _REFCOUNT_EXCEPTION \ > - ".pushsection .text.unlikely\n" \ > + ".pushsection .text..refcount\n"\ Why two dots? (.text.refcount_ex?) -Mike
Re: [PATCH] locking/refcounts, x86/asm: Use unique .text section for refcount exceptions
On Fri, Sep 1, 2017 at 2:43 PM, Ard Biesheuvelwrote: > On 1 September 2017 at 21:22, Kees Cook wrote: >> Using .text.unlikely for refcount exceptions isn't safe because gcc may >> move entire functions into .text.unlikely (e.g. in6_dev_get()), which >> would cause any uses of a protected refcount_t function to stay inline >> with the function, triggering the protection unconditionally: >> >> .section.text.unlikely,"ax",@progbits >> .type in6_dev_get, @function >> in6_dev_getx: >> .LFB4673: >> .loc 2 4128 0 >> .cfi_startproc >> ... >> lock; incl 480(%rbx) >> js 111f >> .pushsection .text.unlikely >> 111:lea 480(%rbx), %rcx >> 112:.byte 0x0f, 0xff >> .popsection >> 113: >> >> This creates a unique .text section and adds an additional test to the >> exception handler to WARN in the case of having none of OF, SF, nor ZF >> set so we can see things like this more easily in the future. >> >> Reported-by: Mike Galbraith >> Fixes: 7a46ec0e2f48 ("locking/refcounts, x86/asm: Implement fast refcount >> overflow protection") >> Signed-off-by: Kees Cook >> --- >> arch/x86/Kconfig | 2 +- >> arch/x86/include/asm/refcount.h | 2 +- >> arch/x86/mm/extable.c | 7 ++- >> include/asm-generic/vmlinux.lds.h | 1 + >> 4 files changed, 9 insertions(+), 3 deletions(-) >> >> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig >> index eaa8ff41f424..c6acdcdb3fc6 100644 >> --- a/arch/x86/Kconfig >> +++ b/arch/x86/Kconfig >> @@ -56,7 +56,7 @@ config X86 >> select ARCH_HAS_MMIO_FLUSH >> select ARCH_HAS_PMEM_APIif X86_64 >> # Causing hangs/crashes, see the commit that added this change for >> details. >> - select ARCH_HAS_REFCOUNTif BROKEN >> + select ARCH_HAS_REFCOUNT >> select ARCH_HAS_UACCESS_FLUSHCACHE if X86_64 >> select ARCH_HAS_SET_MEMORY >> select ARCH_HAS_SG_CHAIN >> diff --git a/arch/x86/include/asm/refcount.h >> b/arch/x86/include/asm/refcount.h >> index ff871210b9f2..4e44250e7d0d 100644 >> --- a/arch/x86/include/asm/refcount.h >> +++ b/arch/x86/include/asm/refcount.h >> @@ -15,7 +15,7 @@ >> * back to the regular execution flow in .text. >> */ >> #define _REFCOUNT_EXCEPTION\ >> - ".pushsection .text.unlikely\n" \ >> + ".pushsection .text..refcount\n"\ > > You could also use a .subsection here: those are always emitted out of > line, but into the same section. I considered it (especially after looking to see how you handled it in the arm64 port) but decided against it in favor of collecting them all at the end with .text.unlikely. -Kees -- Kees Cook Pixel Security
Re: [PATCH] locking/refcounts, x86/asm: Use unique .text section for refcount exceptions
On Fri, Sep 1, 2017 at 2:43 PM, Ard Biesheuvel wrote: > On 1 September 2017 at 21:22, Kees Cook wrote: >> Using .text.unlikely for refcount exceptions isn't safe because gcc may >> move entire functions into .text.unlikely (e.g. in6_dev_get()), which >> would cause any uses of a protected refcount_t function to stay inline >> with the function, triggering the protection unconditionally: >> >> .section.text.unlikely,"ax",@progbits >> .type in6_dev_get, @function >> in6_dev_getx: >> .LFB4673: >> .loc 2 4128 0 >> .cfi_startproc >> ... >> lock; incl 480(%rbx) >> js 111f >> .pushsection .text.unlikely >> 111:lea 480(%rbx), %rcx >> 112:.byte 0x0f, 0xff >> .popsection >> 113: >> >> This creates a unique .text section and adds an additional test to the >> exception handler to WARN in the case of having none of OF, SF, nor ZF >> set so we can see things like this more easily in the future. >> >> Reported-by: Mike Galbraith >> Fixes: 7a46ec0e2f48 ("locking/refcounts, x86/asm: Implement fast refcount >> overflow protection") >> Signed-off-by: Kees Cook >> --- >> arch/x86/Kconfig | 2 +- >> arch/x86/include/asm/refcount.h | 2 +- >> arch/x86/mm/extable.c | 7 ++- >> include/asm-generic/vmlinux.lds.h | 1 + >> 4 files changed, 9 insertions(+), 3 deletions(-) >> >> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig >> index eaa8ff41f424..c6acdcdb3fc6 100644 >> --- a/arch/x86/Kconfig >> +++ b/arch/x86/Kconfig >> @@ -56,7 +56,7 @@ config X86 >> select ARCH_HAS_MMIO_FLUSH >> select ARCH_HAS_PMEM_APIif X86_64 >> # Causing hangs/crashes, see the commit that added this change for >> details. >> - select ARCH_HAS_REFCOUNTif BROKEN >> + select ARCH_HAS_REFCOUNT >> select ARCH_HAS_UACCESS_FLUSHCACHE if X86_64 >> select ARCH_HAS_SET_MEMORY >> select ARCH_HAS_SG_CHAIN >> diff --git a/arch/x86/include/asm/refcount.h >> b/arch/x86/include/asm/refcount.h >> index ff871210b9f2..4e44250e7d0d 100644 >> --- a/arch/x86/include/asm/refcount.h >> +++ b/arch/x86/include/asm/refcount.h >> @@ -15,7 +15,7 @@ >> * back to the regular execution flow in .text. >> */ >> #define _REFCOUNT_EXCEPTION\ >> - ".pushsection .text.unlikely\n" \ >> + ".pushsection .text..refcount\n"\ > > You could also use a .subsection here: those are always emitted out of > line, but into the same section. I considered it (especially after looking to see how you handled it in the arm64 port) but decided against it in favor of collecting them all at the end with .text.unlikely. -Kees -- Kees Cook Pixel Security
Re: [PATCH] locking/refcounts, x86/asm: Use unique .text section for refcount exceptions
On 1 September 2017 at 21:22, Kees Cookwrote: > Using .text.unlikely for refcount exceptions isn't safe because gcc may > move entire functions into .text.unlikely (e.g. in6_dev_get()), which > would cause any uses of a protected refcount_t function to stay inline > with the function, triggering the protection unconditionally: > > .section.text.unlikely,"ax",@progbits > .type in6_dev_get, @function > in6_dev_getx: > .LFB4673: > .loc 2 4128 0 > .cfi_startproc > ... > lock; incl 480(%rbx) > js 111f > .pushsection .text.unlikely > 111:lea 480(%rbx), %rcx > 112:.byte 0x0f, 0xff > .popsection > 113: > > This creates a unique .text section and adds an additional test to the > exception handler to WARN in the case of having none of OF, SF, nor ZF > set so we can see things like this more easily in the future. > > Reported-by: Mike Galbraith > Fixes: 7a46ec0e2f48 ("locking/refcounts, x86/asm: Implement fast refcount > overflow protection") > Signed-off-by: Kees Cook > --- > arch/x86/Kconfig | 2 +- > arch/x86/include/asm/refcount.h | 2 +- > arch/x86/mm/extable.c | 7 ++- > include/asm-generic/vmlinux.lds.h | 1 + > 4 files changed, 9 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > index eaa8ff41f424..c6acdcdb3fc6 100644 > --- a/arch/x86/Kconfig > +++ b/arch/x86/Kconfig > @@ -56,7 +56,7 @@ config X86 > select ARCH_HAS_MMIO_FLUSH > select ARCH_HAS_PMEM_APIif X86_64 > # Causing hangs/crashes, see the commit that added this change for > details. > - select ARCH_HAS_REFCOUNTif BROKEN > + select ARCH_HAS_REFCOUNT > select ARCH_HAS_UACCESS_FLUSHCACHE if X86_64 > select ARCH_HAS_SET_MEMORY > select ARCH_HAS_SG_CHAIN > diff --git a/arch/x86/include/asm/refcount.h b/arch/x86/include/asm/refcount.h > index ff871210b9f2..4e44250e7d0d 100644 > --- a/arch/x86/include/asm/refcount.h > +++ b/arch/x86/include/asm/refcount.h > @@ -15,7 +15,7 @@ > * back to the regular execution flow in .text. > */ > #define _REFCOUNT_EXCEPTION\ > - ".pushsection .text.unlikely\n" \ > + ".pushsection .text..refcount\n"\ You could also use a .subsection here: those are always emitted out of line, but into the same section. > "111:\tlea %[counter], %%" _ASM_CX "\n" \ > "112:\t" ASM_UD0 "\n" \ > ASM_UNREACHABLE \ > diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c > index c076f710de4c..cf0d74b47ae0 100644 > --- a/arch/x86/mm/extable.c > +++ b/arch/x86/mm/extable.c > @@ -66,12 +66,17 @@ bool ex_handler_refcount(const struct > exception_table_entry *fixup, > * wrapped around) will be set. Additionally, seeing the refcount > * reach 0 will set ZF (Zero Flag: result was zero). In each of > * these cases we want a report, since it's a boundary condition. > -* > +* The SF case is not reported since it indicates post-boundary > +* manipulations below zero or above INT_MAX. And if none of the > +* flags are set, something has gone very wrong, so report it. > */ > if (regs->flags & (X86_EFLAGS_OF | X86_EFLAGS_ZF)) { > bool zero = regs->flags & X86_EFLAGS_ZF; > > refcount_error_report(regs, zero ? "hit zero" : "overflow"); > + } else if ((regs->flags & X86_EFLAGS_SF) == 0) { > + /* Report if none of OF, ZF, nor SF are set. */ > + refcount_error_report(regs, "unexpected saturation"); > } > > return true; > diff --git a/include/asm-generic/vmlinux.lds.h > b/include/asm-generic/vmlinux.lds.h > index 8acfc1e099e1..e549bff87c5b 100644 > --- a/include/asm-generic/vmlinux.lds.h > +++ b/include/asm-generic/vmlinux.lds.h > @@ -459,6 +459,7 @@ > #define TEXT_TEXT \ > ALIGN_FUNCTION(); \ > *(.text.hot TEXT_MAIN .text.fixup .text.unlikely) \ > + *(.text..refcount) \ > *(.ref.text)\ > MEM_KEEP(init.text) \ > MEM_KEEP(exit.text) \ > -- > 2.7.4 > > > -- > Kees Cook > Pixel Security
Re: [PATCH] locking/refcounts, x86/asm: Use unique .text section for refcount exceptions
On 1 September 2017 at 21:22, Kees Cook wrote: > Using .text.unlikely for refcount exceptions isn't safe because gcc may > move entire functions into .text.unlikely (e.g. in6_dev_get()), which > would cause any uses of a protected refcount_t function to stay inline > with the function, triggering the protection unconditionally: > > .section.text.unlikely,"ax",@progbits > .type in6_dev_get, @function > in6_dev_getx: > .LFB4673: > .loc 2 4128 0 > .cfi_startproc > ... > lock; incl 480(%rbx) > js 111f > .pushsection .text.unlikely > 111:lea 480(%rbx), %rcx > 112:.byte 0x0f, 0xff > .popsection > 113: > > This creates a unique .text section and adds an additional test to the > exception handler to WARN in the case of having none of OF, SF, nor ZF > set so we can see things like this more easily in the future. > > Reported-by: Mike Galbraith > Fixes: 7a46ec0e2f48 ("locking/refcounts, x86/asm: Implement fast refcount > overflow protection") > Signed-off-by: Kees Cook > --- > arch/x86/Kconfig | 2 +- > arch/x86/include/asm/refcount.h | 2 +- > arch/x86/mm/extable.c | 7 ++- > include/asm-generic/vmlinux.lds.h | 1 + > 4 files changed, 9 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > index eaa8ff41f424..c6acdcdb3fc6 100644 > --- a/arch/x86/Kconfig > +++ b/arch/x86/Kconfig > @@ -56,7 +56,7 @@ config X86 > select ARCH_HAS_MMIO_FLUSH > select ARCH_HAS_PMEM_APIif X86_64 > # Causing hangs/crashes, see the commit that added this change for > details. > - select ARCH_HAS_REFCOUNTif BROKEN > + select ARCH_HAS_REFCOUNT > select ARCH_HAS_UACCESS_FLUSHCACHE if X86_64 > select ARCH_HAS_SET_MEMORY > select ARCH_HAS_SG_CHAIN > diff --git a/arch/x86/include/asm/refcount.h b/arch/x86/include/asm/refcount.h > index ff871210b9f2..4e44250e7d0d 100644 > --- a/arch/x86/include/asm/refcount.h > +++ b/arch/x86/include/asm/refcount.h > @@ -15,7 +15,7 @@ > * back to the regular execution flow in .text. > */ > #define _REFCOUNT_EXCEPTION\ > - ".pushsection .text.unlikely\n" \ > + ".pushsection .text..refcount\n"\ You could also use a .subsection here: those are always emitted out of line, but into the same section. > "111:\tlea %[counter], %%" _ASM_CX "\n" \ > "112:\t" ASM_UD0 "\n" \ > ASM_UNREACHABLE \ > diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c > index c076f710de4c..cf0d74b47ae0 100644 > --- a/arch/x86/mm/extable.c > +++ b/arch/x86/mm/extable.c > @@ -66,12 +66,17 @@ bool ex_handler_refcount(const struct > exception_table_entry *fixup, > * wrapped around) will be set. Additionally, seeing the refcount > * reach 0 will set ZF (Zero Flag: result was zero). In each of > * these cases we want a report, since it's a boundary condition. > -* > +* The SF case is not reported since it indicates post-boundary > +* manipulations below zero or above INT_MAX. And if none of the > +* flags are set, something has gone very wrong, so report it. > */ > if (regs->flags & (X86_EFLAGS_OF | X86_EFLAGS_ZF)) { > bool zero = regs->flags & X86_EFLAGS_ZF; > > refcount_error_report(regs, zero ? "hit zero" : "overflow"); > + } else if ((regs->flags & X86_EFLAGS_SF) == 0) { > + /* Report if none of OF, ZF, nor SF are set. */ > + refcount_error_report(regs, "unexpected saturation"); > } > > return true; > diff --git a/include/asm-generic/vmlinux.lds.h > b/include/asm-generic/vmlinux.lds.h > index 8acfc1e099e1..e549bff87c5b 100644 > --- a/include/asm-generic/vmlinux.lds.h > +++ b/include/asm-generic/vmlinux.lds.h > @@ -459,6 +459,7 @@ > #define TEXT_TEXT \ > ALIGN_FUNCTION(); \ > *(.text.hot TEXT_MAIN .text.fixup .text.unlikely) \ > + *(.text..refcount) \ > *(.ref.text)\ > MEM_KEEP(init.text) \ > MEM_KEEP(exit.text) \ > -- > 2.7.4 > > > -- > Kees Cook > Pixel Security
[PATCH] locking/refcounts, x86/asm: Use unique .text section for refcount exceptions
Using .text.unlikely for refcount exceptions isn't safe because gcc may move entire functions into .text.unlikely (e.g. in6_dev_get()), which would cause any uses of a protected refcount_t function to stay inline with the function, triggering the protection unconditionally: .section.text.unlikely,"ax",@progbits .type in6_dev_get, @function in6_dev_getx: .LFB4673: .loc 2 4128 0 .cfi_startproc ... lock; incl 480(%rbx) js 111f .pushsection .text.unlikely 111:lea 480(%rbx), %rcx 112:.byte 0x0f, 0xff .popsection 113: This creates a unique .text section and adds an additional test to the exception handler to WARN in the case of having none of OF, SF, nor ZF set so we can see things like this more easily in the future. Reported-by: Mike GalbraithFixes: 7a46ec0e2f48 ("locking/refcounts, x86/asm: Implement fast refcount overflow protection") Signed-off-by: Kees Cook --- arch/x86/Kconfig | 2 +- arch/x86/include/asm/refcount.h | 2 +- arch/x86/mm/extable.c | 7 ++- include/asm-generic/vmlinux.lds.h | 1 + 4 files changed, 9 insertions(+), 3 deletions(-) diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index eaa8ff41f424..c6acdcdb3fc6 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -56,7 +56,7 @@ config X86 select ARCH_HAS_MMIO_FLUSH select ARCH_HAS_PMEM_APIif X86_64 # Causing hangs/crashes, see the commit that added this change for details. - select ARCH_HAS_REFCOUNTif BROKEN + select ARCH_HAS_REFCOUNT select ARCH_HAS_UACCESS_FLUSHCACHE if X86_64 select ARCH_HAS_SET_MEMORY select ARCH_HAS_SG_CHAIN diff --git a/arch/x86/include/asm/refcount.h b/arch/x86/include/asm/refcount.h index ff871210b9f2..4e44250e7d0d 100644 --- a/arch/x86/include/asm/refcount.h +++ b/arch/x86/include/asm/refcount.h @@ -15,7 +15,7 @@ * back to the regular execution flow in .text. */ #define _REFCOUNT_EXCEPTION\ - ".pushsection .text.unlikely\n" \ + ".pushsection .text..refcount\n"\ "111:\tlea %[counter], %%" _ASM_CX "\n" \ "112:\t" ASM_UD0 "\n" \ ASM_UNREACHABLE \ diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c index c076f710de4c..cf0d74b47ae0 100644 --- a/arch/x86/mm/extable.c +++ b/arch/x86/mm/extable.c @@ -66,12 +66,17 @@ bool ex_handler_refcount(const struct exception_table_entry *fixup, * wrapped around) will be set. Additionally, seeing the refcount * reach 0 will set ZF (Zero Flag: result was zero). In each of * these cases we want a report, since it's a boundary condition. -* +* The SF case is not reported since it indicates post-boundary +* manipulations below zero or above INT_MAX. And if none of the +* flags are set, something has gone very wrong, so report it. */ if (regs->flags & (X86_EFLAGS_OF | X86_EFLAGS_ZF)) { bool zero = regs->flags & X86_EFLAGS_ZF; refcount_error_report(regs, zero ? "hit zero" : "overflow"); + } else if ((regs->flags & X86_EFLAGS_SF) == 0) { + /* Report if none of OF, ZF, nor SF are set. */ + refcount_error_report(regs, "unexpected saturation"); } return true; diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h index 8acfc1e099e1..e549bff87c5b 100644 --- a/include/asm-generic/vmlinux.lds.h +++ b/include/asm-generic/vmlinux.lds.h @@ -459,6 +459,7 @@ #define TEXT_TEXT \ ALIGN_FUNCTION(); \ *(.text.hot TEXT_MAIN .text.fixup .text.unlikely) \ + *(.text..refcount) \ *(.ref.text)\ MEM_KEEP(init.text) \ MEM_KEEP(exit.text) \ -- 2.7.4 -- Kees Cook Pixel Security
[PATCH] locking/refcounts, x86/asm: Use unique .text section for refcount exceptions
Using .text.unlikely for refcount exceptions isn't safe because gcc may move entire functions into .text.unlikely (e.g. in6_dev_get()), which would cause any uses of a protected refcount_t function to stay inline with the function, triggering the protection unconditionally: .section.text.unlikely,"ax",@progbits .type in6_dev_get, @function in6_dev_getx: .LFB4673: .loc 2 4128 0 .cfi_startproc ... lock; incl 480(%rbx) js 111f .pushsection .text.unlikely 111:lea 480(%rbx), %rcx 112:.byte 0x0f, 0xff .popsection 113: This creates a unique .text section and adds an additional test to the exception handler to WARN in the case of having none of OF, SF, nor ZF set so we can see things like this more easily in the future. Reported-by: Mike Galbraith Fixes: 7a46ec0e2f48 ("locking/refcounts, x86/asm: Implement fast refcount overflow protection") Signed-off-by: Kees Cook --- arch/x86/Kconfig | 2 +- arch/x86/include/asm/refcount.h | 2 +- arch/x86/mm/extable.c | 7 ++- include/asm-generic/vmlinux.lds.h | 1 + 4 files changed, 9 insertions(+), 3 deletions(-) diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index eaa8ff41f424..c6acdcdb3fc6 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -56,7 +56,7 @@ config X86 select ARCH_HAS_MMIO_FLUSH select ARCH_HAS_PMEM_APIif X86_64 # Causing hangs/crashes, see the commit that added this change for details. - select ARCH_HAS_REFCOUNTif BROKEN + select ARCH_HAS_REFCOUNT select ARCH_HAS_UACCESS_FLUSHCACHE if X86_64 select ARCH_HAS_SET_MEMORY select ARCH_HAS_SG_CHAIN diff --git a/arch/x86/include/asm/refcount.h b/arch/x86/include/asm/refcount.h index ff871210b9f2..4e44250e7d0d 100644 --- a/arch/x86/include/asm/refcount.h +++ b/arch/x86/include/asm/refcount.h @@ -15,7 +15,7 @@ * back to the regular execution flow in .text. */ #define _REFCOUNT_EXCEPTION\ - ".pushsection .text.unlikely\n" \ + ".pushsection .text..refcount\n"\ "111:\tlea %[counter], %%" _ASM_CX "\n" \ "112:\t" ASM_UD0 "\n" \ ASM_UNREACHABLE \ diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c index c076f710de4c..cf0d74b47ae0 100644 --- a/arch/x86/mm/extable.c +++ b/arch/x86/mm/extable.c @@ -66,12 +66,17 @@ bool ex_handler_refcount(const struct exception_table_entry *fixup, * wrapped around) will be set. Additionally, seeing the refcount * reach 0 will set ZF (Zero Flag: result was zero). In each of * these cases we want a report, since it's a boundary condition. -* +* The SF case is not reported since it indicates post-boundary +* manipulations below zero or above INT_MAX. And if none of the +* flags are set, something has gone very wrong, so report it. */ if (regs->flags & (X86_EFLAGS_OF | X86_EFLAGS_ZF)) { bool zero = regs->flags & X86_EFLAGS_ZF; refcount_error_report(regs, zero ? "hit zero" : "overflow"); + } else if ((regs->flags & X86_EFLAGS_SF) == 0) { + /* Report if none of OF, ZF, nor SF are set. */ + refcount_error_report(regs, "unexpected saturation"); } return true; diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h index 8acfc1e099e1..e549bff87c5b 100644 --- a/include/asm-generic/vmlinux.lds.h +++ b/include/asm-generic/vmlinux.lds.h @@ -459,6 +459,7 @@ #define TEXT_TEXT \ ALIGN_FUNCTION(); \ *(.text.hot TEXT_MAIN .text.fixup .text.unlikely) \ + *(.text..refcount) \ *(.ref.text)\ MEM_KEEP(init.text) \ MEM_KEEP(exit.text) \ -- 2.7.4 -- Kees Cook Pixel Security