| Message ID | 20231128100352.35430-2-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/x86: add testing for self modifying code and livepatch | expand |
On 28.11.2023 11:03, Roger Pau Monne wrote: > The minimal function size requirements for livepatch are either 5 bytes (for > jmp) or 9 bytes (for endbr + jmp). Ensure that functions are always at least > that size by requesting the compiled to align the functions to 8 or 16 bytes, > depending on whether Xen is build with IBT support. How is alignment going to enforce minimum function size? If a function is last in a section, there may not be any padding added (ahead of linking at least). The trailing padding also isn't part of the function. > Note that it's possible for the compiler to end up using a higher function > alignment regardless of the passed value, so this change just make sure that > the minimum required for livepatch to work is present. > > Since the option (-falign-functions) is supported by both minimal required > compiler versions of clang and gcc there's no need to add a test to check for > its presence. > > The alignment is currently only implemented for livepatch on x86, I'm unsure > whether ARM has a mandatory function alignment high enough to cover for the > space required by the replacement instruction(s). Indeed I was wondering whether this shouldn't be generalized, if indeed required. Jan
On Thu, Nov 30, 2023 at 05:55:07PM +0100, Jan Beulich wrote: > On 28.11.2023 11:03, Roger Pau Monne wrote: > > The minimal function size requirements for livepatch are either 5 bytes (for > > jmp) or 9 bytes (for endbr + jmp). Ensure that functions are always at least > > that size by requesting the compiled to align the functions to 8 or 16 bytes, > > depending on whether Xen is build with IBT support. > > How is alignment going to enforce minimum function size? If a function is > last in a section, there may not be any padding added (ahead of linking at > least). The trailing padding also isn't part of the function. If each function lives in it's own section (by using -ffunction-sections), and each section is aligned, then I think we can guarantee that there will always be enough padding space? Even the last function/section on the .text block would still be aligned, and as long as the function alignment <= SECTION_ALIGN there will be enough padding left. I should add some build time assert that CONFIG_CC_FUNCTION_ALIGNMENT <= SECTION_ALIGN. While the trailing padding is not part of the function itself, I don't see issues in overwriting it with the replacement function code. > > Note that it's possible for the compiler to end up using a higher function > > alignment regardless of the passed value, so this change just make sure that > > the minimum required for livepatch to work is present. > > > > Since the option (-falign-functions) is supported by both minimal required > > compiler versions of clang and gcc there's no need to add a test to check for > > its presence. > > > > The alignment is currently only implemented for livepatch on x86, I'm unsure > > whether ARM has a mandatory function alignment high enough to cover for the > > space required by the replacement instruction(s). > > Indeed I was wondering whether this shouldn't be generalized, if indeed > required. Well, more than required it's nice to have in order to avoid failures. The checks in the livepatch code will trigger if there's not enough space to apply the replacement code. Thanks, Roger.
On 30.11.2023 18:37, Roger Pau Monné wrote: > On Thu, Nov 30, 2023 at 05:55:07PM +0100, Jan Beulich wrote: >> On 28.11.2023 11:03, Roger Pau Monne wrote: >>> The minimal function size requirements for livepatch are either 5 bytes (for >>> jmp) or 9 bytes (for endbr + jmp). Ensure that functions are always at least >>> that size by requesting the compiled to align the functions to 8 or 16 bytes, >>> depending on whether Xen is build with IBT support. >> >> How is alignment going to enforce minimum function size? If a function is >> last in a section, there may not be any padding added (ahead of linking at >> least). The trailing padding also isn't part of the function. > > If each function lives in it's own section (by using > -ffunction-sections), and each section is aligned, then I think we can > guarantee that there will always be enough padding space? > > Even the last function/section on the .text block would still be > aligned, and as long as the function alignment <= SECTION_ALIGN > there will be enough padding left. I should add some build time > assert that CONFIG_CC_FUNCTION_ALIGNMENT <= SECTION_ALIGN. I'm not sure of there being a requirement for a section to be padded to its alignment. If the following section has smaller alignment, it could be made start earlier. Of course our linker scripts might guarantee this ... > While the trailing padding is not part of the function itself, I don't > see issues in overwriting it with the replacement function code. If it's really padding, yes. Jan
On Fri, Dec 01, 2023 at 07:53:29AM +0100, Jan Beulich wrote: > On 30.11.2023 18:37, Roger Pau Monné wrote: > > On Thu, Nov 30, 2023 at 05:55:07PM +0100, Jan Beulich wrote: > >> On 28.11.2023 11:03, Roger Pau Monne wrote: > >>> The minimal function size requirements for livepatch are either 5 bytes (for > >>> jmp) or 9 bytes (for endbr + jmp). Ensure that functions are always at least > >>> that size by requesting the compiled to align the functions to 8 or 16 bytes, > >>> depending on whether Xen is build with IBT support. > >> > >> How is alignment going to enforce minimum function size? If a function is > >> last in a section, there may not be any padding added (ahead of linking at > >> least). The trailing padding also isn't part of the function. > > > > If each function lives in it's own section (by using > > -ffunction-sections), and each section is aligned, then I think we can > > guarantee that there will always be enough padding space? > > > > Even the last function/section on the .text block would still be > > aligned, and as long as the function alignment <= SECTION_ALIGN > > there will be enough padding left. I should add some build time > > assert that CONFIG_CC_FUNCTION_ALIGNMENT <= SECTION_ALIGN. > > I'm not sure of there being a requirement for a section to be padded to > its alignment. If the following section has smaller alignment, it could > be made start earlier. Of course our linker scripts might guarantee > this ... I do think so, given our linker script arrangements for the .text section: DECL_SECTION(.text) { [...] } PHDR(text) = 0x9090 . = ALIGN(SECTION_ALIGN); The end of the text section is aligned to SECTION_ALIGN, so as long as SECTION_ALIGN >= CONFIG_CC_FUNCTION_ALIGNMENT the alignment should guarantee a minimal function size. Do you think it would be clearer if I add the following paragraph: "Given the Xen linker script arrangement of the .text section, we can ensure that when all functions are aligned to the given boundary the function size will always be a multiple of such alignment, even for the last function in .text, as the linker script aligns the end of the section to SECTION_ALIGN." Thanks, Roger.
On 01.12.2023 09:50, Roger Pau Monné wrote: > On Fri, Dec 01, 2023 at 07:53:29AM +0100, Jan Beulich wrote: >> On 30.11.2023 18:37, Roger Pau Monné wrote: >>> On Thu, Nov 30, 2023 at 05:55:07PM +0100, Jan Beulich wrote: >>>> On 28.11.2023 11:03, Roger Pau Monne wrote: >>>>> The minimal function size requirements for livepatch are either 5 bytes (for >>>>> jmp) or 9 bytes (for endbr + jmp). Ensure that functions are always at least >>>>> that size by requesting the compiled to align the functions to 8 or 16 bytes, >>>>> depending on whether Xen is build with IBT support. >>>> >>>> How is alignment going to enforce minimum function size? If a function is >>>> last in a section, there may not be any padding added (ahead of linking at >>>> least). The trailing padding also isn't part of the function. >>> >>> If each function lives in it's own section (by using >>> -ffunction-sections), and each section is aligned, then I think we can >>> guarantee that there will always be enough padding space? >>> >>> Even the last function/section on the .text block would still be >>> aligned, and as long as the function alignment <= SECTION_ALIGN >>> there will be enough padding left. I should add some build time >>> assert that CONFIG_CC_FUNCTION_ALIGNMENT <= SECTION_ALIGN. >> >> I'm not sure of there being a requirement for a section to be padded to >> its alignment. If the following section has smaller alignment, it could >> be made start earlier. Of course our linker scripts might guarantee >> this ... > > I do think so, given our linker script arrangements for the .text > section: > > DECL_SECTION(.text) { > [...] > } PHDR(text) = 0x9090 > > . = ALIGN(SECTION_ALIGN); > > The end of the text section is aligned to SECTION_ALIGN, so as long as > SECTION_ALIGN >= CONFIG_CC_FUNCTION_ALIGNMENT the alignment should > guarantee a minimal function size. > > Do you think it would be clearer if I add the following paragraph: > > "Given the Xen linker script arrangement of the .text section, we can > ensure that when all functions are aligned to the given boundary the > function size will always be a multiple of such alignment, even for > the last function in .text, as the linker script aligns the end of the > section to SECTION_ALIGN." I think this would be useful to have there. Beyond that, assembly code also needs considering btw. Jan
On Fri, Dec 01, 2023 at 10:41:45AM +0100, Jan Beulich wrote: > On 01.12.2023 09:50, Roger Pau Monné wrote: > > On Fri, Dec 01, 2023 at 07:53:29AM +0100, Jan Beulich wrote: > >> On 30.11.2023 18:37, Roger Pau Monné wrote: > >>> On Thu, Nov 30, 2023 at 05:55:07PM +0100, Jan Beulich wrote: > >>>> On 28.11.2023 11:03, Roger Pau Monne wrote: > >>>>> The minimal function size requirements for livepatch are either 5 bytes (for > >>>>> jmp) or 9 bytes (for endbr + jmp). Ensure that functions are always at least > >>>>> that size by requesting the compiled to align the functions to 8 or 16 bytes, > >>>>> depending on whether Xen is build with IBT support. > >>>> > >>>> How is alignment going to enforce minimum function size? If a function is > >>>> last in a section, there may not be any padding added (ahead of linking at > >>>> least). The trailing padding also isn't part of the function. > >>> > >>> If each function lives in it's own section (by using > >>> -ffunction-sections), and each section is aligned, then I think we can > >>> guarantee that there will always be enough padding space? > >>> > >>> Even the last function/section on the .text block would still be > >>> aligned, and as long as the function alignment <= SECTION_ALIGN > >>> there will be enough padding left. I should add some build time > >>> assert that CONFIG_CC_FUNCTION_ALIGNMENT <= SECTION_ALIGN. > >> > >> I'm not sure of there being a requirement for a section to be padded to > >> its alignment. If the following section has smaller alignment, it could > >> be made start earlier. Of course our linker scripts might guarantee > >> this ... > > > > I do think so, given our linker script arrangements for the .text > > section: > > > > DECL_SECTION(.text) { > > [...] > > } PHDR(text) = 0x9090 > > > > . = ALIGN(SECTION_ALIGN); > > > > The end of the text section is aligned to SECTION_ALIGN, so as long as > > SECTION_ALIGN >= CONFIG_CC_FUNCTION_ALIGNMENT the alignment should > > guarantee a minimal function size. > > > > Do you think it would be clearer if I add the following paragraph: > > > > "Given the Xen linker script arrangement of the .text section, we can > > ensure that when all functions are aligned to the given boundary the > > function size will always be a multiple of such alignment, even for > > the last function in .text, as the linker script aligns the end of the > > section to SECTION_ALIGN." > > I think this would be useful to have there. Beyond that, assembly code > also needs considering btw. Assembly will get dealt with once we start to also have separate sections for each assembly function. We cannot patch assembly code at the moment anyway, due to lack of debug symbols. Thanks, Roger.
On 01.12.2023 11:21, Roger Pau Monné wrote: > On Fri, Dec 01, 2023 at 10:41:45AM +0100, Jan Beulich wrote: >> On 01.12.2023 09:50, Roger Pau Monné wrote: >>> On Fri, Dec 01, 2023 at 07:53:29AM +0100, Jan Beulich wrote: >>>> On 30.11.2023 18:37, Roger Pau Monné wrote: >>>>> On Thu, Nov 30, 2023 at 05:55:07PM +0100, Jan Beulich wrote: >>>>>> On 28.11.2023 11:03, Roger Pau Monne wrote: >>>>>>> The minimal function size requirements for livepatch are either 5 bytes (for >>>>>>> jmp) or 9 bytes (for endbr + jmp). Ensure that functions are always at least >>>>>>> that size by requesting the compiled to align the functions to 8 or 16 bytes, >>>>>>> depending on whether Xen is build with IBT support. >>>>>> >>>>>> How is alignment going to enforce minimum function size? If a function is >>>>>> last in a section, there may not be any padding added (ahead of linking at >>>>>> least). The trailing padding also isn't part of the function. >>>>> >>>>> If each function lives in it's own section (by using >>>>> -ffunction-sections), and each section is aligned, then I think we can >>>>> guarantee that there will always be enough padding space? >>>>> >>>>> Even the last function/section on the .text block would still be >>>>> aligned, and as long as the function alignment <= SECTION_ALIGN >>>>> there will be enough padding left. I should add some build time >>>>> assert that CONFIG_CC_FUNCTION_ALIGNMENT <= SECTION_ALIGN. >>>> >>>> I'm not sure of there being a requirement for a section to be padded to >>>> its alignment. If the following section has smaller alignment, it could >>>> be made start earlier. Of course our linker scripts might guarantee >>>> this ... >>> >>> I do think so, given our linker script arrangements for the .text >>> section: >>> >>> DECL_SECTION(.text) { >>> [...] >>> } PHDR(text) = 0x9090 >>> >>> . = ALIGN(SECTION_ALIGN); >>> >>> The end of the text section is aligned to SECTION_ALIGN, so as long as >>> SECTION_ALIGN >= CONFIG_CC_FUNCTION_ALIGNMENT the alignment should >>> guarantee a minimal function size. >>> >>> Do you think it would be clearer if I add the following paragraph: >>> >>> "Given the Xen linker script arrangement of the .text section, we can >>> ensure that when all functions are aligned to the given boundary the >>> function size will always be a multiple of such alignment, even for >>> the last function in .text, as the linker script aligns the end of the >>> section to SECTION_ALIGN." >> >> I think this would be useful to have there. Beyond that, assembly code >> also needs considering btw. > > Assembly will get dealt with once we start to also have separate > sections for each assembly function. We cannot patch assembly code at > the moment anyway, due to lack of debug symbols. Well, yes, that's one part of it. The other is that some .text coming from an assembly source may follow one coming from some C source, and if the assembly one then isn't properly aligned, padding space again wouldn't necessarily be large enough. This may be alright now (where .text is the only thing that can come from .S and would be linked ahead of all .text.*, being the only thing that can come from .c), but it might subtly when assembly code is also switched to per-function sections (you may recall that a patch to this effect is already pending: "common: honor CONFIG_CC_SPLIT_SECTIONS also for assembly functions"). Jan
On Fri, Dec 01, 2023 at 11:59:09AM +0100, Jan Beulich wrote: > On 01.12.2023 11:21, Roger Pau Monné wrote: > > On Fri, Dec 01, 2023 at 10:41:45AM +0100, Jan Beulich wrote: > >> On 01.12.2023 09:50, Roger Pau Monné wrote: > >>> On Fri, Dec 01, 2023 at 07:53:29AM +0100, Jan Beulich wrote: > >>>> On 30.11.2023 18:37, Roger Pau Monné wrote: > >>>>> On Thu, Nov 30, 2023 at 05:55:07PM +0100, Jan Beulich wrote: > >>>>>> On 28.11.2023 11:03, Roger Pau Monne wrote: > >>>>>>> The minimal function size requirements for livepatch are either 5 bytes (for > >>>>>>> jmp) or 9 bytes (for endbr + jmp). Ensure that functions are always at least > >>>>>>> that size by requesting the compiled to align the functions to 8 or 16 bytes, > >>>>>>> depending on whether Xen is build with IBT support. > >>>>>> > >>>>>> How is alignment going to enforce minimum function size? If a function is > >>>>>> last in a section, there may not be any padding added (ahead of linking at > >>>>>> least). The trailing padding also isn't part of the function. > >>>>> > >>>>> If each function lives in it's own section (by using > >>>>> -ffunction-sections), and each section is aligned, then I think we can > >>>>> guarantee that there will always be enough padding space? > >>>>> > >>>>> Even the last function/section on the .text block would still be > >>>>> aligned, and as long as the function alignment <= SECTION_ALIGN > >>>>> there will be enough padding left. I should add some build time > >>>>> assert that CONFIG_CC_FUNCTION_ALIGNMENT <= SECTION_ALIGN. > >>>> > >>>> I'm not sure of there being a requirement for a section to be padded to > >>>> its alignment. If the following section has smaller alignment, it could > >>>> be made start earlier. Of course our linker scripts might guarantee > >>>> this ... > >>> > >>> I do think so, given our linker script arrangements for the .text > >>> section: > >>> > >>> DECL_SECTION(.text) { > >>> [...] > >>> } PHDR(text) = 0x9090 > >>> > >>> . = ALIGN(SECTION_ALIGN); > >>> > >>> The end of the text section is aligned to SECTION_ALIGN, so as long as > >>> SECTION_ALIGN >= CONFIG_CC_FUNCTION_ALIGNMENT the alignment should > >>> guarantee a minimal function size. > >>> > >>> Do you think it would be clearer if I add the following paragraph: > >>> > >>> "Given the Xen linker script arrangement of the .text section, we can > >>> ensure that when all functions are aligned to the given boundary the > >>> function size will always be a multiple of such alignment, even for > >>> the last function in .text, as the linker script aligns the end of the > >>> section to SECTION_ALIGN." > >> > >> I think this would be useful to have there. Beyond that, assembly code > >> also needs considering btw. > > > > Assembly will get dealt with once we start to also have separate > > sections for each assembly function. We cannot patch assembly code at > > the moment anyway, due to lack of debug symbols. > > Well, yes, that's one part of it. The other is that some .text coming > from an assembly source may follow one coming from some C source, and > if the assembly one then isn't properly aligned, padding space again > wouldn't necessarily be large enough. This may be alright now (where > .text is the only thing that can come from .S and would be linked > ahead of all .text.*, being the only thing that can come from .c), What about adding: #ifdef CONFIG_CC_SPLIT_SECTIONS *(.text.*) #endif #ifdef CONFIG_CC_FUNCTION_ALIGNMENT /* Ensure enough padding regardless of next section alignment. */ . = ALIGN(CONFIG_CC_FUNCTION_ALIGNMENT) #endif In order to assert that the end of .text.* is also aligned? > but > it might subtly when assembly code is also switched to per-function > sections (you may recall that a patch to this effect is already > pending: "common: honor CONFIG_CC_SPLIT_SECTIONS also for assembly > functions"). Yes, I think such patch should also honor the required alignment specified in CONFIG_CC_FUNCTION_ALIGNMENT. Thanks, Roger.
On 01.12.2023 12:31, Roger Pau Monné wrote: > On Fri, Dec 01, 2023 at 11:59:09AM +0100, Jan Beulich wrote: >> On 01.12.2023 11:21, Roger Pau Monné wrote: >>> On Fri, Dec 01, 2023 at 10:41:45AM +0100, Jan Beulich wrote: >>>> On 01.12.2023 09:50, Roger Pau Monné wrote: >>>>> On Fri, Dec 01, 2023 at 07:53:29AM +0100, Jan Beulich wrote: >>>>>> On 30.11.2023 18:37, Roger Pau Monné wrote: >>>>>>> On Thu, Nov 30, 2023 at 05:55:07PM +0100, Jan Beulich wrote: >>>>>>>> On 28.11.2023 11:03, Roger Pau Monne wrote: >>>>>>>>> The minimal function size requirements for livepatch are either 5 bytes (for >>>>>>>>> jmp) or 9 bytes (for endbr + jmp). Ensure that functions are always at least >>>>>>>>> that size by requesting the compiled to align the functions to 8 or 16 bytes, >>>>>>>>> depending on whether Xen is build with IBT support. >>>>>>>> >>>>>>>> How is alignment going to enforce minimum function size? If a function is >>>>>>>> last in a section, there may not be any padding added (ahead of linking at >>>>>>>> least). The trailing padding also isn't part of the function. >>>>>>> >>>>>>> If each function lives in it's own section (by using >>>>>>> -ffunction-sections), and each section is aligned, then I think we can >>>>>>> guarantee that there will always be enough padding space? >>>>>>> >>>>>>> Even the last function/section on the .text block would still be >>>>>>> aligned, and as long as the function alignment <= SECTION_ALIGN >>>>>>> there will be enough padding left. I should add some build time >>>>>>> assert that CONFIG_CC_FUNCTION_ALIGNMENT <= SECTION_ALIGN. >>>>>> >>>>>> I'm not sure of there being a requirement for a section to be padded to >>>>>> its alignment. If the following section has smaller alignment, it could >>>>>> be made start earlier. Of course our linker scripts might guarantee >>>>>> this ... >>>>> >>>>> I do think so, given our linker script arrangements for the .text >>>>> section: >>>>> >>>>> DECL_SECTION(.text) { >>>>> [...] >>>>> } PHDR(text) = 0x9090 >>>>> >>>>> . = ALIGN(SECTION_ALIGN); >>>>> >>>>> The end of the text section is aligned to SECTION_ALIGN, so as long as >>>>> SECTION_ALIGN >= CONFIG_CC_FUNCTION_ALIGNMENT the alignment should >>>>> guarantee a minimal function size. >>>>> >>>>> Do you think it would be clearer if I add the following paragraph: >>>>> >>>>> "Given the Xen linker script arrangement of the .text section, we can >>>>> ensure that when all functions are aligned to the given boundary the >>>>> function size will always be a multiple of such alignment, even for >>>>> the last function in .text, as the linker script aligns the end of the >>>>> section to SECTION_ALIGN." >>>> >>>> I think this would be useful to have there. Beyond that, assembly code >>>> also needs considering btw. >>> >>> Assembly will get dealt with once we start to also have separate >>> sections for each assembly function. We cannot patch assembly code at >>> the moment anyway, due to lack of debug symbols. >> >> Well, yes, that's one part of it. The other is that some .text coming >> from an assembly source may follow one coming from some C source, and >> if the assembly one then isn't properly aligned, padding space again >> wouldn't necessarily be large enough. This may be alright now (where >> .text is the only thing that can come from .S and would be linked >> ahead of all .text.*, being the only thing that can come from .c), > > What about adding: > > #ifdef CONFIG_CC_SPLIT_SECTIONS > *(.text.*) > #endif > #ifdef CONFIG_CC_FUNCTION_ALIGNMENT > /* Ensure enough padding regardless of next section alignment. */ > . = ALIGN(CONFIG_CC_FUNCTION_ALIGNMENT) > #endif > > In order to assert that the end of .text.* is also aligned? Probably. >> but >> it might subtly when assembly code is also switched to per-function >> sections (you may recall that a patch to this effect is already >> pending: "common: honor CONFIG_CC_SPLIT_SECTIONS also for assembly >> functions"). > > Yes, I think such patch should also honor the required alignment > specified in CONFIG_CC_FUNCTION_ALIGNMENT. I've added a note for myself to that patch, to adjust once yours has landed (which given the state of my series is likely going to be much earlier). Jan
On 28/11/2023 10:03 am, Roger Pau Monne wrote: > diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile > index f3abdf9cd111..f629157086d0 100644 > --- a/xen/arch/x86/Makefile > +++ b/xen/arch/x86/Makefile > @@ -82,6 +82,8 @@ obj-$(CONFIG_COMPAT) += x86_64/platform_hypercall.o > obj-y += sysctl.o > endif > > +CFLAGS-$(CONFIG_LIVEPATCH) += -falign-functions=$(CONFIG_CC_FUNCTION_ALIGNMENT) I'd really prefer not to express it like this. For one, a major reason for using an alignment of 16b or more is simply performance. Also, it isn't "CC" when we get the asm macros working. Copy Linux more closely. Then, you have LIVEPATCH select FUNCTION_ALIGNMENT_{8,16}B as appropriate. And PERFORMANCE selects FUNCTION_ALIGNMENT_16B or perhaps 32B depending on uarch. If we ever get around to having KCFI, then we need 16B irrespective of anything else. As for the subject, it's not really about size; the function size is still going to be small irrespective of the alignment. It's about having space in the final binary to livepatch: int foo(void) { return 0; } into int foo(void) { return 1; } which is something you can't always do right now without overwriting the head of whichever function is following foo() in the binary. ~Andrew
On Tue, Dec 05, 2023 at 01:42:42PM +0000, Andrew Cooper wrote: > On 28/11/2023 10:03 am, Roger Pau Monne wrote: > > diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile > > index f3abdf9cd111..f629157086d0 100644 > > --- a/xen/arch/x86/Makefile > > +++ b/xen/arch/x86/Makefile > > @@ -82,6 +82,8 @@ obj-$(CONFIG_COMPAT) += x86_64/platform_hypercall.o > > obj-y += sysctl.o > > endif > > > > +CFLAGS-$(CONFIG_LIVEPATCH) += -falign-functions=$(CONFIG_CC_FUNCTION_ALIGNMENT) > > I'd really prefer not to express it like this. For one, a major reason > for using an alignment of 16b or more is simply performance. > > Also, it isn't "CC" when we get the asm macros working. > > Copy Linux more closely. Then, you have LIVEPATCH select > FUNCTION_ALIGNMENT_{8,16}B as appropriate. And PERFORMANCE selects > FUNCTION_ALIGNMENT_16B or perhaps 32B depending on uarch. So just use CONFIG_FUNCTION_ALIGNMENT and drop the CC part of it? That would indeed be fine. We will also need to adjust CC_SPLIT_SECTIONS to drop the CC_ prefix when we start using it in assembly code. > If we ever get around to having KCFI, then we need 16B irrespective of > anything else. > > > > As for the subject, it's not really about size; the function size is > still going to be small irrespective of the alignment. What about wording it like: x86/livepatch: set function alignment to ensure minimal space between functions Thanks, Roger.
On 05.12.2023 16:01, Roger Pau Monné wrote: > On Tue, Dec 05, 2023 at 01:42:42PM +0000, Andrew Cooper wrote: >> On 28/11/2023 10:03 am, Roger Pau Monne wrote: >>> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile >>> index f3abdf9cd111..f629157086d0 100644 >>> --- a/xen/arch/x86/Makefile >>> +++ b/xen/arch/x86/Makefile >>> @@ -82,6 +82,8 @@ obj-$(CONFIG_COMPAT) += x86_64/platform_hypercall.o >>> obj-y += sysctl.o >>> endif >>> >>> +CFLAGS-$(CONFIG_LIVEPATCH) += -falign-functions=$(CONFIG_CC_FUNCTION_ALIGNMENT) >> >> I'd really prefer not to express it like this. For one, a major reason >> for using an alignment of 16b or more is simply performance. >> >> Also, it isn't "CC" when we get the asm macros working. >> >> Copy Linux more closely. Then, you have LIVEPATCH select >> FUNCTION_ALIGNMENT_{8,16}B as appropriate. And PERFORMANCE selects >> FUNCTION_ALIGNMENT_16B or perhaps 32B depending on uarch. > > So just use CONFIG_FUNCTION_ALIGNMENT and drop the CC part of it? > That would indeed be fine. We will also need to adjust > CC_SPLIT_SECTIONS to drop the CC_ prefix when we start using it in > assembly code. Could we prune the CC infixes once everything is settled asm-code-wise? >> If we ever get around to having KCFI, then we need 16B irrespective of >> anything else. >> >> >> >> As for the subject, it's not really about size; the function size is >> still going to be small irrespective of the alignment. > > What about wording it like: > > x86/livepatch: set function alignment to ensure minimal space between functions This still wouldn't be right, as there may be no padding at all between functions (if they're just the right size). Maybe "minimal distance between function entry points"? Getting long-ish, though ... Jan
On Tue, Dec 05, 2023 at 04:14:57PM +0100, Jan Beulich wrote: > On 05.12.2023 16:01, Roger Pau Monné wrote: > > On Tue, Dec 05, 2023 at 01:42:42PM +0000, Andrew Cooper wrote: > >> On 28/11/2023 10:03 am, Roger Pau Monne wrote: > >>> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile > >>> index f3abdf9cd111..f629157086d0 100644 > >>> --- a/xen/arch/x86/Makefile > >>> +++ b/xen/arch/x86/Makefile > >>> @@ -82,6 +82,8 @@ obj-$(CONFIG_COMPAT) += x86_64/platform_hypercall.o > >>> obj-y += sysctl.o > >>> endif > >>> > >>> +CFLAGS-$(CONFIG_LIVEPATCH) += -falign-functions=$(CONFIG_CC_FUNCTION_ALIGNMENT) > >> > >> I'd really prefer not to express it like this. For one, a major reason > >> for using an alignment of 16b or more is simply performance. > >> > >> Also, it isn't "CC" when we get the asm macros working. > >> > >> Copy Linux more closely. Then, you have LIVEPATCH select > >> FUNCTION_ALIGNMENT_{8,16}B as appropriate. And PERFORMANCE selects > >> FUNCTION_ALIGNMENT_16B or perhaps 32B depending on uarch. > > > > So just use CONFIG_FUNCTION_ALIGNMENT and drop the CC part of it? > > That would indeed be fine. We will also need to adjust > > CC_SPLIT_SECTIONS to drop the CC_ prefix when we start using it in > > assembly code. > > Could we prune the CC infixes once everything is settled asm-code-wise? That would also be fine by me. > >> If we ever get around to having KCFI, then we need 16B irrespective of > >> anything else. > >> > >> > >> > >> As for the subject, it's not really about size; the function size is > >> still going to be small irrespective of the alignment. > > > > What about wording it like: > > > > x86/livepatch: set function alignment to ensure minimal space between functions > > This still wouldn't be right, as there may be no padding at all between > functions (if they're just the right size). But no padding would still be fine given the text above, as then the minimal space requirement is already meet? > Maybe "minimal distance > between function entry points"? Getting long-ish, though ... Oh, I see. You want to explicitly mention the distance is between function entry points, as otherwise one way to read the subject would be distance between function end and next function entry point? It's indeed a bit long for my taste, but I don't mind adjusting if you think the current wording could cause confusion. Thanks, Roger.
On 05.12.2023 16:36, Roger Pau Monné wrote: > On Tue, Dec 05, 2023 at 04:14:57PM +0100, Jan Beulich wrote: >> On 05.12.2023 16:01, Roger Pau Monné wrote: >>> On Tue, Dec 05, 2023 at 01:42:42PM +0000, Andrew Cooper wrote: >>>> As for the subject, it's not really about size; the function size is >>>> still going to be small irrespective of the alignment. >>> >>> What about wording it like: >>> >>> x86/livepatch: set function alignment to ensure minimal space between functions >> >> This still wouldn't be right, as there may be no padding at all between >> functions (if they're just the right size). > > But no padding would still be fine given the text above, as then the > minimal space requirement is already meet? > >> Maybe "minimal distance >> between function entry points"? Getting long-ish, though ... > > Oh, I see. You want to explicitly mention the distance is between > function entry points, as otherwise one way to read the subject would > be distance between function end and next function entry point? Yes, I saw no other way of reading it. IOW ... > It's indeed a bit long for my taste, but I don't mind adjusting if you > think the current wording could cause confusion. ... it already did cause confusion. But maybe we can still think of shrinking the result some ... Jan
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig index 1acdffc51c22..612a4acf079b 100644 --- a/xen/arch/x86/Kconfig +++ b/xen/arch/x86/Kconfig @@ -47,6 +47,12 @@ config HAS_CC_CET_IBT # Retpoline check to work around https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93654 def_bool $(cc-option,-fcf-protection=branch -mmanual-endbr -mindirect-branch=thunk-extern) && $(as-instr,endbr64) +# Set function alignment to ensure enough padding available +config CC_FUNCTION_ALIGNMENT + int + default 16 if LIVEPATCH && XEN_IBT + default 8 if LIVEPATCH + menu "Architecture Features" source "arch/Kconfig" diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile index f3abdf9cd111..f629157086d0 100644 --- a/xen/arch/x86/Makefile +++ b/xen/arch/x86/Makefile @@ -82,6 +82,8 @@ obj-$(CONFIG_COMPAT) += x86_64/platform_hypercall.o obj-y += sysctl.o endif +CFLAGS-$(CONFIG_LIVEPATCH) += -falign-functions=$(CONFIG_CC_FUNCTION_ALIGNMENT) + extra-y += asm-macros.i extra-y += xen.lds diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c index ee539f001b73..4a6ba09e0ec5 100644 --- a/xen/arch/x86/livepatch.c +++ b/xen/arch/x86/livepatch.c @@ -109,6 +109,10 @@ int arch_livepatch_verify_func(const struct livepatch_func *func) */ uint8_t needed = ARCH_PATCH_INSN_SIZE; + BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE + + (IS_ENABLED(CONIFG_XEN_IBT) ? ENDBR64_LEN : 0) > + CONFIG_CC_FUNCTION_ALIGNMENT); + if ( is_endbr64(func->old_addr) || is_endbr64_poison(func->old_addr) ) needed += ENDBR64_LEN;
The minimal function size requirements for livepatch are either 5 bytes (for jmp) or 9 bytes (for endbr + jmp). Ensure that functions are always at least that size by requesting the compiled to align the functions to 8 or 16 bytes, depending on whether Xen is build with IBT support. Note that it's possible for the compiler to end up using a higher function alignment regardless of the passed value, so this change just make sure that the minimum required for livepatch to work is present. Since the option (-falign-functions) is supported by both minimal required compiler versions of clang and gcc there's no need to add a test to check for its presence. The alignment is currently only implemented for livepatch on x86, I'm unsure whether ARM has a mandatory function alignment high enough to cover for the space required by the replacement instruction(s). Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Changes since v1: - New in this version. --- xen/arch/x86/Kconfig | 6 ++++++ xen/arch/x86/Makefile | 2 ++ xen/arch/x86/livepatch.c | 4 ++++ 3 files changed, 12 insertions(+)