From 10953f90d724fab67897fde13a47f84499de5b59 Mon Sep 17 00:00:00 2001 From: John Levon Date: Fri, 16 Aug 2024 17:56:32 +0100 Subject: [PATCH 1/2] samples: keep coverity quiet >>> CID 467268: (INTEGER_OVERFLOW) >>> Expression "32UL + bitmap_size", which is equal to 31, where "bitmap_size" is known to be equal to 18446744073709551615, overflows the type that receives it, an unsigned integer 64 bits wide. 824 size_t size = sizeof(*res) + sizeof(*report) + bitmap_size; It's correct, this could overflow, though as this is example code, it doesn't matter. Nonetheless let's make this be a saturating add. Signed-off-by: John Levon --- samples/client.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/samples/client.c b/samples/client.c index e8b737f4..7e6721f8 100644 --- a/samples/client.c +++ b/samples/client.c @@ -821,7 +821,8 @@ get_dirty_bitmap(int sock, struct client_dma_region *dma_region, uint64_t bitmap_size = get_bitmap_size(dma_region->map.size, sysconf(_SC_PAGESIZE)); - size_t size = sizeof(*res) + sizeof(*report) + bitmap_size; + /* Saturating add to keep coverity happy. */ + size_t size = satadd_u64(sizeof(*res) + sizeof(*report), bitmap_size); void *data = calloc(1, size); assert(data != NULL); From a6d8ebbaf6b57dad9ac846e992ee5442e2e3d280 Mon Sep 17 00:00:00 2001 From: John Levon Date: Fri, 16 Aug 2024 18:03:52 +0100 Subject: [PATCH 2/2] Add further sanity checking of hdr->error_no >>> CID 467267: Insecure data handling (INTEGER_OVERFLOW) >>> The cast of "hdr->error_no" to a signed type could result in a negative number. Indeed, if a client sends a very large ->error_no, this could end up with a negative errno value. This doesn't seem like an issue, but nonetheless tighten up our validation. For some reason Coverity only complained about tran_pipe.c, but the same problem exists in tran_sock.c. Signed-off-by: John Levon --- lib/private.h | 6 ++++++ lib/tran_pipe.c | 2 +- lib/tran_sock.c | 2 +- 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/lib/private.h b/lib/private.h index e3d97b32..b89df8b2 100644 --- a/lib/private.h +++ b/lib/private.h @@ -61,6 +61,12 @@ sizeof(struct vfio_user_header) + \ sizeof(struct vfio_user_region_access)) +/* + * Maximum value we are prepared to accept in hdr->error_no. Somewhat arbitrary + * value low enough to avoid any signed conversion issues. + */ +#define SERVER_MAX_ERROR_NO (4096) + /* * Structure used to hold an in-flight request+reply. * diff --git a/lib/tran_pipe.c b/lib/tran_pipe.c index 4c3dc9c1..48d8ea4e 100644 --- a/lib/tran_pipe.c +++ b/lib/tran_pipe.c @@ -137,7 +137,7 @@ tran_pipe_recv(int fd, struct vfio_user_header *hdr, bool is_reply, } if (hdr->flags & VFIO_USER_F_ERROR) { - if (hdr->error_no <= 0) { + if (hdr->error_no <= 0 || hdr->error_no > SERVER_MAX_ERROR_NO) { hdr->error_no = EINVAL; } return ERROR_INT(hdr->error_no); diff --git a/lib/tran_sock.c b/lib/tran_sock.c index 024e5b01..5f144b29 100644 --- a/lib/tran_sock.c +++ b/lib/tran_sock.c @@ -217,7 +217,7 @@ tran_sock_recv_fds(int sock, struct vfio_user_header *hdr, bool is_reply, } if (hdr->flags & VFIO_USER_F_ERROR) { - if (hdr->error_no <= 0) { + if (hdr->error_no <= 0 || hdr->error_no > SERVER_MAX_ERROR_NO) { hdr->error_no = EINVAL; } return ERROR_INT(hdr->error_no);