From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.0 required=3.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1D3D2C433E0 for ; Thu, 11 Jun 2020 19:39:12 +0000 (UTC) Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id E1FE320836 for ; Thu, 11 Jun 2020 19:39:11 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (1024-bit key) header.d=xen.org header.i=@xen.org header.b="bvuut4iG" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org E1FE320836 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=xen.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=xen-devel-bounces@lists.xenproject.org Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1jjT1v-0008I8-Vi; Thu, 11 Jun 2020 19:38:27 +0000 Received: from all-amaz-eas1.inumbo.com ([34.197.232.57] helo=us1-amaz-eas2.inumbo.com) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1jjT1u-0008I3-R8 for xen-devel@lists.xenproject.org; Thu, 11 Jun 2020 19:38:26 +0000 X-Inumbo-ID: 1dcd6e70-ac1b-11ea-b570-12813bfff9fa Received: from mail.xenproject.org (unknown [104.130.215.37]) by us1-amaz-eas2.inumbo.com (Halon) with ESMTPS id 1dcd6e70-ac1b-11ea-b570-12813bfff9fa; Thu, 11 Jun 2020 19:38:25 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=xen.org; s=20200302mail; h=Content-Transfer-Encoding:Content-Type:In-Reply-To: MIME-Version:Date:Message-ID:From:References:Cc:To:Subject:Sender:Reply-To: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=q6cqqMdgtTXIpIzrKG4x/y7h7KqE1tyD+pSV9cB939s=; b=bvuut4iGxFjNfH+HSEpPLUeHOa sRDzMz5xULNUVUqFXYCmeUJ8VX0HH0keqTLd1bZ/tJWkVfXi9GS6ysHT182gadRJF9nx1bMSRF0Us +7GEs7uEiOkSmDO5F6lOW3sZeA+NJOKdSyFLKAOVnNz4vJd7UvaYPdkmC7aZUIXs92rk=; Received: from xenbits.xenproject.org ([104.239.192.120]) by mail.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1jjT1p-0003t9-1x; Thu, 11 Jun 2020 19:38:21 +0000 Received: from cpc91200-cmbg18-2-0-cust94.5-4.cable.virginm.net ([81.100.41.95] helo=a483e7b01a66.ant.amazon.com) by xenbits.xenproject.org with esmtpsa (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1jjT1o-0006Nm-Rh; Thu, 11 Jun 2020 19:38:20 +0000 Subject: Re: [PATCH 1/2] xen/arm: Convert runstate address during hypcall To: Stefano Stabellini , Julien Grall References: <8b450dddb2c855225c97741dce66453a80b9add2.1591806713.git.bertrand.marquis@arm.com> From: Julien Grall Message-ID: <74475748-e884-1e6e-633d-bf67d5ed32fe@xen.org> Date: Thu, 11 Jun 2020 20:38:18 +0100 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:68.0) Gecko/20100101 Thunderbird/68.9.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 7bit X-BeenThere: xen-devel@lists.xenproject.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Cc: Wei Liu , Andrew Cooper , Ian Jackson , George Dunlap , Bertrand Marquis , Jan Beulich , xen-devel , nd , Volodymyr Babchuk , =?UTF-8?Q?Roger_Pau_Monn=c3=a9?= Errors-To: xen-devel-bounces@lists.xenproject.org Sender: "Xen-devel" Hi Stefano, On 11/06/2020 19:50, Stefano Stabellini wrote: > On Thu, 11 Jun 2020, Julien Grall wrote: >>>> + return -EINVAL; >>>> } >>>> >>>> - __copy_to_guest(runstate_guest(v), &runstate, 1); >>>> + v->arch.runstate_guest.page = page; >>>> + v->arch.runstate_guest.offset = offset; >>>> + >>>> + spin_unlock(&v->arch.runstate_guest.lock); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> + >>>> +/* Update per-VCPU guest runstate shared memory area (if registered). */ >>>> +static void update_runstate_area(struct vcpu *v) >>>> +{ >>>> + struct vcpu_runstate_info *guest_runstate; >>>> + void *p; >>>> + >>>> + spin_lock(&v->arch.runstate_guest.lock); >>>> >>>> - if ( guest_handle ) >>>> + if ( v->arch.runstate_guest.page ) >>>> { >>>> - runstate.state_entry_time &= ~XEN_RUNSTATE_UPDATE; >>>> + p = __map_domain_page(v->arch.runstate_guest.page); >>>> + guest_runstate = p + v->arch.runstate_guest.offset; >>>> + >>>> + if ( VM_ASSIST(v->domain, runstate_update_flag) ) >>>> + { >>>> + v->runstate.state_entry_time |= XEN_RUNSTATE_UPDATE; >>>> + guest_runstate->state_entry_time |= XEN_RUNSTATE_UPDATE; >>> >>> I think that this write to guest_runstate should use write_atomic or >>> another atomic write operation. >> >> I thought about suggesting the same, but guest_copy_* helpers may not >> do a single memory write to state_entry_time. >> What are you trying to prevent with the write_atomic()? > > I am thinking that without using an atomic write, it would be (at least > theoretically) possible for a guest to see a partial write to > state_entry_time, which is not good. It is already the case with existing implementation as Xen may write byte by byte. So are you suggesting the existing code is also buggy? > In theory, the set of assembly > instructions generated by the compiler could go through an intermediate > state that we don't want the guest to see. In practice, I doubt that any > possible combination of assembly instructions generated by the compiler > could lead to something harmful. Well, I think you first need to define the theoritical state you are worried about. Then we can discuss whether they can happen in practice and how we can prevent them in the existing and new code. So what sort of state your are concerned? Cheers, -- Julien Grall