From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH v3] vsprintf: Make sure argument to %pX specifier is valid Date: Wed, 18 Feb 2015 11:10:18 -0500 Message-ID: <54E4B96A.7060304@oracle.com> References: <1424273966-3566-1-git-send-email-boris.ostrovsky@oracle.com> <54E4B555.7030700@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <54E4B555.7030700@linaro.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Julien Grall , 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/18/2015 10:52 AM, Julien Grall wrote: > Hi Boris, > > On 18/02/2015 15:39, 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 value of the >> pointer in parentheses. >> >> For example: >> >> struct vcpu *v0 = NULL; >> struct vcpu *v1 = (void *)0xffUL; >> unsigned val = 0xab; >> unsigned *ptr = &val; >> unsigned *badptr = (void *)0xab; >> printk("v0 = %pv, v1 = %pv, curr = %pv\n", v0, v1, current); >> printk("badptr = %*ph, ptr = %*ph\n", 1, badptr, 1, ptr); >> >> will produce >> v0 = (0), v1 = (ff), curr = d0v3 >> badptr = (ab), ptr = ab >> >> Signed-off-by: Boris Ostrovsky >> --- >> xen/common/vsprintf.c | 23 ++++++++++++++++++++++- >> 1 files changed, 22 insertions(+), 1 deletions(-) >> >> v3: >> * Print value of the bad pointer in parentheses. >> (I understand Andrew's dislike of additional switch but I >> think this is the cleanest way) >> >> 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). > > This assumption is valid on ARM too. Although, we may have some > mappings after HYPERVISOR_VIRT_START why are not valid. > > On ARM, HYPERVISOR_VIRT_END marks the end of mapping which is not > always mapped (such as the domheap). Would it make sense to test it? > Although it seems that x86 doesn't have the same meaning for this macro. This patch is only trying to avoid obviously bad pointers but it doesn't guarantee that a pointer that's not below HYPERVISOR_VIRT_START is good (e.g. if you try to print %pv for HYPERVISOR_VIRT_START bad things will likely happen). Since there are cases when we want to dereference something above HYPERVISOR_VIRT_END I think adding more tests would make things a bit more complicated: we will just keep adding more tests. I don't know if there is a good solution short of testing "goodness" of each pointer (regardless of where it points). -boris