Re: [Xen-devel] [PATCH v4 07/16] livepatch/arm/x86: Check payload for for unwelcomed symbols.
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, wrote:
On 19/09/2016 16:11, Jan Beulich wrote:
On 19.09.16 at 15:33, wrote:
On 19/09/2016 11:27, Jan Beulich wrote:
On 16.09.16 at 18:38, 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,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 07/16] livepatch/arm/x86: Check payload for for unwelcomed symbols.
>>> On 19.09.16 at 19:32, wrote:
> On Mon, Sep 19, 2016 at 08:48:10AM -0600, Jan Beulich wrote:
>> >>> On 19.09.16 at 16:13, wrote:
>>
>> >
>> > On 19/09/2016 16:11, Jan Beulich wrote:
>> > On 19.09.16 at 15:33, wrote:
>> >>> On 19/09/2016 11:27, Jan Beulich wrote:
>> >>> On 16.09.16 at 18:38, 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@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 07/16] livepatch/arm/x86: Check payload for for unwelcomed symbols.
On Mon, Sep 19, 2016 at 08:48:10AM -0600, Jan Beulich wrote:
> >>> On 19.09.16 at 16:13, wrote:
>
> >
> > On 19/09/2016 16:11, Jan Beulich wrote:
> > On 19.09.16 at 15:33, wrote:
> >>> On 19/09/2016 11:27, Jan Beulich wrote:
> >>> On 16.09.16 at 18:38, 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@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 07/16] livepatch/arm/x86: Check payload for for unwelcomed symbols.
>>> On 19.09.16 at 16:13, wrote:
>
> On 19/09/2016 16:11, Jan Beulich wrote:
> On 19.09.16 at 15:33, wrote:
>>> On 19/09/2016 11:27, Jan Beulich wrote:
>>> On 16.09.16 at 18:38, 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
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 07/16] livepatch/arm/x86: Check payload for for unwelcomed symbols.
On 19/09/2016 16:11, Jan Beulich wrote:
On 19.09.16 at 15:33, wrote:
On 19/09/2016 11:27, Jan Beulich wrote:
On 16.09.16 at 18:38, 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,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 07/16] livepatch/arm/x86: Check payload for for unwelcomed symbols.
>>> On 19.09.16 at 15:33, wrote:
> Hi,
>
> On 19/09/2016 11:27, Jan Beulich wrote:
> On 16.09.16 at 18:38, 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
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 07/16] livepatch/arm/x86: Check payload for for unwelcomed symbols.
Hi,
On 19/09/2016 11:27, Jan Beulich wrote:
On 16.09.16 at 18:38, 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,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4 07/16] livepatch/arm/x86: Check payload for for unwelcomed symbols.
>>> On 16.09.16 at 18:38, 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
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
