From 9d0703d95f30e4b6daf2e58189077348753355f2 Mon Sep 17 00:00:00 2001
From: SidneyAn <ran1.an@intel.com>
Date: Fri, 16 Nov 2018 09:42:30 +0800
Subject: [PATCH] ensure string "null-terminated" and fix memory overwrite
 risk.

Description:
1. once new socket is added, "strncpy" is used to copy instance_name
from source string to dest, but it does not guarantee null terminated.
2. there is a memory overwrite risk when it get instance_name from
a file's name

Solution:
1. we bounded length of string instance_name to ensure it is
"null-terminated".
2. limit the copy length when instance_name is get

Test Case:
  1. success to build and deploy 1 controller + 1 compute (virtual)
  2. trigger memory overwrite in a debug version with some logs added.
     With origin code, "instance_name" in function "file_to_instance_name()"
is assigned to a string whose length is greater than its capacity.
     With patch code, "instance_name" has a limit assign length
and a null terminate.

Reproduce:
To trigger memory overwrite case, a socket file with super long name is
generated under "/var/lib/libvirt/qemu/" which is monitored by this software

Closes-Bug: 1794704
Signed-off-by: SidneyAn <ran1.an@intel.com>
Change-Id: Ifb97e3dc1b59ebdc23cda73731fb02dc342d0520
---
 .../host-guest-comm-2.0/host_instance_mgmt.c     | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/guest-comm/host-guest-comm-2.0/host_instance_mgmt.c b/guest-comm/host-guest-comm-2.0/host_instance_mgmt.c
index 5d6f1d78..6fb7d069 100644
--- a/guest-comm/host-guest-comm-2.0/host_instance_mgmt.c
+++ b/guest-comm/host-guest-comm-2.0/host_instance_mgmt.c
@@ -294,9 +294,17 @@ void vio_full_disconnect(instance_t *instance)
  *
  * Returns a pointer to the instance name on success, or NULL on failure.
  */
-char *file_to_instance_name(char *filename, char* instance_name) {
+char *file_to_instance_name(char *filename, char* instance_name,
+                            unsigned int inst_name_len) {
     int rc;
-    rc = sscanf(filename, "cgcs.messaging.%[^.].sock", instance_name);
+    char format_string[100];
+
+    if (inst_name_len == 0)
+        return NULL;
+
+    snprintf(format_string, sizeof(format_string),
+            "cgcs.messaging.%%%d[^.].sock", inst_name_len-1);
+    rc = sscanf(filename, format_string, instance_name);
     if (rc == 1)
         return instance_name;
     else
@@ -328,7 +336,7 @@ static void socket_deleted(char *fn)
     if (!fn)
         return;
     
-    instance_name = file_to_instance_name(fn, buf);
+    instance_name = file_to_instance_name(fn, buf, sizeof(buf));
     if (!instance_name)
         // Not a file we care about.
         return;
@@ -429,7 +437,7 @@ static int socket_added(char *filename)
         return -1;
     }
     
-    instance_name = file_to_instance_name(filename, namebuf);
+    instance_name = file_to_instance_name(filename, namebuf, sizeof(namebuf));
     if (!instance_name) {
         // Not a bug, just not a file we care about.
         return -1;