Re: [Xen-devel] [PATCH v4 07/16] livepatch/arm/x86: Check payload for for unwelcomed symbols.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.09.16 at 19:32, <konrad.wilk@xxxxxxxxxx> wrote:
> On Mon, Sep 19, 2016 at 08:48:10AM -0600, Jan Beulich wrote:
>> >>> On 19.09.16 at 16:13, <julien.grall@xxxxxxx> wrote:
>> 
>> > 
>> > On 19/09/2016 16:11, Jan Beulich wrote:
>> >>>>> On 19.09.16 at 15:33, <julien.grall@xxxxxxx> wrote:
>> >>> On 19/09/2016 11:27, Jan Beulich wrote:
>> >>>>>>> On 16.09.16 at 18:38, <konrad.wilk@xxxxxxxxxx> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel