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

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

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:
                                                                               
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
> 

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