[dpdk-dev,2/8] vhost: avoid enum fields in VhostUserMsg
Checks
Commit Message
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(-)
Comments
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
@@ -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