| Message ID | 1474043908-12101-8-git-send-email-konrad.wilk@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 16.09.16 at 18:38, <konrad.wilk@oracle.com> wrote: > --- a/xen/arch/arm/livepatch.c > +++ b/xen/arch/arm/livepatch.c > @@ -117,6 +117,20 @@ bool arch_livepatch_symbol_ok(const struct livepatch_elf *elf, > return true; > } > > +bool arch_livepatch_symbol_deny(const struct livepatch_elf *elf, > + const struct livepatch_elf_sym *sym) > +{ > +#ifdef CONFIG_ARM_32 > + /* > + * Xen does not use Thumb instructions - and we should not see any of > + * them. If we do, abort. > + */ > + if ( sym->name && *sym->name == '$' && sym->name[1] == 't' ) I'm not sure here: Are all symbols starting with $t to be rejected, or only $t but not e.g. $txyz? According to some other code I have lying around it ought to be "$t" and any symbols starting with "$t.", and would be in line with patch 6. But I guess the ARM maintainers will know best. Jan
Hi, On 19/09/2016 11:27, Jan Beulich wrote: >>>> On 16.09.16 at 18:38, <konrad.wilk@oracle.com> wrote: >> --- a/xen/arch/arm/livepatch.c >> +++ b/xen/arch/arm/livepatch.c >> @@ -117,6 +117,20 @@ bool arch_livepatch_symbol_ok(const struct livepatch_elf *elf, >> return true; >> } >> >> +bool arch_livepatch_symbol_deny(const struct livepatch_elf *elf, >> + const struct livepatch_elf_sym *sym) >> +{ >> +#ifdef CONFIG_ARM_32 >> + /* >> + * Xen does not use Thumb instructions - and we should not see any of >> + * them. If we do, abort. >> + */ >> + if ( sym->name && *sym->name == '$' && sym->name[1] == 't' ) Please use sym->name[0] for readability. Also, you may want to check the length of the symbol before checking the second character. > > I'm not sure here: Are all symbols starting with $t to be rejected, or > only $t but not e.g. $txyz? According to some other code I have > lying around it ought to be "$t" and any symbols starting with "$t.", > and would be in line with patch 6. But I guess the ARM maintainers > will know best. Only $t and $t.* should be rejected. All the others may be valid in the future. Looking at the spec, I am wondering if we should also check the type and binding of the symbols. I have the impression that the naming is specific to STT_NOTYPE and STB_LOCAL. Any opinions? Similar question for the previous patch (#6). Regards,
>>> On 19.09.16 at 15:33, <julien.grall@arm.com> wrote: > Hi, > > On 19/09/2016 11:27, Jan Beulich wrote: >>>>> On 16.09.16 at 18:38, <konrad.wilk@oracle.com> wrote: >>> --- a/xen/arch/arm/livepatch.c >>> +++ b/xen/arch/arm/livepatch.c >>> @@ -117,6 +117,20 @@ bool arch_livepatch_symbol_ok(const struct livepatch_elf > *elf, >>> return true; >>> } >>> >>> +bool arch_livepatch_symbol_deny(const struct livepatch_elf *elf, >>> + const struct livepatch_elf_sym *sym) >>> +{ >>> +#ifdef CONFIG_ARM_32 >>> + /* >>> + * Xen does not use Thumb instructions - and we should not see any of >>> + * them. If we do, abort. >>> + */ >>> + if ( sym->name && *sym->name == '$' && sym->name[1] == 't' ) > > Please use sym->name[0] for readability. Also, you may want to check the > length of the symbol before checking the second character. Why would the length check be needed? If the first character is $, then looking at the second one is always valid (and it being nul will be properly dealt with by the expression above). Jan
On 19/09/2016 16:11, Jan Beulich wrote: >>>> On 19.09.16 at 15:33, <julien.grall@arm.com> wrote: >> On 19/09/2016 11:27, Jan Beulich wrote: >>>>>> On 16.09.16 at 18:38, <konrad.wilk@oracle.com> wrote: >>>> --- a/xen/arch/arm/livepatch.c >>>> +++ b/xen/arch/arm/livepatch.c >>>> @@ -117,6 +117,20 @@ bool arch_livepatch_symbol_ok(const struct livepatch_elf >> *elf, >>>> return true; >>>> } >>>> >>>> +bool arch_livepatch_symbol_deny(const struct livepatch_elf *elf, >>>> + const struct livepatch_elf_sym *sym) >>>> +{ >>>> +#ifdef CONFIG_ARM_32 >>>> + /* >>>> + * Xen does not use Thumb instructions - and we should not see any of >>>> + * them. If we do, abort. >>>> + */ >>>> + if ( sym->name && *sym->name == '$' && sym->name[1] == 't' ) >> >> Please use sym->name[0] for readability. Also, you may want to check the >> length of the symbol before checking the second character. > > Why would the length check be needed? If the first character is $, > then looking at the second one is always valid (and it being nul will > be properly dealt with by the expression above). Because you may have a payload which is not valid? Or maybe you consider that all the payload are trusted. Regards,
>>> On 19.09.16 at 16:13, <julien.grall@arm.com> wrote: > > On 19/09/2016 16:11, Jan Beulich wrote: >>>>> On 19.09.16 at 15:33, <julien.grall@arm.com> wrote: >>> On 19/09/2016 11:27, Jan Beulich wrote: >>>>>>> On 16.09.16 at 18:38, <konrad.wilk@oracle.com> wrote: >>>>> --- a/xen/arch/arm/livepatch.c >>>>> +++ b/xen/arch/arm/livepatch.c >>>>> @@ -117,6 +117,20 @@ bool arch_livepatch_symbol_ok(const struct livepatch_elf >>> *elf, >>>>> return true; >>>>> } >>>>> >>>>> +bool arch_livepatch_symbol_deny(const struct livepatch_elf *elf, >>>>> + const struct livepatch_elf_sym *sym) >>>>> +{ >>>>> +#ifdef CONFIG_ARM_32 >>>>> + /* >>>>> + * Xen does not use Thumb instructions - and we should not see any of >>>>> + * them. If we do, abort. >>>>> + */ >>>>> + if ( sym->name && *sym->name == '$' && sym->name[1] == 't' ) >>> >>> Please use sym->name[0] for readability. Also, you may want to check the >>> length of the symbol before checking the second character. >> >> Why would the length check be needed? If the first character is $, >> then looking at the second one is always valid (and it being nul will >> be properly dealt with by the expression above). > > Because you may have a payload which is not valid? Or maybe you consider > that all the payload are trusted. If all symbols' names are inside their string tables and the string tables are both contained inside the image and have a zero byte at their end (all of which gets verified afair), nothing bad can happen I think. Jan
On Mon, Sep 19, 2016 at 08:48:10AM -0600, Jan Beulich wrote: > >>> On 19.09.16 at 16:13, <julien.grall@arm.com> wrote: > > > > > On 19/09/2016 16:11, Jan Beulich wrote: > >>>>> On 19.09.16 at 15:33, <julien.grall@arm.com> wrote: > >>> On 19/09/2016 11:27, Jan Beulich wrote: > >>>>>>> On 16.09.16 at 18:38, <konrad.wilk@oracle.com> wrote: > >>>>> --- a/xen/arch/arm/livepatch.c > >>>>> +++ b/xen/arch/arm/livepatch.c > >>>>> @@ -117,6 +117,20 @@ bool arch_livepatch_symbol_ok(const struct livepatch_elf > >>> *elf, > >>>>> return true; > >>>>> } > >>>>> > >>>>> +bool arch_livepatch_symbol_deny(const struct livepatch_elf *elf, > >>>>> + const struct livepatch_elf_sym *sym) > >>>>> +{ > >>>>> +#ifdef CONFIG_ARM_32 > >>>>> + /* > >>>>> + * Xen does not use Thumb instructions - and we should not see any of > >>>>> + * them. If we do, abort. > >>>>> + */ > >>>>> + if ( sym->name && *sym->name == '$' && sym->name[1] == 't' ) > >>> > >>> Please use sym->name[0] for readability. Also, you may want to check the > >>> length of the symbol before checking the second character. > >> > >> Why would the length check be needed? If the first character is $, > >> then looking at the second one is always valid (and it being nul will > >> be properly dealt with by the expression above). > > > > Because you may have a payload which is not valid? Or maybe you consider > > that all the payload are trusted. > > If all symbols' names are inside their string tables and the string > tables are both contained inside the image and have a zero byte > at their end (all of which gets verified afair), nothing bad can > happen I think. Exactly. All of those checks are done so we are sure that the sym->name[0] points to something. Julien, I can use strlen if you prefer, so it would be like so: bool arch_livepatch_symbol_deny(const struct livepatch_elf *elf, const struct livepatch_elf_sym *sym) { #ifdef CONFIG_ARM_32 /* * Xen does not use Thumb instructions - and we should not see any of * them. If we do, abort. */ if ( sym-name && sym->name[0] == '$' && sym->name[1] == 't' ) { size_t len = strlen(sym->name); if ( (len >= 3 && (sym->name[2] == '.')) || len == 2 ) return true; } #endif return false; } Or this way: bool arch_livepatch_symbol_deny(const struct livepatch_elf *elf, const struct livepatch_elf_sym *sym) { #ifdef CONFIG_ARM_32 /* * Xen does not use Thumb instructions - and we should not see any * of * them. If we do, abort. */ if ( sym->name && sym->name[0] == '$' && sym->name[1] == 't' ) { if ( sym->name[2] && sym->name[2] != '.' ) return false; return true; } #endif return false; } Both add exactly the same amount of lines of code :-) > > Jan >
>>> On 19.09.16 at 19:32, <konrad.wilk@oracle.com> wrote: > On Mon, Sep 19, 2016 at 08:48:10AM -0600, Jan Beulich wrote: >> >>> On 19.09.16 at 16:13, <julien.grall@arm.com> wrote: >> >> > >> > On 19/09/2016 16:11, Jan Beulich wrote: >> >>>>> On 19.09.16 at 15:33, <julien.grall@arm.com> wrote: >> >>> On 19/09/2016 11:27, Jan Beulich wrote: >> >>>>>>> On 16.09.16 at 18:38, <konrad.wilk@oracle.com> wrote: >> >>>>> --- a/xen/arch/arm/livepatch.c >> >>>>> +++ b/xen/arch/arm/livepatch.c >> >>>>> @@ -117,6 +117,20 @@ bool arch_livepatch_symbol_ok(const struct > livepatch_elf >> >>> *elf, >> >>>>> return true; >> >>>>> } >> >>>>> >> >>>>> +bool arch_livepatch_symbol_deny(const struct livepatch_elf *elf, >> >>>>> + const struct livepatch_elf_sym *sym) >> >>>>> +{ >> >>>>> +#ifdef CONFIG_ARM_32 >> >>>>> + /* >> >>>>> + * Xen does not use Thumb instructions - and we should not see any of >> >>>>> + * them. If we do, abort. >> >>>>> + */ >> >>>>> + if ( sym->name && *sym->name == '$' && sym->name[1] == 't' ) >> >>> >> >>> Please use sym->name[0] for readability. Also, you may want to check the >> >>> length of the symbol before checking the second character. >> >> >> >> Why would the length check be needed? If the first character is $, >> >> then looking at the second one is always valid (and it being nul will >> >> be properly dealt with by the expression above). >> > >> > Because you may have a payload which is not valid? Or maybe you consider >> > that all the payload are trusted. >> >> If all symbols' names are inside their string tables and the string >> tables are both contained inside the image and have a zero byte >> at their end (all of which gets verified afair), nothing bad can >> happen I think. > > Exactly. All of those checks are done so we are sure that the > sym->name[0] points to something. > > > Julien, I can use strlen if you prefer, so it would be like so: If I came across this code, I'd be tempted to immediately submit a patch to rip out that strlen(), so if you ask me - please don't. Jan
Hi Konrad, On 19/09/2016 19:32, Konrad Rzeszutek Wilk wrote: > On Mon, Sep 19, 2016 at 08:48:10AM -0600, Jan Beulich wrote: >>>>> On 19.09.16 at 16:13, <julien.grall@arm.com> wrote: >> >>> >>> On 19/09/2016 16:11, Jan Beulich wrote: >>>>>>> On 19.09.16 at 15:33, <julien.grall@arm.com> wrote: >>>>> On 19/09/2016 11:27, Jan Beulich wrote: >>>>>>>>> On 16.09.16 at 18:38, <konrad.wilk@oracle.com> wrote: >>>>>>> --- a/xen/arch/arm/livepatch.c >>>>>>> +++ b/xen/arch/arm/livepatch.c >>>>>>> @@ -117,6 +117,20 @@ bool arch_livepatch_symbol_ok(const struct livepatch_elf >>>>> *elf, >>>>>>> return true; >>>>>>> } >>>>>>> >>>>>>> +bool arch_livepatch_symbol_deny(const struct livepatch_elf *elf, >>>>>>> + const struct livepatch_elf_sym *sym) >>>>>>> +{ >>>>>>> +#ifdef CONFIG_ARM_32 >>>>>>> + /* >>>>>>> + * Xen does not use Thumb instructions - and we should not see any of >>>>>>> + * them. If we do, abort. >>>>>>> + */ >>>>>>> + if ( sym->name && *sym->name == '$' && sym->name[1] == 't' ) >>>>> >>>>> Please use sym->name[0] for readability. Also, you may want to check the >>>>> length of the symbol before checking the second character. >>>> >>>> Why would the length check be needed? If the first character is $, >>>> then looking at the second one is always valid (and it being nul will >>>> be properly dealt with by the expression above). >>> >>> Because you may have a payload which is not valid? Or maybe you consider >>> that all the payload are trusted. >> >> If all symbols' names are inside their string tables and the string >> tables are both contained inside the image and have a zero byte >> at their end (all of which gets verified afair), nothing bad can >> happen I think. > > Exactly. All of those checks are done so we are sure that the > sym->name[0] points to something. > Hmmm right, sorry for the confusion. > bool arch_livepatch_symbol_deny(const struct livepatch_elf *elf, > const struct livepatch_elf_sym *sym) > { > #ifdef CONFIG_ARM_32 > /* > * Xen does not use Thumb instructions - and we should not see any > * of > * them. If we do, abort. > */ > if ( sym->name && sym->name[0] == '$' && sym->name[1] == 't' ) > { > if ( sym->name[2] && sym->name[2] != '.' ) > return false; > > return true; > } > #endif > return false; > } The second one is fine by me with a small change: if ( sym->name[2] && sym->name[2] != '.' ) return false; return true; Could be replaced by: return ( !sym->name[2] || sym->name[2] == '.' ); Regards,
diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c index a87d48c..13d6c10 100644 --- a/xen/arch/arm/livepatch.c +++ b/xen/arch/arm/livepatch.c @@ -117,6 +117,20 @@ bool arch_livepatch_symbol_ok(const struct livepatch_elf *elf, return true; } +bool arch_livepatch_symbol_deny(const struct livepatch_elf *elf, + const struct livepatch_elf_sym *sym) +{ +#ifdef CONFIG_ARM_32 + /* + * Xen does not use Thumb instructions - and we should not see any of + * them. If we do, abort. + */ + if ( sym->name && *sym->name == '$' && sym->name[1] == 't' ) + return true; +#endif + return false; +} + int arch_livepatch_perform_rel(struct livepatch_elf *elf, const struct livepatch_elf_sec *base, const struct livepatch_elf_sec *rela) diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c index efc487a..79be833 100644 --- a/xen/arch/x86/livepatch.c +++ b/xen/arch/x86/livepatch.c @@ -125,6 +125,13 @@ bool arch_livepatch_symbol_ok(const struct livepatch_elf *elf, return true; } +bool arch_livepatch_symbol_deny(const struct livepatch_elf *elf, + const struct livepatch_elf_sym *sym) +{ + /* No special checks on x86. */ + return false; +} + int arch_livepatch_perform_rel(struct livepatch_elf *elf, const struct livepatch_elf_sec *base, const struct livepatch_elf_sec *rela) diff --git a/xen/common/livepatch_elf.c b/xen/common/livepatch_elf.c index 79c290e..a4f6794 100644 --- a/xen/common/livepatch_elf.c +++ b/xen/common/livepatch_elf.c @@ -251,6 +251,13 @@ static int elf_get_sym(struct livepatch_elf *elf, const void *data) sym[i].sym = s; sym[i].name = strtab_sec->data + delta; + /* e.g. On ARM we should NEVER see $t* symbols. */ + if ( arch_livepatch_symbol_deny(elf, &sym[i]) ) + { + dprintk(XENLOG_ERR, LIVEPATCH "%s: Symbol '%s' should not be in payload!\n", + elf->name, sym[i].name); + return -EINVAL; + } } elf->nsym = nsym; diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h index 03abc5b..d107a75 100644 --- a/xen/include/xen/livepatch.h +++ b/xen/include/xen/livepatch.h @@ -50,6 +50,8 @@ bool_t is_patch(const void *addr); int arch_livepatch_verify_elf(const struct livepatch_elf *elf); bool arch_livepatch_symbol_ok(const struct livepatch_elf *elf, const struct livepatch_elf_sym *sym); +bool arch_livepatch_symbol_deny(const struct livepatch_elf *elf, + const struct livepatch_elf_sym *sym); int arch_livepatch_perform_rel(struct livepatch_elf *elf, const struct livepatch_elf_sec *base, const struct livepatch_elf_sec *rela);
Certain platforms, such as ARM [32|64] add extra mapping symbols such as $x (for ARM64 instructions), or more interesting to this patch: $t (for Thumb instructions). These symbols are suppose to help the final linker to make any adjustments (such as add an veneer). But more importantly - we do not compile Xen with any Thumb instructions (which are variable length) - and if we find these mapping symbols we should disallow such payload. Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Cc: Ross Lagerwall <ross.lagerwall@citrix.com> Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Julien Grall <julien.grall@arm.com Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> v3: New submission. Use &sym[i] instead of sym (as that will always be NULL). v4: Use bool instead of int for return Update comment in common code about ARM odd symbols. s/_check/_deny/ to make it more clear. --- xen/arch/arm/livepatch.c | 14 ++++++++++++++ xen/arch/x86/livepatch.c | 7 +++++++ xen/common/livepatch_elf.c | 7 +++++++ xen/include/xen/livepatch.h | 2 ++ 4 files changed, 30 insertions(+)