ipmitool/0024-sdr-Fix-segfault-on-invalid-unit-types.patch

From 12e2f5da63fcfdb544f87ec492e5b1bc4f89868c Mon Sep 17 00:00:00 2001
From: Alexander Amelkin <alexander@amelkin.msk.ru>
Date: Fri, 19 Apr 2019 15:07:25 +0300
Subject: [PATCH] sdr: Fix segfault on invalid unit types

The program would crash if the BMC returned an out of range (>90)
unit type for a full sensor record. This commit adds a range check
and also add support for IPMI 2.0 additional unit types 91 and 92
("fatal error" and "grams").

Resolves ipmitool/ipmitool#118

Signed-off-by: Alexander Amelkin <alexander@amelkin.msk.ru>
---
 include/ipmitool/ipmi_sdr.h | 32 ++++++++++++++---
 lib/ipmi_sdr.c              | 72 +++++++++++++++++++++++++------------
 2 files changed, 78 insertions(+), 26 deletions(-)

diff --git a/include/ipmitool/ipmi_sdr.h b/include/ipmitool/ipmi_sdr.h
index 5e6afd3..9f783c4 100644
--- a/include/ipmitool/ipmi_sdr.h
+++ b/include/ipmitool/ipmi_sdr.h
@@ -37,6 +37,7 @@
 # include <config.h>
 #endif
 
+#include <stdbool.h>
 #include <inttypes.h>
 #include <math.h>
 #include <ipmitool/bswap.h>
@@ -381,6 +382,29 @@ struct sdr_record_common_sensor {
 
 	struct sdr_record_mask mask;
 
+/* IPMI 2.0, Table 43-1, byte 21[7:6] Analog (numeric) Data Format */
+#define SDR_UNIT_FMT_UNSIGNED 0 /* unsigned */
+#define SDR_UNIT_FMT_1S_COMPL 1 /* 1's complement (signed) */
+#define SDR_UNIT_FMT_2S_COMPL 2 /* 2's complement (signed) */
+#define SDR_UNIT_FMT_NA 3 /* does not return analog (numeric) reading */
+/* IPMI 2.0, Table 43-1, byte 21[5:3] Rate */
+#define SDR_UNIT_RATE_NONE 0 /* none */
+#define SDR_UNIT_RATE_MICROSEC 1 /* per us */
+#define SDR_UNIT_RATE_MILLISEC 2 /* per ms */
+#define SDR_UNIT_RATE_SEC 3 /* per s */
+#define SDR_UNIT_RATE_MIN 4 /* per min */
+#define SDR_UNIT_RATE_HR 5 /* per hour */
+#define SDR_UNIT_RATE_DAY 6 /* per day */
+#define SDR_UNIT_RATE_RSVD 7 /* reserved */
+/* IPMI 2.0, Table 43-1, byte 21[2:1] Modifier Unit */
+#define SDR_UNIT_MOD_NONE 0 /* none */
+#define SDR_UNIT_MOD_DIV 1 /* Basic Unit / Modifier Unit */
+#define SDR_UNIT_MOD_MUL 2 /* Basic Unit * Mofifier Unit */
+#define SDR_UNIT_MOD_RSVD 3 /* Reserved */
+/* IPMI 2.0, Table 43-1, byte 21[0] Percentage */
+#define SDR_UNIT_PCT_NO 0
+#define SDR_UNIT_PCT_YES 1
+
 	struct {
 #if WORDS_BIGENDIAN
 		uint8_t analog:2;
@@ -394,8 +418,8 @@ struct sdr_record_common_sensor {
 		uint8_t analog:2;
 #endif
 		struct {
-			uint8_t base;
-			uint8_t modifier;
+			uint8_t base; /* Base unit type code per IPMI 2.0 Table 43-15 */
+			uint8_t modifier; /* Modifier unit type code per Table 43-15 */
 		} ATTRIBUTE_PACKING type;
 	} ATTRIBUTE_PACKING unit;
 } ATTRIBUTE_PACKING;
@@ -833,8 +857,8 @@ void ipmi_sdr_print_sensor_hysteresis(struct sdr_record_common_sensor *sensor,
 		 struct sdr_record_full_sensor   *full,
 		 uint8_t hysteresis_value,
 		 const char *hdrstr);
-const char *ipmi_sdr_get_unit_string(uint8_t pct, uint8_t type,
-				      uint8_t base, uint8_t modifier);
+const char *ipmi_sdr_get_unit_string(bool pct, uint8_t type,
+                                     uint8_t base, uint8_t modifier);
 struct sensor_reading *
 ipmi_sdr_read_sensor_value(struct ipmi_intf *intf,
 		struct sdr_record_common_sensor *sensor,
diff --git a/lib/ipmi_sdr.c b/lib/ipmi_sdr.c
index eb40b36..b43765a 100644
--- a/lib/ipmi_sdr.c
+++ b/lib/ipmi_sdr.c
@@ -68,8 +68,9 @@ static struct sdr_record_list *sdr_list_head = NULL;
 static struct sdr_record_list *sdr_list_tail = NULL;
 static struct ipmi_sdr_iterator *sdr_list_itr = NULL;
 
-/* unit description codes (IPMI v1.5 section 37.16) */
-#define UNIT_MAX	0x90
+/* IPMI 2.0 Table 43-15, Sensor Unit Type Codes */
+#define UNIT_TYPE_MAX 92 /* This is the ID of "grams" */
+#define UNIT_TYPE_LONGEST_NAME 19 /* This is the length of "color temp deg K" */
 static const char *unit_desc[] = {
 	"unspecified",
 	"degrees C",
@@ -161,7 +162,9 @@ static const char *unit_desc[] = {
 	"characters",
 	"error",
 	"correctable error",
-	"uncorrectable error"
+	"uncorrectable error",
+	"fatal error",
+	"grams"
 };
 
 /* sensor type codes (IPMI v1.5 table 36.3)
@@ -220,35 +223,60 @@ void printf_sdr_usage();
 uint16_t
 ipmi_intf_get_max_response_data_size(struct ipmi_intf * intf);
 
-/* ipmi_sdr_get_unit_string  -  return units for base/modifier
+/** ipmi_sdr_get_unit_string  -  return units for base/modifier
  *
- * @pct:	units are a percentage
- * @type:	unit type
- * @base:	base
- * @modifier:	modifier
+ * @param[in] pct       Indicates that units are a percentage
+ * @param[in] relation  Modifier unit to base unit relation
+ *                      (SDR_UNIT_MOD_NONE, SDR_UNIT_MOD_MUL,
+ *                      or SDR_UNIT_MOD_DIV)
+ * @param[in] base      The base unit type id
+ * @param[in] modifier  The modifier unit type id
  *
- * returns pointer to static string
+ * @returns a pointer to static string
  */
 const char *
-ipmi_sdr_get_unit_string(uint8_t pct, uint8_t type, uint8_t base, uint8_t modifier)
+ipmi_sdr_get_unit_string(bool pct, uint8_t relation,
+                         uint8_t base, uint8_t modifier)
 {
-	static char unitstr[16];
+	/*
+	 * Twice as long as the longest possible unit name, plus
+	 * two characters for '%' and relation (either '*' or '/'),
+	 * plus the terminating null-byte.
+	 */
+	static char unitstr[2 * UNIT_TYPE_LONGEST_NAME + 2 + 1];
+
 	/*
 	 * By default, if units are supposed to be percent, we will pre-pend
 	 * the percent string  to the textual representation of the units.
 	 */
-	char *pctstr = pct ? "% " : "";
-	memset(unitstr, 0, sizeof (unitstr));
-	switch (type) {
-	case 2:
-		snprintf(unitstr, sizeof (unitstr), "%s%s * %s",
-			 pctstr, unit_desc[base], unit_desc[modifier]);
+	const char *pctstr = pct ? "% " : "";
+	const char *basestr;
+	const char *modstr;
+
+	if (base <= UNIT_TYPE_MAX) {
+		basestr = unit_desc[base];
+	}
+	else {
+		basestr = "invalid";
+	}
+
+	if (modifier <= UNIT_TYPE_MAX) {
+		modstr = unit_desc[base];
+	}
+	else {
+		modstr = "invalid";
+	}
+
+	switch (relation) {
+	case SDR_UNIT_MOD_MUL:
+		snprintf(unitstr, sizeof (unitstr), "%s%s*%s",
+			 pctstr, basestr, modstr);
 		break;
-	case 1:
+	case SDR_UNIT_MOD_DIV:
 		snprintf(unitstr, sizeof (unitstr), "%s%s/%s",
-			 pctstr, unit_desc[base], unit_desc[modifier]);
+			 pctstr, basestr, modstr);
 		break;
-	case 0:
+	case SDR_UNIT_MOD_NONE:
 	default:
 		/*
 		 * Display the text "percent" only when the Base unit is
@@ -257,8 +285,8 @@ ipmi_sdr_get_unit_string(uint8_t pct, uint8_t type, uint8_t base, uint8_t modifi
 		if (base == 0 && pct) {
 			snprintf(unitstr, sizeof(unitstr), "percent");
 		} else {
-			snprintf(unitstr, sizeof (unitstr), "%s%s", 
-				pctstr, unit_desc[base]);
+			snprintf(unitstr, sizeof (unitstr), "%s%s",
+			         pctstr, basestr);
 		}
 		break;
 	}
-- 
2.40.1