Add further sanity checking of hdr->error_no (#805)

>>> 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 <[email protected]>
nutanix · Aug 19, 2024 · b1a156d · b1a156d
1 parent 3f1500b
commit b1a156d
Show file tree

Hide file tree

Showing 3 changed files with 8 additions and 2 deletions.
diff --git 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
@@ -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
@@ -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);