From mboxrd@z Thu Jan 1 00:00:00 1970 From: Maxime Coquelin Subject: Re: [PATCH 2/8] vhost: avoid enum fields in VhostUserMsg Date: Tue, 6 Feb 2018 10:47:31 +0100 Message-ID: References: <20180205121642.26428-1-stefanha@redhat.com> <20180205121642.26428-3-stefanha@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: Yuanhan Liu To: Stefan Hajnoczi , dev@dpdk.org 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) In-Reply-To: <20180205121642.26428-3-stefanha@redhat.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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