From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id DCC931B686 for ; Tue, 6 Feb 2018 10:47:40 +0100 (CET) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 4643C61D22; Tue, 6 Feb 2018 09:47:40 +0000 (UTC) Received: from [10.36.112.19] (ovpn-112-19.ams2.redhat.com [10.36.112.19]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 27BAF60462; Tue, 6 Feb 2018 09:47:35 +0000 (UTC) To: Stefan Hajnoczi , dev@dpdk.org Cc: Yuanhan Liu References: <20180205121642.26428-1-stefanha@redhat.com> <20180205121642.26428-3-stefanha@redhat.com> From: Maxime Coquelin Message-ID: Date: Tue, 6 Feb 2018 10:47:31 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 MIME-Version: 1.0 In-Reply-To: <20180205121642.26428-3-stefanha@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Tue, 06 Feb 2018 09:47:40 +0000 (UTC) Subject: Re: [dpdk-dev] [PATCH 2/8] vhost: avoid enum fields in VhostUserMsg X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 06 Feb 2018 09:47:41 -0000 On 02/05/2018 01:16 PM, Stefan Hajnoczi wrote: > The VhostUserMsg struct binary representation must match the vhost-user > protocol specification since this struct is read from and written to the > socket. > > The VhostUserMsg.request union contains enum fields. Enum binary > representation is implementation-defined according to the C standard and > it is unportable to make assumptions about the representation: > > 6.7.2.2 Enumeration specifiers > ... > Each enumerated type shall be compatible with char, a signed integer > type, or an unsigned integer type. The choice of type is > implementation-defined, but shall be capable of representing the > values of all the members of the enumeration. > > Additionally, librte_vhost relies on the enum type being unsigned when > validating untrusted inputs: > > if (ret <= 0 || msg.request.master >= VHOST_USER_MAX) { > > If msg.request.master is signed then negative values pass this check! > > Even if we assume gcc on x86_64 (SysV amd64 ABI) and don't care about > portability, the actual enum constants still affect the final type. For > example, if we add a negative constant then its type changes to signed > int: > > typedef enum VhostUserRequest { > ... > VHOST_USER_INVALID = -1, > }; > > This is very fragile and it's unlikely that anyone changing the code > would remember this. A security hole can be introduced accidentally. > > This patch switches VhostUserMsg.request fields to uint32_t to avoid the > portability and potential security issues. > > Signed-off-by: Stefan Hajnoczi > --- > lib/librte_vhost/vhost_user.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/lib/librte_vhost/vhost_user.h b/lib/librte_vhost/vhost_user.h > index d4bd604b9..0fafbe6e0 100644 > --- a/lib/librte_vhost/vhost_user.h > +++ b/lib/librte_vhost/vhost_user.h > @@ -81,8 +81,8 @@ typedef struct VhostUserLog { > > typedef struct VhostUserMsg { > union { > - VhostUserRequest master; > - VhostUserSlaveRequest slave; > + uint32_t master; /* a VhostUserRequest value */ > + uint32_t slave; /* a VhostUserSlaveRequest value*/ > } request; > > #define VHOST_USER_VERSION_MASK 0x3 > Maybe we could simplify to: typedef struct VhostUserMsg { uint32_t request; /* a VhostUserRequest or VhostUserSlaveRequest value */ ... Also, it seems QEMU's vhost-user master implementation uses an enum for the request in its VhostUserMsg struct. Should it be fixed too? Thanks, Maxime