Re: [dpdk-dev] [PATCH 2/8] vhost: avoid enum fields in VhostUserMsg

[Maxime Coquelin <maxime.coquelin@redhat.com>]

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 <stefanha@redhat.com>
> ---
>   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