starlingx/nfv
Code Issues Proposed changes

ensure string "null-terminated" and fix memory overwrite risk.

Browse Source 
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
This commit is contained in:
SidneyAn 2018-11-16 09:42:30 +08:00 committed by Ran An
parent ddefd385c5
commit 9d0703d95f
1 changed files with 12 additions and 4 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

16
guest-comm/host-guest-comm-2.0/host_instance_mgmt.c
View File

@ -294,9 +294,17 @@ void vio_full_disconnect(instance_t *instance)
* *
* Returns a pointer to the instance name on success, or NULL on failure. * 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; 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) if (rc == 1)
return instance_name; return instance_name;
else else
@ -328,7 +336,7 @@ static void socket_deleted(char *fn)
if (!fn) if (!fn)
return; return;
instance_name = file_to_instance_name(fn, buf); instance_name = file_to_instance_name(fn, buf, sizeof(buf));
if (!instance_name) if (!instance_name)
// Not a file we care about. // Not a file we care about.
return; return;
@ -429,7 +437,7 @@ static int socket_added(char *filename)
return -1; return -1;
} }
instance_name = file_to_instance_name(filename, namebuf); instance_name = file_to_instance_name(filename, namebuf, sizeof(namebuf));
if (!instance_name) { if (!instance_name) {
// Not a bug, just not a file we care about. // Not a bug, just not a file we care about.
return -1; return -1;