From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH v5 06/16] x86/hvm: add length to mmio check op Date: Thu, 2 Jul 2015 17:37:30 +0100 Message-ID: <559568CA.2060406@citrix.com> References: <1435669558-5421-1-git-send-email-paul.durrant@citrix.com> <1435669558-5421-7-git-send-email-paul.durrant@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZAhUV-0001ZA-Dq for xen-devel@lists.xenproject.org; Thu, 02 Jul 2015 16:37:35 +0000 In-Reply-To: <1435669558-5421-7-git-send-email-paul.durrant@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: Paul Durrant , xen-devel@lists.xenproject.org Cc: Keir Fraser , Jan Beulich List-Id: xen-devel@lists.xenproject.org On 30/06/15 14:05, Paul Durrant wrote: > When memory mapped I/O is range checked by internal handlers, the length > of the access should be taken into account. > > Signed-off-by: Paul Durrant > Cc: Keir Fraser > Cc: Jan Beulich > Cc: Andrew Cooper > --- > xen/arch/x86/hvm/intercept.c | 22 +++++++++++++++++++--- > xen/include/asm-x86/hvm/io.h | 16 ++++++++++++++++ > 2 files changed, 35 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/x86/hvm/intercept.c b/xen/arch/x86/hvm/intercept.c > index 7d36785..42050f4 100644 > --- a/xen/arch/x86/hvm/intercept.c > +++ b/xen/arch/x86/hvm/intercept.c > @@ -35,9 +35,19 @@ > static bool_t hvm_mmio_accept(const struct hvm_io_handler *handler, > const ioreq_t *p) > { > + paddr_t first = hvm_mmio_first_byte(p); > + paddr_t last = hvm_mmio_last_byte(p); > + > BUG_ON(handler->type != IOREQ_TYPE_COPY); > > - return handler->mmio.ops->check(current, p->addr); > + if ( !handler->mmio.ops->check(current, first) ) > + return 0; > + I would put a comment here about an IO access straddling an MMIO handler boundary, so that someone investigating this domain crash gets some clue as to why. > + if ( p->size > 1 && > + !handler->mmio.ops->check(current, last) ) > + domain_crash(current->domain); > + > + return 1; > } > > static int hvm_mmio_read(const struct hvm_io_handler *handler, > @@ -112,7 +122,8 @@ static const struct hvm_io_ops portio_ops = { > static int hvm_process_io_intercept(const struct hvm_io_handler *handler, > ioreq_t *p) > { > - struct hvm_vcpu_io *vio = ¤t->arch.hvm_vcpu.hvm_io; > + struct vcpu *curr = current; > + struct hvm_vcpu_io *vio = &curr->arch.hvm_vcpu.hvm_io; > const struct hvm_io_ops *ops = > (p->type == IOREQ_TYPE_COPY) ? > &mmio_ops : > @@ -223,6 +234,9 @@ static int hvm_process_io_intercept(const struct hvm_io_handler *handler, > > if ( i != 0 ) > { > + if ( rc == X86EMUL_UNHANDLEABLE ) > + domain_crash(curr->domain); > + > p->count = i; > rc = X86EMUL_OKAY; > } > @@ -342,7 +356,9 @@ bool_t hvm_mmio_internal(paddr_t gpa) > { > ioreq_t p = { > .type = IOREQ_TYPE_COPY, > - .addr = gpa > + .addr = gpa, As a general note, many compilers (gcc includes) permit having a comma as the final token before the } which avoids a diff which looks like this when adding a subsequent member. Otherwise, Reviewed-by: Andrew Cooper