From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH v2] vsprintf: Make sure argument to %pX specifier is valid Date: Thu, 12 Feb 2015 11:33:10 -0500 Message-ID: <54DCD5C6.603@oracle.com> References: <1423688332-3464-1-git-send-email-boris.ostrovsky@oracle.com> <54DC88D3.5080803@citrix.com> <54DCC060.8040208@oracle.com> <54DCC4E6.6070908@citrix.com> <54DCC8E5.8080305@oracle.com> <54DCCB3B.8090703@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <54DCCB3B.8090703@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andrew Cooper , ian.campbell@citrix.com, ian.jackson@eu.citrix.com, jbeulich@suse.com, keir@xen.org, tim@xen.org Cc: xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org 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 >>>>>> --- >>>>>> 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