From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefan Hajnoczi Subject: [PATCH 2/8] vhost: avoid enum fields in VhostUserMsg Date: Mon, 5 Feb 2018 12:16:36 +0000 Message-ID: <20180205121642.26428-3-stefanha@redhat.com> References: <20180205121642.26428-1-stefanha@redhat.com> Cc: Maxime Coquelin , Yuanhan Liu , Stefan Hajnoczi To: dev@dpdk.org Return-path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id 9EF961B33B for ; Mon, 5 Feb 2018 13:17:04 +0100 (CET) In-Reply-To: <20180205121642.26428-1-stefanha@redhat.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 -- 2.14.3