| Message ID | 20170719193718.bvkkde5apbboudrk@treble (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jul 19, 2017 at 12:37 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote: > On Tue, Jul 18, 2017 at 05:03:34PM -0700, Kees Cook wrote: >> +/* >> + * Body of refcount error handling: in .text.unlikely, saved into CX the >> + * address of the refcount that has entered a bad state, and trigger an >> + * exception. Fixup address is back in regular execution flow in .text. >> + */ >> +#define _REFCOUNT_EXCEPTION \ >> + ".pushsection .text.unlikely\n" \ >> + "111:\tlea %[counter], %%" _ASM_CX "\n" \ >> + "112:\t" ASM_UD0 "\n" \ >> + ".popsection\n" \ >> + "113:\n" \ >> + _ASM_EXTABLE_REFCOUNT(112b, 113b) > > This confuses the freshly merged objtool 2.0, which is now too smart for > its own good. It's reporting some errors like: > > >> kernel/sched/autogroup.o: warning: objtool: sched_autogroup_exit()+0x48: return with modified stack frame > >> kernel/sched/autogroup.o: warning: objtool: .text.unlikely+0x27: stack state mismatch: reg1[3]=-2-40 reg2[3]=-2-24 > >> kernel/sched/autogroup.o: warning: objtool: sched_autogroup_exit()+0x14: stack state mismatch: reg1[3]=-2-40 reg2[3]=-2-24 > > Because the UD instructions are used for both WARN and BUG, objtool > doesn't know whether control flow continues past the instruction. So in > cases like this, it needs an "unreachable" annotation. > > Here's a patch to fix it, feel free to squash it into yours: Thanks! I'll add it for v7. > diff --git a/arch/x86/include/asm/refcount.h b/arch/x86/include/asm/refcount.h > index 13b91e850a02..e7587db3487c 100644 > --- a/arch/x86/include/asm/refcount.h > +++ b/arch/x86/include/asm/refcount.h > @@ -15,6 +15,7 @@ > ".pushsection .text.unlikely\n" \ > "111:\tlea %[counter], %%" _ASM_CX "\n" \ > "112:\t" ASM_UD0 "\n" \ > + ASM_UNREACHABLE \ > ".popsection\n" \ > "113:\n" \ > _ASM_EXTABLE_REFCOUNT(112b, 113b) > diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h > index cd4bbe8242bd..85e0b8f42ca0 100644 > --- a/include/linux/compiler-gcc.h > +++ b/include/linux/compiler-gcc.h > @@ -202,15 +202,25 @@ > #endif > > #ifdef CONFIG_STACK_VALIDATION > + > #define annotate_unreachable() ({ \ > asm("%c0:\t\n" \ > - ".pushsection .discard.unreachable\t\n" \ > - ".long %c0b - .\t\n" \ > - ".popsection\t\n" : : "i" (__LINE__)); \ > + ".pushsection .discard.unreachable\n\t" \ > + ".long %c0b - .\n\t" \ > + ".popsection\n\t" : : "i" (__LINE__)); \ Is this just an indentation change? > }) > + > +#define ASM_UNREACHABLE \ > + "999: .pushsection .discard.unreachable\n\t" \ > + ".long 999b - .\n\t" \ > + ".popsection\n\t" Just so I understand, we'll get a single byte added for each exception case, but it'll get discarded during final link? > + > #else > + > #define annotate_unreachable() > -#endif > +#define ASM_UNREACHABLE > + > +#endif /* CONFIG_STACK_VALIDATION */ > > /* > * Mark a position in code as unreachable. This can be used to Thanks! -Kees
On Wed, Jul 19, 2017 at 12:45:19PM -0700, Kees Cook wrote: > > diff --git a/arch/x86/include/asm/refcount.h b/arch/x86/include/asm/refcount.h > > index 13b91e850a02..e7587db3487c 100644 > > --- a/arch/x86/include/asm/refcount.h > > +++ b/arch/x86/include/asm/refcount.h > > @@ -15,6 +15,7 @@ > > ".pushsection .text.unlikely\n" \ > > "111:\tlea %[counter], %%" _ASM_CX "\n" \ > > "112:\t" ASM_UD0 "\n" \ > > + ASM_UNREACHABLE \ > > ".popsection\n" \ > > "113:\n" \ > > _ASM_EXTABLE_REFCOUNT(112b, 113b) > > diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h > > index cd4bbe8242bd..85e0b8f42ca0 100644 > > --- a/include/linux/compiler-gcc.h > > +++ b/include/linux/compiler-gcc.h > > @@ -202,15 +202,25 @@ > > #endif > > > > #ifdef CONFIG_STACK_VALIDATION > > + > > #define annotate_unreachable() ({ \ > > asm("%c0:\t\n" \ > > - ".pushsection .discard.unreachable\t\n" \ > > - ".long %c0b - .\t\n" \ > > - ".popsection\t\n" : : "i" (__LINE__)); \ > > + ".pushsection .discard.unreachable\n\t" \ > > + ".long %c0b - .\n\t" \ > > + ".popsection\n\t" : : "i" (__LINE__)); \ > > Is this just an indentation change? This was sneaking in a fix to put the tab after the newline instead of before it. I figured it's not worth its own commit. > > }) > > + > > +#define ASM_UNREACHABLE \ > > + "999: .pushsection .discard.unreachable\n\t" \ > > + ".long 999b - .\n\t" \ > > + ".popsection\n\t" > > Just so I understand, we'll get a single byte added for each exception > case, but it'll get discarded during final link? I think it's four bytes actually, but yeah, the section gets stripped at vmlinux link time.
On Wed, Jul 19, 2017 at 12:52 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote: > On Wed, Jul 19, 2017 at 12:45:19PM -0700, Kees Cook wrote: >> > diff --git a/arch/x86/include/asm/refcount.h b/arch/x86/include/asm/refcount.h >> > index 13b91e850a02..e7587db3487c 100644 >> > --- a/arch/x86/include/asm/refcount.h >> > +++ b/arch/x86/include/asm/refcount.h >> > @@ -15,6 +15,7 @@ >> > ".pushsection .text.unlikely\n" \ >> > "111:\tlea %[counter], %%" _ASM_CX "\n" \ >> > "112:\t" ASM_UD0 "\n" \ >> > + ASM_UNREACHABLE \ >> > ".popsection\n" \ >> > "113:\n" \ >> > _ASM_EXTABLE_REFCOUNT(112b, 113b) >> > diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h >> > index cd4bbe8242bd..85e0b8f42ca0 100644 >> > --- a/include/linux/compiler-gcc.h >> > +++ b/include/linux/compiler-gcc.h >> > @@ -202,15 +202,25 @@ >> > #endif >> > >> > #ifdef CONFIG_STACK_VALIDATION >> > + >> > #define annotate_unreachable() ({ \ >> > asm("%c0:\t\n" \ >> > - ".pushsection .discard.unreachable\t\n" \ >> > - ".long %c0b - .\t\n" \ >> > - ".popsection\t\n" : : "i" (__LINE__)); \ >> > + ".pushsection .discard.unreachable\n\t" \ >> > + ".long %c0b - .\n\t" \ >> > + ".popsection\n\t" : : "i" (__LINE__)); \ >> >> Is this just an indentation change? > > This was sneaking in a fix to put the tab after the newline instead of > before it. I figured it's not worth its own commit. Ah! Now I see it. Gotcha. >> > }) >> > + >> > +#define ASM_UNREACHABLE \ >> > + "999: .pushsection .discard.unreachable\n\t" \ >> > + ".long 999b - .\n\t" \ >> > + ".popsection\n\t" >> >> Just so I understand, we'll get a single byte added for each exception >> case, but it'll get discarded during final link? > > I think it's four bytes actually, but yeah, the section gets stripped at > vmlinux link time. Right, yes. BTW, I think this needs compiler.h coverage instead of the #else in compiler-gcc.h (since it's different from how annotate_unreachable is used only in compiler-gcc.h. I'll adjust. Also, in looking at CONFIG_STACK_VALIDATION, do you want it to just warn and skip, or do you want to error out the build if validation isn't available but it's in the .config? -Kees
On Wed, Jul 19, 2017 at 03:50:14PM -0700, Kees Cook wrote: > >> > }) > >> > + > >> > +#define ASM_UNREACHABLE \ > >> > + "999: .pushsection .discard.unreachable\n\t" \ > >> > + ".long 999b - .\n\t" \ > >> > + ".popsection\n\t" > >> > >> Just so I understand, we'll get a single byte added for each exception > >> case, but it'll get discarded during final link? > > > > I think it's four bytes actually, but yeah, the section gets stripped at > > vmlinux link time. > > Right, yes. > > BTW, I think this needs compiler.h coverage instead of the #else in > compiler-gcc.h (since it's different from how annotate_unreachable is > used only in compiler-gcc.h. I'll adjust. Ah, right. Sounds good. > Also, in looking at CONFIG_STACK_VALIDATION, do you want it to just > warn and skip, or do you want to error out the build if validation > isn't available but it's in the .config? I think the current warn and skip behavior is fine. It's usually not a life-or-death matter, and if it is, you'll be checking the warnings anyway.
On Wed, Jul 19, 2017 at 12:37 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote: > +#define ASM_UNREACHABLE \ > + "999: .pushsection .discard.unreachable\n\t" \ > + ".long 999b - .\n\t" \ > + ".popsection\n\t" Will this end up running into the same bug fixed with 3d1e236022cc1426b0834565995ddee2ca231cee and the __LINE__ macro? I could tell under which conditions gcc would do this kind of merging... -Kees
On Wed, Jul 19, 2017 at 04:12:37PM -0700, Kees Cook wrote: > On Wed, Jul 19, 2017 at 12:37 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > +#define ASM_UNREACHABLE \ > > + "999: .pushsection .discard.unreachable\n\t" \ > > + ".long 999b - .\n\t" \ > > + ".popsection\n\t" > > Will this end up running into the same bug fixed with > 3d1e236022cc1426b0834565995ddee2ca231cee and the __LINE__ macro? I > could tell under which conditions gcc would do this kind of merging... I hope not, but objtool should complain about it if it does. We could involve the __LINE__ macro, or some other way to make the labels unique, but that might complicate the code a bit, so we can delay doing that unless necessary.
diff --git a/arch/x86/include/asm/refcount.h b/arch/x86/include/asm/refcount.h index 13b91e850a02..e7587db3487c 100644 --- a/arch/x86/include/asm/refcount.h +++ b/arch/x86/include/asm/refcount.h @@ -15,6 +15,7 @@ ".pushsection .text.unlikely\n" \ "111:\tlea %[counter], %%" _ASM_CX "\n" \ "112:\t" ASM_UD0 "\n" \ + ASM_UNREACHABLE \ ".popsection\n" \ "113:\n" \ _ASM_EXTABLE_REFCOUNT(112b, 113b) diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h index cd4bbe8242bd..85e0b8f42ca0 100644 --- a/include/linux/compiler-gcc.h +++ b/include/linux/compiler-gcc.h @@ -202,15 +202,25 @@ #endif #ifdef CONFIG_STACK_VALIDATION + #define annotate_unreachable() ({ \ asm("%c0:\t\n" \ - ".pushsection .discard.unreachable\t\n" \ - ".long %c0b - .\t\n" \ - ".popsection\t\n" : : "i" (__LINE__)); \ + ".pushsection .discard.unreachable\n\t" \ + ".long %c0b - .\n\t" \ + ".popsection\n\t" : : "i" (__LINE__)); \ }) + +#define ASM_UNREACHABLE \ + "999: .pushsection .discard.unreachable\n\t" \ + ".long 999b - .\n\t" \ + ".popsection\n\t" + #else + #define annotate_unreachable() -#endif +#define ASM_UNREACHABLE + +#endif /* CONFIG_STACK_VALIDATION */ /* * Mark a position in code as unreachable. This can be used to