* [PATCH v2] vsprintf: Make sure argument to %pX specifier is valid
@ 2015-02-11 20:58 Boris Ostrovsky
2015-02-12 11:04 ` Andrew Cooper
0 siblings, 1 reply; 9+ messages in thread
From: Boris Ostrovsky @ 2015-02-11 20:58 UTC (permalink / raw
To: ian.campbell, ian.jackson, jbeulich, keir, tim; +Cc: boris.ostrovsky, xen-devel
If invalid pointer (i.e. something smaller than HYPERVISOR_VIRT_START)
is passed for %*ph/%pv/%ps/%pS format specifiers then print "(NULL)"
Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
xen/common/vsprintf.c | 23 ++++++++++++++++-------
1 files changed, 16 insertions(+), 7 deletions(-)
v2:
* Print "(NULL)" instead of specifier-specific string
* Consider all addresses under HYPERVISOR_VIRT_START as invalid. (I think
this is true for both x86 and ARM but I don't have ARM platform to test).
diff --git a/xen/common/vsprintf.c b/xen/common/vsprintf.c
index 065cc42..b9542b5 100644
--- a/xen/common/vsprintf.c
+++ b/xen/common/vsprintf.c
@@ -270,6 +270,22 @@ static char *pointer(char *str, char *end, const char **fmt_ptr,
const char *fmt = *fmt_ptr, *s;
/* Custom %p suffixes. See XEN_ROOT/docs/misc/printk-formats.txt */
+
+ switch ( fmt[1] )
+ {
+ case 'h':
+ case 's':
+ case 'S':
+ case 'v':
+ ++*fmt_ptr;
+ }
+
+ if ( (unsigned long)arg < HYPERVISOR_VIRT_START )
+ {
+ char *s = "(NULL)";
+ return string(str, end, s, -1, -1, 0);
+ }
+
switch ( fmt[1] )
{
case 'h': /* Raw buffer as hex string. */
@@ -277,9 +293,6 @@ static char *pointer(char *str, char *end, const char **fmt_ptr,
const uint8_t *hex_buffer = arg;
unsigned int i;
- /* Consumed 'h' from the format string. */
- ++*fmt_ptr;
-
/* Bound user count from %* to between 0 and 64 bytes. */
if ( field_width <= 0 )
return str;
@@ -306,9 +319,6 @@ static char *pointer(char *str, char *end, const char **fmt_ptr,
unsigned long sym_size, sym_offset;
char namebuf[KSYM_NAME_LEN+1];
- /* Advance parents fmt string, as we have consumed 's' or 'S' */
- ++*fmt_ptr;
-
s = symbols_lookup((unsigned long)arg, &sym_size, &sym_offset, namebuf);
/* If the symbol is not found, fall back to printing the address */
@@ -335,7 +345,6 @@ static char *pointer(char *str, char *end, const char **fmt_ptr,
{
const struct vcpu *v = arg;
- ++*fmt_ptr;
if ( str < end )
*str = 'd';
str = number(str + 1, end, v->domain->domain_id, 10, -1, -1, 0);
--
1.7.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] vsprintf: Make sure argument to %pX specifier is valid
2015-02-11 20:58 [PATCH v2] vsprintf: Make sure argument to %pX specifier is valid Boris Ostrovsky
@ 2015-02-12 11:04 ` Andrew Cooper
2015-02-12 15:01 ` Boris Ostrovsky
0 siblings, 1 reply; 9+ messages in thread
From: Andrew Cooper @ 2015-02-12 11:04 UTC (permalink / raw
To: Boris Ostrovsky, ian.campbell, ian.jackson, jbeulich, keir, tim; +Cc: xen-devel
On 11/02/15 20:58, Boris Ostrovsky wrote:
> If invalid pointer (i.e. something smaller than HYPERVISOR_VIRT_START)
> is passed for %*ph/%pv/%ps/%pS format specifiers then print "(NULL)"
>
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> ---
> xen/common/vsprintf.c | 23 ++++++++++++++++-------
> 1 files changed, 16 insertions(+), 7 deletions(-)
>
> v2:
> * Print "(NULL)" instead of specifier-specific string
> * Consider all addresses under HYPERVISOR_VIRT_START as invalid. (I think
> this is true for both x86 and ARM but I don't have ARM platform to test).
>
>
> diff --git a/xen/common/vsprintf.c b/xen/common/vsprintf.c
> index 065cc42..b9542b5 100644
> --- a/xen/common/vsprintf.c
> +++ b/xen/common/vsprintf.c
> @@ -270,6 +270,22 @@ static char *pointer(char *str, char *end, const char **fmt_ptr,
> const char *fmt = *fmt_ptr, *s;
>
> /* Custom %p suffixes. See XEN_ROOT/docs/misc/printk-formats.txt */
> +
> + switch ( fmt[1] )
> + {
> + case 'h':
> + case 's':
> + case 'S':
> + case 'v':
> + ++*fmt_ptr;
> + }
> +
> + if ( (unsigned long)arg < HYPERVISOR_VIRT_START )
> + {
> + char *s = "(NULL)";
> + return string(str, end, s, -1, -1, 0);
> + }
> +
> switch ( fmt[1] )
This wont function, as you have inverted the increment of *fmt_ptr and
check of fmt[1].
"(NULL)" is inappropriate for non-null pointers less than VIRT_START.
Given the VIRT check, I would just put the entire switch statement
inside an "if ( (unsigned long)arg < HYPERVISOR_VIRT_START )" block and
let it fall through to the plain number case for a bogus pointer.
~Andrew
> {
> case 'h': /* Raw buffer as hex string. */
> @@ -277,9 +293,6 @@ static char *pointer(char *str, char *end, const char **fmt_ptr,
> const uint8_t *hex_buffer = arg;
> unsigned int i;
>
> - /* Consumed 'h' from the format string. */
> - ++*fmt_ptr;
> -
> /* Bound user count from %* to between 0 and 64 bytes. */
> if ( field_width <= 0 )
> return str;
> @@ -306,9 +319,6 @@ static char *pointer(char *str, char *end, const char **fmt_ptr,
> unsigned long sym_size, sym_offset;
> char namebuf[KSYM_NAME_LEN+1];
>
> - /* Advance parents fmt string, as we have consumed 's' or 'S' */
> - ++*fmt_ptr;
> -
> s = symbols_lookup((unsigned long)arg, &sym_size, &sym_offset, namebuf);
>
> /* If the symbol is not found, fall back to printing the address */
> @@ -335,7 +345,6 @@ static char *pointer(char *str, char *end, const char **fmt_ptr,
> {
> const struct vcpu *v = arg;
>
> - ++*fmt_ptr;
> if ( str < end )
> *str = 'd';
> str = number(str + 1, end, v->domain->domain_id, 10, -1, -1, 0);
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] vsprintf: Make sure argument to %pX specifier is valid
2015-02-12 11:04 ` Andrew Cooper
@ 2015-02-12 15:01 ` Boris Ostrovsky
2015-02-12 15:21 ` Andrew Cooper
0 siblings, 1 reply; 9+ messages in thread
From: Boris Ostrovsky @ 2015-02-12 15:01 UTC (permalink / raw
To: Andrew Cooper, ian.campbell, ian.jackson, jbeulich, keir, tim; +Cc: xen-devel
On 02/12/2015 06:04 AM, Andrew Cooper wrote:
> On 11/02/15 20:58, Boris Ostrovsky wrote:
>> If invalid pointer (i.e. something smaller than HYPERVISOR_VIRT_START)
>> is passed for %*ph/%pv/%ps/%pS format specifiers then print "(NULL)"
>>
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> ---
>> xen/common/vsprintf.c | 23 ++++++++++++++++-------
>> 1 files changed, 16 insertions(+), 7 deletions(-)
>>
>> v2:
>> * Print "(NULL)" instead of specifier-specific string
>> * Consider all addresses under HYPERVISOR_VIRT_START as invalid. (I think
>> this is true for both x86 and ARM but I don't have ARM platform to test).
>>
>>
>> diff --git a/xen/common/vsprintf.c b/xen/common/vsprintf.c
>> index 065cc42..b9542b5 100644
>> --- a/xen/common/vsprintf.c
>> +++ b/xen/common/vsprintf.c
>> @@ -270,6 +270,22 @@ static char *pointer(char *str, char *end, const char **fmt_ptr,
>> const char *fmt = *fmt_ptr, *s;
>>
>> /* Custom %p suffixes. See XEN_ROOT/docs/misc/printk-formats.txt */
>> +
>> + switch ( fmt[1] )
>> + {
>> + case 'h':
>> + case 's':
>> + case 'S':
>> + case 'v':
>> + ++*fmt_ptr;
>> + }
>> +
>> + if ( (unsigned long)arg < HYPERVISOR_VIRT_START )
>> + {
>> + char *s = "(NULL)";
>> + return string(str, end, s, -1, -1, 0);
>> + }
>> +
>> switch ( fmt[1] )
> This wont function, as you have inverted the increment of *fmt_ptr and
> check of fmt[1].
fmt value doesn't change, it is stashed at the top of the routine.
(What *is* wrong in the above code is the fact that the arg test is done
outside the switch. It should be part of the four case statements,
otherwise we will print plain %p arguments as "NULL").
>
> "(NULL)" is inappropriate for non-null pointers less than VIRT_START.
Yes, I thought about it after I sent it. "(invalid)"?
>
> Given the VIRT check, I would just put the entire switch statement
> inside an "if ( (unsigned long)arg < HYPERVISOR_VIRT_START )" block and
> let it fall through to the plain number case for a bogus pointer.
Not sure I understand what you are suggesting here, sorry.
-boris
>
> ~Andrew
>
>> {
>> case 'h': /* Raw buffer as hex string. */
>> @@ -277,9 +293,6 @@ static char *pointer(char *str, char *end, const char **fmt_ptr,
>> const uint8_t *hex_buffer = arg;
>> unsigned int i;
>>
>> - /* Consumed 'h' from the format string. */
>> - ++*fmt_ptr;
>> -
>> /* Bound user count from %* to between 0 and 64 bytes. */
>> if ( field_width <= 0 )
>> return str;
>> @@ -306,9 +319,6 @@ static char *pointer(char *str, char *end, const char **fmt_ptr,
>> unsigned long sym_size, sym_offset;
>> char namebuf[KSYM_NAME_LEN+1];
>>
>> - /* Advance parents fmt string, as we have consumed 's' or 'S' */
>> - ++*fmt_ptr;
>> -
>> s = symbols_lookup((unsigned long)arg, &sym_size, &sym_offset, namebuf);
>>
>> /* If the symbol is not found, fall back to printing the address */
>> @@ -335,7 +345,6 @@ static char *pointer(char *str, char *end, const char **fmt_ptr,
>> {
>> const struct vcpu *v = arg;
>>
>> - ++*fmt_ptr;
>> if ( str < end )
>> *str = 'd';
>> str = number(str + 1, end, v->domain->domain_id, 10, -1, -1, 0);
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] vsprintf: Make sure argument to %pX specifier is valid
2015-02-12 15:01 ` Boris Ostrovsky
@ 2015-02-12 15:21 ` Andrew Cooper
2015-02-12 15:38 ` Boris Ostrovsky
0 siblings, 1 reply; 9+ messages in thread
From: Andrew Cooper @ 2015-02-12 15:21 UTC (permalink / raw
To: Boris Ostrovsky, ian.campbell, ian.jackson, jbeulich, keir, tim; +Cc: xen-devel
On 12/02/15 15:01, Boris Ostrovsky wrote:
> On 02/12/2015 06:04 AM, Andrew Cooper wrote:
>> On 11/02/15 20:58, Boris Ostrovsky wrote:
>>> If invalid pointer (i.e. something smaller than HYPERVISOR_VIRT_START)
>>> is passed for %*ph/%pv/%ps/%pS format specifiers then print "(NULL)"
>>>
>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>> ---
>>> xen/common/vsprintf.c | 23 ++++++++++++++++-------
>>> 1 files changed, 16 insertions(+), 7 deletions(-)
>>>
>>> v2:
>>> * Print "(NULL)" instead of specifier-specific string
>>> * Consider all addresses under HYPERVISOR_VIRT_START as invalid.
>>> (I think
>>> this is true for both x86 and ARM but I don't have ARM platform
>>> to test).
>>>
>>>
>>> diff --git a/xen/common/vsprintf.c b/xen/common/vsprintf.c
>>> index 065cc42..b9542b5 100644
>>> --- a/xen/common/vsprintf.c
>>> +++ b/xen/common/vsprintf.c
>>> @@ -270,6 +270,22 @@ static char *pointer(char *str, char *end,
>>> const char **fmt_ptr,
>>> const char *fmt = *fmt_ptr, *s;
>>> /* Custom %p suffixes. See
>>> XEN_ROOT/docs/misc/printk-formats.txt */
>>> +
>>> + switch ( fmt[1] )
>>> + {
>>> + case 'h':
>>> + case 's':
>>> + case 'S':
>>> + case 'v':
>>> + ++*fmt_ptr;
>>> + }
>>> +
>>> + if ( (unsigned long)arg < HYPERVISOR_VIRT_START )
>>> + {
>>> + char *s = "(NULL)";
>>> + return string(str, end, s, -1, -1, 0);
>>> + }
>>> +
>>> switch ( fmt[1] )
>> This wont function, as you have inverted the increment of *fmt_ptr and
>> check of fmt[1].
>
> fmt value doesn't change, it is stashed at the top of the routine.
You are correct. My apologies. I however dislike the splitting of the
switch into two.
>
> (What *is* wrong in the above code is the fact that the arg test is
> done outside the switch. It should be part of the four case
> statements, otherwise we will print plain %p arguments as "NULL").
>
>
>>
>> "(NULL)" is inappropriate for non-null pointers less than VIRT_START.
>
> Yes, I thought about it after I sent it. "(invalid)"?
Better, but overriding the number with a string does hide information.
In the case that the pointer is invalid, it would be useful to see its
contents.
>
>>
>> Given the VIRT check, I would just put the entire switch statement
>> inside an "if ( (unsigned long)arg < HYPERVISOR_VIRT_START )" block and
>> let it fall through to the plain number case for a bogus pointer.
>
> Not sure I understand what you are suggesting here, sorry.
>
> -boris
if ( (unsigned long)arg < HYPERVISOR_VIRT_START )
{
switch ( fmt[1] )
{
....
}
}
This makes the patch a whole 3 line addition and indenting the whole
switch block by 4 spaces.
~Andrew
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] vsprintf: Make sure argument to %pX specifier is valid
2015-02-12 15:21 ` Andrew Cooper
@ 2015-02-12 15:38 ` Boris Ostrovsky
2015-02-12 15:48 ` Andrew Cooper
0 siblings, 1 reply; 9+ messages in thread
From: Boris Ostrovsky @ 2015-02-12 15:38 UTC (permalink / raw
To: Andrew Cooper, ian.campbell, ian.jackson, jbeulich, keir, tim; +Cc: xen-devel
On 02/12/2015 10:21 AM, Andrew Cooper wrote:
> On 12/02/15 15:01, Boris Ostrovsky wrote:
>> On 02/12/2015 06:04 AM, Andrew Cooper wrote:
>>> On 11/02/15 20:58, Boris Ostrovsky wrote:
>>>> If invalid pointer (i.e. something smaller than HYPERVISOR_VIRT_START)
>>>> is passed for %*ph/%pv/%ps/%pS format specifiers then print "(NULL)"
>>>>
>>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>>> ---
>>>> xen/common/vsprintf.c | 23 ++++++++++++++++-------
>>>> 1 files changed, 16 insertions(+), 7 deletions(-)
>>>>
>>>> v2:
>>>> * Print "(NULL)" instead of specifier-specific string
>>>> * Consider all addresses under HYPERVISOR_VIRT_START as invalid.
>>>> (I think
>>>> this is true for both x86 and ARM but I don't have ARM platform
>>>> to test).
>>>>
>>>>
>>>> diff --git a/xen/common/vsprintf.c b/xen/common/vsprintf.c
>>>> index 065cc42..b9542b5 100644
>>>> --- a/xen/common/vsprintf.c
>>>> +++ b/xen/common/vsprintf.c
>>>> @@ -270,6 +270,22 @@ static char *pointer(char *str, char *end,
>>>> const char **fmt_ptr,
>>>> const char *fmt = *fmt_ptr, *s;
>>>> /* Custom %p suffixes. See
>>>> XEN_ROOT/docs/misc/printk-formats.txt */
>>>> +
>>>> + switch ( fmt[1] )
>>>> + {
>>>> + case 'h':
>>>> + case 's':
>>>> + case 'S':
>>>> + case 'v':
>>>> + ++*fmt_ptr;
>>>> + }
>>>> +
>>>> + if ( (unsigned long)arg < HYPERVISOR_VIRT_START )
>>>> + {
>>>> + char *s = "(NULL)";
>>>> + return string(str, end, s, -1, -1, 0);
>>>> + }
>>>> +
>>>> switch ( fmt[1] )
>>> This wont function, as you have inverted the increment of *fmt_ptr and
>>> check of fmt[1].
>>
>> fmt value doesn't change, it is stashed at the top of the routine.
>
> You are correct. My apologies. I however dislike the splitting of the
> switch into two.
>
>>
>> (What *is* wrong in the above code is the fact that the arg test is
>> done outside the switch. It should be part of the four case
>> statements, otherwise we will print plain %p arguments as "NULL").
>>
>>
>>>
>>> "(NULL)" is inappropriate for non-null pointers less than VIRT_START.
>>
>> Yes, I thought about it after I sent it. "(invalid)"?
>
> Better, but overriding the number with a string does hide information.
> In the case that the pointer is invalid, it would be useful to see its
> contents.
How about "<0xXXXXXX>" (i.e. effectively replace "%pv" with "<%p>", with
angle brackets indicating invalid pointer)?
>
>>
>>>
>>> Given the VIRT check, I would just put the entire switch statement
>>> inside an "if ( (unsigned long)arg < HYPERVISOR_VIRT_START )" block and
>>> let it fall through to the plain number case for a bogus pointer.
>>
>> Not sure I understand what you are suggesting here, sorry.
>>
>> -boris
>
> if ( (unsigned long)arg < HYPERVISOR_VIRT_START )
> {
> switch ( fmt[1] )
> {
> ....
> }
> }
>
>
> This makes the patch a whole 3 line addition and indenting the whole
> switch block by 4 spaces.
Still don't understand. This will never print anything unless it's a bad
pointer, won't it?
(And if you meant '>=' then we will simply print the invalid pointer in
plain %p format. Which, btw, may be the solution but we will still need
to bump fmt_ptr, so we again will need another switch or something to
test for sub-specifier)
-boris
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] vsprintf: Make sure argument to %pX specifier is valid
2015-02-12 15:38 ` Boris Ostrovsky
@ 2015-02-12 15:48 ` Andrew Cooper
2015-02-12 16:33 ` Boris Ostrovsky
0 siblings, 1 reply; 9+ messages in thread
From: Andrew Cooper @ 2015-02-12 15:48 UTC (permalink / raw
To: Boris Ostrovsky, ian.campbell, ian.jackson, jbeulich, keir, tim; +Cc: xen-devel
On 12/02/15 15:38, Boris Ostrovsky wrote:
> On 02/12/2015 10:21 AM, Andrew Cooper wrote:
>> On 12/02/15 15:01, Boris Ostrovsky wrote:
>>> On 02/12/2015 06:04 AM, Andrew Cooper wrote:
>>>> On 11/02/15 20:58, Boris Ostrovsky wrote:
>>>>> If invalid pointer (i.e. something smaller than
>>>>> HYPERVISOR_VIRT_START)
>>>>> is passed for %*ph/%pv/%ps/%pS format specifiers then print "(NULL)"
>>>>>
>>>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>>>> ---
>>>>> xen/common/vsprintf.c | 23 ++++++++++++++++-------
>>>>> 1 files changed, 16 insertions(+), 7 deletions(-)
>>>>>
>>>>> v2:
>>>>> * Print "(NULL)" instead of specifier-specific string
>>>>> * Consider all addresses under HYPERVISOR_VIRT_START as invalid.
>>>>> (I think
>>>>> this is true for both x86 and ARM but I don't have ARM platform
>>>>> to test).
>>>>>
>>>>>
>>>>> diff --git a/xen/common/vsprintf.c b/xen/common/vsprintf.c
>>>>> index 065cc42..b9542b5 100644
>>>>> --- a/xen/common/vsprintf.c
>>>>> +++ b/xen/common/vsprintf.c
>>>>> @@ -270,6 +270,22 @@ static char *pointer(char *str, char *end,
>>>>> const char **fmt_ptr,
>>>>> const char *fmt = *fmt_ptr, *s;
>>>>> /* Custom %p suffixes. See
>>>>> XEN_ROOT/docs/misc/printk-formats.txt */
>>>>> +
>>>>> + switch ( fmt[1] )
>>>>> + {
>>>>> + case 'h':
>>>>> + case 's':
>>>>> + case 'S':
>>>>> + case 'v':
>>>>> + ++*fmt_ptr;
>>>>> + }
>>>>> +
>>>>> + if ( (unsigned long)arg < HYPERVISOR_VIRT_START )
>>>>> + {
>>>>> + char *s = "(NULL)";
>>>>> + return string(str, end, s, -1, -1, 0);
>>>>> + }
>>>>> +
>>>>> switch ( fmt[1] )
>>>> This wont function, as you have inverted the increment of *fmt_ptr and
>>>> check of fmt[1].
>>>
>>> fmt value doesn't change, it is stashed at the top of the routine.
>>
>> You are correct. My apologies. I however dislike the splitting of the
>> switch into two.
>>
>>>
>>> (What *is* wrong in the above code is the fact that the arg test is
>>> done outside the switch. It should be part of the four case
>>> statements, otherwise we will print plain %p arguments as "NULL").
>>>
>>>
>>>>
>>>> "(NULL)" is inappropriate for non-null pointers less than VIRT_START.
>>>
>>> Yes, I thought about it after I sent it. "(invalid)"?
>>
>> Better, but overriding the number with a string does hide information.
>> In the case that the pointer is invalid, it would be useful to see its
>> contents.
>
>
> How about "<0xXXXXXX>" (i.e. effectively replace "%pv" with "<%p>",
> with angle brackets indicating invalid pointer)?
>
It feels like change for change sake, especially as there is a perfectly
good hex decode for plain %p.
>
>>
>>>
>>>>
>>>> Given the VIRT check, I would just put the entire switch statement
>>>> inside an "if ( (unsigned long)arg < HYPERVISOR_VIRT_START )" block
>>>> and
>>>> let it fall through to the plain number case for a bogus pointer.
>>>
>>> Not sure I understand what you are suggesting here, sorry.
>>>
>>> -boris
>>
>> if ( (unsigned long)arg < HYPERVISOR_VIRT_START )
>> {
>> switch ( fmt[1] )
>> {
>> ....
>> }
>> }
>>
>>
>> This makes the patch a whole 3 line addition and indenting the whole
>> switch block by 4 spaces.
>
> Still don't understand. This will never print anything unless it's a
> bad pointer, won't it?
>
> (And if you meant '>=' then we will simply print the invalid pointer
> in plain %p format. Which, btw, may be the solution but we will still
> need to bump fmt_ptr, so we again will need another switch or
> something to test for sub-specifier)
Oops - I did mean >=. I.e. only do the custom %pX decoding in the case
that arg is a plausible pointer.
There is no need I can see to alter the fmt_ptr handling. The code
currently works, other than the issue at hand of falling over a bad pointer.
~Andrew
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] vsprintf: Make sure argument to %pX specifier is valid
2015-02-12 15:48 ` Andrew Cooper
@ 2015-02-12 16:33 ` Boris Ostrovsky
2015-02-12 16:41 ` Boris Ostrovsky
2015-02-12 16:50 ` Andrew Cooper
0 siblings, 2 replies; 9+ messages in thread
From: Boris Ostrovsky @ 2015-02-12 16:33 UTC (permalink / raw
To: Andrew Cooper, ian.campbell, ian.jackson, jbeulich, keir, tim; +Cc: xen-devel
On 02/12/2015 10:48 AM, Andrew Cooper wrote:
> On 12/02/15 15:38, Boris Ostrovsky wrote:
>> On 02/12/2015 10:21 AM, Andrew Cooper wrote:
>>> On 12/02/15 15:01, Boris Ostrovsky wrote:
>>>> On 02/12/2015 06:04 AM, Andrew Cooper wrote:
>>>>> On 11/02/15 20:58, Boris Ostrovsky wrote:
>>>>>> If invalid pointer (i.e. something smaller than
>>>>>> HYPERVISOR_VIRT_START)
>>>>>> is passed for %*ph/%pv/%ps/%pS format specifiers then print "(NULL)"
>>>>>>
>>>>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>>>>> ---
>>>>>> xen/common/vsprintf.c | 23 ++++++++++++++++-------
>>>>>> 1 files changed, 16 insertions(+), 7 deletions(-)
>>>>>>
>>>>>> v2:
>>>>>> * Print "(NULL)" instead of specifier-specific string
>>>>>> * Consider all addresses under HYPERVISOR_VIRT_START as invalid.
>>>>>> (I think
>>>>>> this is true for both x86 and ARM but I don't have ARM platform
>>>>>> to test).
>>>>>>
>>>>>>
>>>>>> diff --git a/xen/common/vsprintf.c b/xen/common/vsprintf.c
>>>>>> index 065cc42..b9542b5 100644
>>>>>> --- a/xen/common/vsprintf.c
>>>>>> +++ b/xen/common/vsprintf.c
>>>>>> @@ -270,6 +270,22 @@ static char *pointer(char *str, char *end,
>>>>>> const char **fmt_ptr,
>>>>>> const char *fmt = *fmt_ptr, *s;
>>>>>> /* Custom %p suffixes. See
>>>>>> XEN_ROOT/docs/misc/printk-formats.txt */
>>>>>> +
>>>>>> + switch ( fmt[1] )
>>>>>> + {
>>>>>> + case 'h':
>>>>>> + case 's':
>>>>>> + case 'S':
>>>>>> + case 'v':
>>>>>> + ++*fmt_ptr;
>>>>>> + }
>>>>>> +
>>>>>> + if ( (unsigned long)arg < HYPERVISOR_VIRT_START )
>>>>>> + {
>>>>>> + char *s = "(NULL)";
>>>>>> + return string(str, end, s, -1, -1, 0);
>>>>>> + }
>>>>>> +
>>>>>> switch ( fmt[1] )
>>>>> This wont function, as you have inverted the increment of *fmt_ptr and
>>>>> check of fmt[1].
>>>> fmt value doesn't change, it is stashed at the top of the routine.
>>> You are correct. My apologies. I however dislike the splitting of the
>>> switch into two.
>>>
>>>> (What *is* wrong in the above code is the fact that the arg test is
>>>> done outside the switch. It should be part of the four case
>>>> statements, otherwise we will print plain %p arguments as "NULL").
>>>>
>>>>
>>>>> "(NULL)" is inappropriate for non-null pointers less than VIRT_START.
>>>> Yes, I thought about it after I sent it. "(invalid)"?
>>> Better, but overriding the number with a string does hide information.
>>> In the case that the pointer is invalid, it would be useful to see its
>>> contents.
>>
>> How about "<0xXXXXXX>" (i.e. effectively replace "%pv" with "<%p>",
>> with angle brackets indicating invalid pointer)?
>>
> It feels like change for change sake, especially as there is a perfectly
> good hex decode for plain %p.
>
>>>>> Given the VIRT check, I would just put the entire switch statement
>>>>> inside an "if ( (unsigned long)arg < HYPERVISOR_VIRT_START )" block
>>>>> and
>>>>> let it fall through to the plain number case for a bogus pointer.
>>>> Not sure I understand what you are suggesting here, sorry.
>>>>
>>>> -boris
>>> if ( (unsigned long)arg < HYPERVISOR_VIRT_START )
>>> {
>>> switch ( fmt[1] )
>>> {
>>> ....
>>> }
>>> }
>>>
>>>
>>> This makes the patch a whole 3 line addition and indenting the whole
>>> switch block by 4 spaces.
>> Still don't understand. This will never print anything unless it's a
>> bad pointer, won't it?
>>
>> (And if you meant '>=' then we will simply print the invalid pointer
>> in plain %p format. Which, btw, may be the solution but we will still
>> need to bump fmt_ptr, so we again will need another switch or
>> something to test for sub-specifier)
> Oops - I did mean >=. I.e. only do the custom %pX decoding in the case
> that arg is a plausible pointer.
>
> There is no need I can see to alter the fmt_ptr handling. The code
> currently works, other than the issue at hand of falling over a bad pointer.
We do, otherwise we will be printing the sub-specifier. Here is example
of what happens if we don't bump it:
struct vcpu *badvcpu = NULL;
printk("badvcpu: %pv current: %pv\n", badvcpu, current);
console:
badvcpu: 0000000000000000v current: d0v0
Also, for %*ph format, if we just go with falling through to plain
format and not marking somehow that we are printing a bad pointer:
unsigned badval = 0xab;
unsigned *badptr = &badval;
printk("badptr = %*ph\n", 1, badptr);
console:
badptr = ab
We don't know here whether badptr was pointing to 0xab or it itself was
0xab.
-boris
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] vsprintf: Make sure argument to %pX specifier is valid
2015-02-12 16:33 ` Boris Ostrovsky
@ 2015-02-12 16:41 ` Boris Ostrovsky
2015-02-12 16:50 ` Andrew Cooper
1 sibling, 0 replies; 9+ messages in thread
From: Boris Ostrovsky @ 2015-02-12 16:41 UTC (permalink / raw
To: Andrew Cooper, ian.campbell, ian.jackson, jbeulich, keir, tim; +Cc: xen-devel
On 02/12/2015 11:33 AM, Boris Ostrovsky wrote:
>
>
> Also, for %*ph format, if we just go with falling through to plain
> format and not marking somehow that we are printing a bad pointer:
>
> unsigned badval = 0xab;
> unsigned *badptr = &badval;
> printk("badptr = %*ph\n", 1, badptr);
>
> console:
> badptr = ab
>
> We don't know here whether badptr was pointing to 0xab or it itself
> was 0xab.
>
Ugh, bad example. Here is what I meant:
unsigned badval = 0xab;
unsigned *badptr = &badval;
unsigned *badvalptr = (void *)0xab;
printk("badvalptr = %*ph badptr = %*ph\n", 1, badvalptr, 1, badptr);
console:
badvalptr = ab badptr = ab
Sorry.
-boris
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] vsprintf: Make sure argument to %pX specifier is valid
2015-02-12 16:33 ` Boris Ostrovsky
2015-02-12 16:41 ` Boris Ostrovsky
@ 2015-02-12 16:50 ` Andrew Cooper
1 sibling, 0 replies; 9+ messages in thread
From: Andrew Cooper @ 2015-02-12 16:50 UTC (permalink / raw
To: Boris Ostrovsky, ian.campbell, ian.jackson, jbeulich, keir, tim; +Cc: xen-devel
On 12/02/15 16:33, Boris Ostrovsky wrote:
> On 02/12/2015 10:48 AM, Andrew Cooper wrote:
>> On 12/02/15 15:38, Boris Ostrovsky wrote:
>>> On 02/12/2015 10:21 AM, Andrew Cooper wrote:
>>>> On 12/02/15 15:01, Boris Ostrovsky wrote:
>>>>> On 02/12/2015 06:04 AM, Andrew Cooper wrote:
>>>>>> On 11/02/15 20:58, Boris Ostrovsky wrote:
>>>>>>> If invalid pointer (i.e. something smaller than
>>>>>>> HYPERVISOR_VIRT_START)
>>>>>>> is passed for %*ph/%pv/%ps/%pS format specifiers then print
>>>>>>> "(NULL)"
>>>>>>>
>>>>>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>>>>>> ---
>>>>>>> xen/common/vsprintf.c | 23 ++++++++++++++++-------
>>>>>>> 1 files changed, 16 insertions(+), 7 deletions(-)
>>>>>>>
>>>>>>> v2:
>>>>>>> * Print "(NULL)" instead of specifier-specific string
>>>>>>> * Consider all addresses under HYPERVISOR_VIRT_START as
>>>>>>> invalid.
>>>>>>> (I think
>>>>>>> this is true for both x86 and ARM but I don't have ARM
>>>>>>> platform
>>>>>>> to test).
>>>>>>>
>>>>>>>
>>>>>>> diff --git a/xen/common/vsprintf.c b/xen/common/vsprintf.c
>>>>>>> index 065cc42..b9542b5 100644
>>>>>>> --- a/xen/common/vsprintf.c
>>>>>>> +++ b/xen/common/vsprintf.c
>>>>>>> @@ -270,6 +270,22 @@ static char *pointer(char *str, char *end,
>>>>>>> const char **fmt_ptr,
>>>>>>> const char *fmt = *fmt_ptr, *s;
>>>>>>> /* Custom %p suffixes. See
>>>>>>> XEN_ROOT/docs/misc/printk-formats.txt */
>>>>>>> +
>>>>>>> + switch ( fmt[1] )
>>>>>>> + {
>>>>>>> + case 'h':
>>>>>>> + case 's':
>>>>>>> + case 'S':
>>>>>>> + case 'v':
>>>>>>> + ++*fmt_ptr;
>>>>>>> + }
>>>>>>> +
>>>>>>> + if ( (unsigned long)arg < HYPERVISOR_VIRT_START )
>>>>>>> + {
>>>>>>> + char *s = "(NULL)";
>>>>>>> + return string(str, end, s, -1, -1, 0);
>>>>>>> + }
>>>>>>> +
>>>>>>> switch ( fmt[1] )
>>>>>> This wont function, as you have inverted the increment of
>>>>>> *fmt_ptr and
>>>>>> check of fmt[1].
>>>>> fmt value doesn't change, it is stashed at the top of the routine.
>>>> You are correct. My apologies. I however dislike the splitting of
>>>> the
>>>> switch into two.
>>>>
>>>>> (What *is* wrong in the above code is the fact that the arg test is
>>>>> done outside the switch. It should be part of the four case
>>>>> statements, otherwise we will print plain %p arguments as "NULL").
>>>>>
>>>>>
>>>>>> "(NULL)" is inappropriate for non-null pointers less than
>>>>>> VIRT_START.
>>>>> Yes, I thought about it after I sent it. "(invalid)"?
>>>> Better, but overriding the number with a string does hide information.
>>>> In the case that the pointer is invalid, it would be useful to see its
>>>> contents.
>>>
>>> How about "<0xXXXXXX>" (i.e. effectively replace "%pv" with "<%p>",
>>> with angle brackets indicating invalid pointer)?
>>>
>> It feels like change for change sake, especially as there is a perfectly
>> good hex decode for plain %p.
>>
>>>>>> Given the VIRT check, I would just put the entire switch statement
>>>>>> inside an "if ( (unsigned long)arg < HYPERVISOR_VIRT_START )" block
>>>>>> and
>>>>>> let it fall through to the plain number case for a bogus pointer.
>>>>> Not sure I understand what you are suggesting here, sorry.
>>>>>
>>>>> -boris
>>>> if ( (unsigned long)arg < HYPERVISOR_VIRT_START )
>>>> {
>>>> switch ( fmt[1] )
>>>> {
>>>> ....
>>>> }
>>>> }
>>>>
>>>>
>>>> This makes the patch a whole 3 line addition and indenting the whole
>>>> switch block by 4 spaces.
>>> Still don't understand. This will never print anything unless it's a
>>> bad pointer, won't it?
>>>
>>> (And if you meant '>=' then we will simply print the invalid pointer
>>> in plain %p format. Which, btw, may be the solution but we will still
>>> need to bump fmt_ptr, so we again will need another switch or
>>> something to test for sub-specifier)
>> Oops - I did mean >=. I.e. only do the custom %pX decoding in the case
>> that arg is a plausible pointer.
>>
>> There is no need I can see to alter the fmt_ptr handling. The code
>> currently works, other than the issue at hand of falling over a bad
>> pointer.
>
> We do, otherwise we will be printing the sub-specifier. Here is
> example of what happens if we don't bump it:
>
> struct vcpu *badvcpu = NULL;
> printk("badvcpu: %pv current: %pv\n", badvcpu, current);
>
> console:
> badvcpu: 0000000000000000v current: d0v0
>
Ah - I see what you mean now.
>
> Also, for %*ph format, if we just go with falling through to plain
> format and not marking somehow that we are printing a bad pointer:
>
> unsigned badval = 0xab;
> unsigned *badptr = &badval;
> printk("badptr = %*ph\n", 1, badptr);
>
> console:
> badptr = ab
>
> We don't know here whether badptr was pointing to 0xab or it itself
> was 0xab.
Ok. As this is only for making debug code less fragile, it probably is
better to explicitly call out a bad pointer differently.
~Andrew
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-02-12 16:50 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-02-11 20:58 [PATCH v2] vsprintf: Make sure argument to %pX specifier is valid Boris Ostrovsky
2015-02-12 11:04 ` Andrew Cooper
2015-02-12 15:01 ` Boris Ostrovsky
2015-02-12 15:21 ` Andrew Cooper
2015-02-12 15:38 ` Boris Ostrovsky
2015-02-12 15:48 ` Andrew Cooper
2015-02-12 16:33 ` Boris Ostrovsky
2015-02-12 16:41 ` Boris Ostrovsky
2015-02-12 16:50 ` Andrew Cooper
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.