[PATCH] staging: nvec: fix out-of-bounds string copies

[Dang Vu Duc Hien]

The function `nvec_power_bat_notifier()` assumes that `res->length` is
both >= 2 and within the battery info buffers.
Technically, `res->length` is provided by the embedded controller (EC)
and is unverified (ie. a malformed response). If `res->length` < 2, the
expression `res->length - 2` evaluates to a massive size_t value which
is passed to `memcpy()`, causing severe memory corruption. Conversely,
`res->length - 2` being larger than the buffer also causes an
out-of-bounds write.

Introduce helper function `nvec_bat_str_copy()` to safely handle these
string copies by:
1. Ensuring that it returns an error to the upper layer along with a
   `dev_warn()` message when `res->length` < 2.
2. Ensuring that it provides toleration by copying as much as it can
   along with a `dev_warn()` when `res->length - 2` is too big.
3. Guaranteeing a properly sized, null-terminated destination string if
   the copy succeeds.

Signed-off-by: Dang Vu Duc Hien <dvdh12707@xxxxxxxxx>
---
Note: Coincidentally, I have been working on this exact issue earlier
and had this patch prepared just before Lucas Jeffrey submitted his RFC.
I am sending it here to answer his protocol validation question with code.
---
 drivers/staging/nvec/nvec_power.c | 41 ++++++++++++++++++++++++++-----
 1 file changed, 35 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/nvec/nvec_power.c b/drivers/staging/nvec/nvec_power.c
index c514d51a9e57..12699515356d 100644
--- a/drivers/staging/nvec/nvec_power.c
+++ b/drivers/staging/nvec/nvec_power.c
@@ -117,6 +117,32 @@ static void get_bat_mfg_data(struct nvec_power *power)
 	}
 }
 
+static int nvec_bat_str_copy(struct nvec_power *power,
+			     struct bat_response *res, char *dst,
+			     unsigned int dst_size, const char *prop_name)
+{
+	unsigned int len;
+
+	if (res->length < 2) {
+		dev_warn(power->nvec->dev,
+			 "battery %s response length too short (%u)\n",
+			 prop_name, res->length);
+		return -EINVAL;
+	}
+
+	len = res->length - 2;
+	if (len >= dst_size) {
+		dev_warn(power->nvec->dev,
+			 "battery %s string too long (%u)\n",
+			 prop_name, len);
+		len = dst_size - 1;
+	}
+
+	memcpy(dst, &res->plc, len);
+	dst[len] = '\0';
+	return 0;
+}
+
 static int nvec_power_bat_notifier(struct notifier_block *nb,
 				   unsigned long event_type, void *data)
 {
@@ -193,16 +219,19 @@ static int nvec_power_bat_notifier(struct notifier_block *nb,
 		power->bat_temperature = res->plu - 2732;
 		break;
 	case MANUFACTURER:
-		memcpy(power->bat_manu, &res->plc, res->length - 2);
-		power->bat_manu[res->length - 2] = '\0';
+		if (nvec_bat_str_copy(power, res, power->bat_manu,
+				      sizeof(power->bat_manu), "manufacturer") < 0)
+			break;
 		break;
 	case MODEL:
-		memcpy(power->bat_model, &res->plc, res->length - 2);
-		power->bat_model[res->length - 2] = '\0';
+		if (nvec_bat_str_copy(power, res, power->bat_model,
+				      sizeof(power->bat_model), "model") < 0)
+			break;
 		break;
 	case TYPE:
-		memcpy(power->bat_type, &res->plc, res->length - 2);
-		power->bat_type[res->length - 2] = '\0';
+		if (nvec_bat_str_copy(power, res, power->bat_type,
+				      sizeof(power->bat_type), "type") < 0)
+			break;
 		/*
 		 * This differs a little from the spec fill in more if you find
 		 * some.
-- 
2.55.0