[Skiboot] [PATCH v3 7/7] fsp/leds: improve string operations bounds checking

[Nicholas Piggin]

The current code has a few possible issues with string handling, and
gcc flags a number of string / buffer warnings when enabling more
checking.

Some of the issues in the file:

- Mixing of null-terminated arrays (in most cases), and non-null in the
  input/output buffer format. memcpy generally should be used when the
  length is known.
- Lack of input data length bounds checking. Malformed input could
  cause overruns.
- String copying from same sized source and destination array sizes,
  where the source is a NUL terminated string, so the strncpy copies
  the string without its NUL terminator, which becomes NUL terminated
  at the zeroed destination array. Compiler does not like this, and
  it only works if the destination has been zeroed, so not a great
  pattern.
- Attemping to NUL terminate string using strcat, which will overwrite
  a byte past the end of the array if the string length is at maximum,
  or worse if the input was malformed.

This patch fixes several of these issues and fixes a number of compiler
warnings. In general, the buffer and string handling could probably
benefit from a more in-depth audit.

Reviewed-by: Vasant Hegde <hegdevasant at linux.vnet.ibm.com>
Tested-by: Vasant Hegde <hegdevasant at linux.vnet.ibm.com>
Signed-off-by: Nicholas Piggin <npiggin at gmail.com>
---
 hw/fsp/fsp-leds.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/hw/fsp/fsp-leds.c b/hw/fsp/fsp-leds.c
index 9040c0a4d..edfda51da 100644
--- a/hw/fsp/fsp-leds.c
+++ b/hw/fsp/fsp-leds.c
@@ -534,8 +534,11 @@ static int fsp_msg_set_led_state(struct led_set_cmd *spcn_cmd)
 	u32 cmd = FSP_RSP_SET_LED_STATE;
 	int rc = -1;
 
+	memset(sled.lc_code, 0, LOC_CODE_SIZE);
 	sled.lc_len = strlen(spcn_cmd->loc_code);
-	strncpy(sled.lc_code, spcn_cmd->loc_code, sled.lc_len);
+	if (sled.lc_len >= LOC_CODE_SIZE)
+		sled.lc_len = LOC_CODE_SIZE - 1;
+	strncpy(sled.lc_code, spcn_cmd->loc_code, LOC_CODE_SIZE - 1);
 
 	lock(&led_lock);
 
@@ -604,8 +607,8 @@ static int fsp_msg_set_led_state(struct led_set_cmd *spcn_cmd)
 	}
 
 	/* Write into SPCN TCE buffer */
-	buf_write(buf, u8, sled.lc_len);	 /* Location code length */
-	strncpy(buf, sled.lc_code, sled.lc_len); /* Location code */
+	buf_write(buf, u8, sled.lc_len);	/* Location code length */
+	memcpy(buf, sled.lc_code, sled.lc_len);	/* Location code */
 	buf += sled.lc_len;
 	buf_write(buf, u16, sled.state);	/* LED state */
 
@@ -780,7 +783,7 @@ static u32 fsp_push_data_to_tce(struct fsp_led_data *led, u8 *out_data,
 	/* Location code */
 	memset(lcode.loc_code, 0, LOC_CODE_SIZE);
 	lcode.raw_len = strlen(led->loc_code);
-	strncpy(lcode.loc_code, led->loc_code, lcode.raw_len);
+	strncpy(lcode.loc_code, led->loc_code, LOC_CODE_SIZE - 1);
 	lcode.fld_sz = sizeof(lcode.loc_code);
 
 	/* Rest of the structure */
@@ -1631,6 +1634,8 @@ static void fsp_process_leds_data(u16 len)
 	 */
 	buf = led_buffer;
 	while (len) {
+		size_t lc_len;
+
 		/* Prepare */
 		led_data = zalloc(sizeof(struct fsp_led_data));
 		assert(led_data);
@@ -1643,14 +1648,18 @@ static void fsp_process_leds_data(u16 len)
 		buf_read(buf, u8, &led_data->lc_len);
 		len -= sizeof(led_data->lc_len);
 
-		if (led_data->lc_len == 0) {
+		lc_len = led_data->lc_len;
+		if (lc_len == 0) {
 			free(led_data);
 			break;
 		}
 
+		if (lc_len >= LOC_CODE_SIZE)
+			lc_len = LOC_CODE_SIZE - 1;
+
 		/* Location code */
-		strncpy(led_data->loc_code, buf, led_data->lc_len);
-		strcat(led_data->loc_code, "\0");
+		strncpy(led_data->loc_code, buf, lc_len);
+		led_data->loc_code[lc_len] = '\0';
 
 		buf += led_data->lc_len;
 		len -= led_data->lc_len;
@@ -1673,13 +1682,7 @@ static void fsp_process_leds_data(u16 len)
 			assert(encl_led_data);
 
 			/* copy over the original */
-			encl_led_data->rid = led_data->rid;
-			encl_led_data->lc_len = led_data->lc_len;
-			strncpy(encl_led_data->loc_code, led_data->loc_code,
-				led_data->lc_len);
-			encl_led_data->loc_code[led_data->lc_len] = '\0';
-			encl_led_data->parms = led_data->parms;
-			encl_led_data->status = led_data->status;
+			memcpy(encl_led_data, led_data, sizeof(struct fsp_led_data));
 
 			/* Add to the list of enclosure LEDs */
 			list_add_tail(&encl_ledq, &encl_led_data->link);
-- 
2.20.1