From e80f277f4c268d69c162123bc8cbb1819224cea2 Mon Sep 17 00:00:00 2001

From: Richard Hughes <richard@hughsie.com>

Date: Wed, 10 Feb 2021 13:22:59 +0000

Subject: [PATCH 12/12] goodix-moc: Fix several places where the plugin code

 might crash

Fixes https://github.com/fwupd/fwupd/issues/2850

---

 plugins/goodix-moc/fu-goodixmoc-common.c |  83 ----------------

 plugins/goodix-moc/fu-goodixmoc-common.h |  19 +---

 plugins/goodix-moc/fu-goodixmoc-device.c | 120 +++++++++++++----------

 plugins/goodix-moc/meson.build           |   1 -

 4 files changed, 72 insertions(+), 151 deletions(-)

 delete mode 100644 plugins/goodix-moc/fu-goodixmoc-common.c

diff --git plugins/goodix-moc/fu-goodixmoc-common.c plugins/goodix-moc/fu-goodixmoc-common.c

deleted file mode 100644

index 7c81434d..00000000

--- plugins/goodix-moc/fu-goodixmoc-common.c

+++ /dev/null

@@ -1,83 +0,0 @@

-/*

- * Copyright (C) 2016 Richard Hughes <richard@hughsie.com>

- * Copyright (C) 2020 boger wang <boger@goodix.com>

- *

- * SPDX-License-Identifier: LGPL-2.1+

- */

-

-#include "config.h"

-

-#include <fwupd.h>

-#include <string.h>

-

-#include "fu-common.h"

-#include "fu-goodixmoc-common.h"

-

-void

-fu_goodixmoc_build_header (GxfpPkgHeader *pheader,

-			   guint16	  len,

-			   guint8	  cmd0,

-			   guint8	  cmd1,

-			   GxPkgType	  type)

-{

-	static guint8 dummy_seq = 0;

-

-	g_return_if_fail (pheader != NULL);

-

-	pheader->cmd0 = (cmd0);

-	pheader->cmd1 = (cmd1);

-	pheader->pkg_flag = (guint8)type;

-	pheader->reserved = dummy_seq++;

-	pheader->len = len + GX_SIZE_CRC32;

-	pheader->crc8 = fu_common_crc8 ((guint8 *)pheader, 6);

-	pheader->rev_crc8 = ~pheader->crc8;

-}

-

-gboolean

-fu_goodixmoc_parse_header (guint8 *buf, guint32 bufsz,

-			   GxfpPkgHeader *pheader, GError **error)

-{

-	g_return_val_if_fail (buf != NULL, FALSE);

-	g_return_val_if_fail (pheader != NULL, FALSE);

-

-	if (!fu_memcpy_safe ((guint8 *) &pheader, sizeof(*pheader), 0x0,	/* dst */

-			     buf, bufsz, 0x01,					/* src */

-			     sizeof(*pheader), error))

-		return FALSE;

-	memcpy (pheader, buf, sizeof(*pheader));

-	pheader->len = GUINT16_FROM_LE(*(buf + 4));

-	pheader->len -= GX_SIZE_CRC32;

-	return TRUE;

-}

-

-gboolean

-fu_goodixmoc_parse_body (guint8 cmd, guint8 *buf, guint32 bufsz,

-			 GxfpCmdResp *presp, GError **error)

-{

-	g_return_val_if_fail (buf != NULL, FALSE);

-	g_return_val_if_fail (presp != NULL, FALSE);

-

-	presp->result = buf[0];

-	switch (cmd) {

-	case GX_CMD_ACK:

-		if (bufsz == 0) {

-			g_set_error_literal (error,

-					     FWUPD_ERROR,

-					     FWUPD_ERROR_INTERNAL,

-					     "invalid bufsz");

-			return FALSE;

-		}

-		presp->ack_msg.cmd = buf[1];

-		break;

-	case GX_CMD_VERSION:

-		if (!fu_memcpy_safe ((guint8 *) &presp->version_info,

-				     sizeof(presp->version_info), 0x0,		/* dst */

-				     buf, bufsz, 0x01,				/* src */

-				     sizeof(GxfpVersiomInfo), error))

-			return FALSE;

-		break;

-	default:

-		break;

-	}

-	return TRUE;

-}

diff --git plugins/goodix-moc/fu-goodixmoc-common.h plugins/goodix-moc/fu-goodixmoc-common.h

index 4bbdc0c8..c4b69954 100644

--- plugins/goodix-moc/fu-goodixmoc-common.h

+++ plugins/goodix-moc/fu-goodixmoc-common.h

@@ -35,7 +35,7 @@ typedef struct {

 	guint8		 protocol[8];

 	guint8		 flashVersion[8];

 	guint8		 reserved[62];

-} GxfpVersiomInfo;

+} GxfpVersionInfo;

 typedef struct {

 	guint8		 cmd;

@@ -46,7 +46,7 @@ typedef struct {

 	guint8		 result;

 	union {

 		GxfpAckMsg	ack_msg;

-		GxfpVersiomInfo version_info;

+		GxfpVersionInfo version_info;

 	};

 } GxfpCmdResp;

@@ -64,18 +64,3 @@ typedef struct __attribute__((__packed__)) {

 	guint8		 crc8;

 	guint8		 rev_crc8;

 } GxfpPkgHeader;

-

-void		 fu_goodixmoc_build_header	(GxfpPkgHeader	*pheader,

-						 guint16	 len,

-						 guint8		 cmd0,

-						 guint8		 cmd1,

-						 GxPkgType	 type);

-gboolean	 fu_goodixmoc_parse_header	(guint8		*buf,

-						 guint32	 bufsz,

-						 GxfpPkgHeader	*pheader,

-						 GError		**error);

-gboolean	 fu_goodixmoc_parse_body	(guint8		 cmd,

-						 guint8		*buf,

-						 guint32	 bufsz,

-						 GxfpCmdResp	*presp,

-						 GError		**error);

diff --git plugins/goodix-moc/fu-goodixmoc-device.c plugins/goodix-moc/fu-goodixmoc-device.c

index f216aec7..3d359dab 100644

--- plugins/goodix-moc/fu-goodixmoc-device.c

+++ plugins/goodix-moc/fu-goodixmoc-device.c

@@ -14,6 +14,7 @@

 struct _FuGoodixMocDevice {

 	FuUsbDevice	parent_instance;

+	guint8		dummy_seq;

 };

 G_DEFINE_TYPE (FuGoodixMocDevice, fu_goodixmoc_device, FU_TYPE_USB_DEVICE)

@@ -27,26 +28,34 @@ G_DEFINE_TYPE (FuGoodixMocDevice, fu_goodixmoc_device, FU_TYPE_USB_DEVICE)

 #define GX_FLASH_TRANSFER_BLOCK_SIZE	1000	/* 1000 */

 static gboolean

-goodixmoc_device_cmd_send (GUsbDevice *usbdevice,

+goodixmoc_device_cmd_send (FuGoodixMocDevice *self,

 			   guint8      cmd0,

 			   guint8      cmd1,

 			   GxPkgType   type,

 			   GByteArray *req,

 			   GError    **error)

 {

-	GxfpPkgHeader header = { 0 };

-	guint32 crc_actual = 0;

+	GUsbDevice *usb_device = fu_usb_device_get_dev (FU_USB_DEVICE (self));

+	guint32 crc_all = 0;

+	guint32 crc_hdr = 0;

 	gsize actual_len = 0;

 	g_autoptr(GByteArray) buf = g_byte_array_new ();

-	fu_goodixmoc_build_header (&header, req->len, cmd0, cmd1, type);

-	g_byte_array_append (buf, (guint8 *)&header, sizeof(header));

+	/* build header */

+	fu_byte_array_append_uint8 (buf, cmd0);

+	fu_byte_array_append_uint8 (buf, cmd1);

+	fu_byte_array_append_uint8 (buf, type);			/* pkg_flag */

+	fu_byte_array_append_uint8 (buf, self->dummy_seq++);	/* reserved */

+	fu_byte_array_append_uint16 (buf, req->len + GX_SIZE_CRC32, G_LITTLE_ENDIAN);

+	crc_hdr = fu_common_crc8 (buf->data, buf->len);

+	fu_byte_array_append_uint8 (buf, crc_hdr);

+	fu_byte_array_append_uint8 (buf, ~crc_hdr);

 	g_byte_array_append (buf, req->data, req->len);

-	crc_actual = fu_common_crc32 (buf->data, sizeof(header) + req->len);

-	fu_byte_array_append_uint32 (buf, crc_actual, G_LITTLE_ENDIAN);

+	crc_all = fu_common_crc32 (buf->data, buf->len);

+	fu_byte_array_append_uint32 (buf, crc_all, G_LITTLE_ENDIAN);

 	/* send zero length package */

-	if (!g_usb_device_bulk_transfer (usbdevice,

+	if (!g_usb_device_bulk_transfer (usb_device,

 					 GX_USB_BULK_EP_OUT,

 					 NULL,

 					 0,

@@ -62,7 +71,7 @@ goodixmoc_device_cmd_send (GUsbDevice *usbdevice,

 	}

 	/* send data */

-	if (!g_usb_device_bulk_transfer (usbdevice,

+	if (!g_usb_device_bulk_transfer (usb_device,

 					 GX_USB_BULK_EP_OUT,

 					 buf->data,

 					 buf->len,

@@ -84,12 +93,12 @@ goodixmoc_device_cmd_send (GUsbDevice *usbdevice,

 }

 static gboolean

-goodixmoc_device_cmd_recv (GUsbDevice  *usbdevice,

+goodixmoc_device_cmd_recv (FuGoodixMocDevice *self,

 			   GxfpCmdResp *presponse,

 			   gboolean     data_reply,

 			   GError     **error)

 {

-	GxfpPkgHeader header = { 0 };

+	GUsbDevice *usb_device = fu_usb_device_get_dev (FU_USB_DEVICE (self));

 	guint32 crc_actual = 0;

 	guint32 crc_calculated = 0;

 	gsize actual_len = 0;

@@ -102,9 +111,11 @@ goodixmoc_device_cmd_recv (GUsbDevice  *usbdevice,

 	* | zlp | ack | zlp | data |

 	*/

 	while (1) {

+		guint16 header_len = 0x0;

+		guint8 header_cmd0 = 0x0;

 		g_autoptr(GByteArray) reply = g_byte_array_new ();

 		fu_byte_array_set_size (reply, GX_FLASH_TRANSFER_BLOCK_SIZE);

-		if (!g_usb_device_bulk_transfer (usbdevice,

+		if (!g_usb_device_bulk_transfer (usb_device,

 						 GX_USB_BULK_EP_IN,

 						 reply->data,

 						 reply->len,

@@ -125,12 +136,14 @@ goodixmoc_device_cmd_recv (GUsbDevice  *usbdevice,

 		}

 		/* parse package header */

-		if (!fu_goodixmoc_parse_header (reply->data,

-						actual_len,

-						&header,

-						error))

+		if (!fu_common_read_uint8_safe (reply->data, reply->len, 0x0,

+						&header_cmd0, error))

+			return FALSE;

+		if (!fu_common_read_uint16_safe	(reply->data, reply->len, 0x4,

+						 &header_len, G_LITTLE_ENDIAN,

+						 error))

 			return FALSE;

-		offset = sizeof(header) + header.len;

+		offset = sizeof(GxfpPkgHeader) + header_len - GX_SIZE_CRC32;

 		crc_actual = fu_common_crc32 (reply->data, offset);

 		if (!fu_common_read_uint32_safe	(reply->data,

 						 reply->len,

@@ -149,15 +162,33 @@ goodixmoc_device_cmd_recv (GUsbDevice  *usbdevice,

 		}

 		/* parse package data */

-		if (!fu_goodixmoc_parse_body (header.cmd0,

-					      reply->data + sizeof(header),

-					      header.len,

-					      presponse,

-					      error))

+		if (!fu_common_read_uint8_safe (reply->data, reply->len,

+						sizeof(GxfpPkgHeader) + 0x00,

+						&presponse->result, error))

 			return FALSE;

+		if (header_cmd0 == GX_CMD_ACK) {

+			if (header_len == 0) {

+				g_set_error_literal (error,

+						     FWUPD_ERROR,

+						     FWUPD_ERROR_INTERNAL,

+						     "invalid bufsz");

+				return FALSE;

+			}

+			if (!fu_common_read_uint8_safe (reply->data, reply->len,

+							sizeof(GxfpPkgHeader) + 0x01,

+							&presponse->ack_msg.cmd, error))

+				return FALSE;

+		} else if (header_cmd0 == GX_CMD_VERSION) {

+			if (!fu_memcpy_safe ((guint8 *) &presponse->version_info,

+					     sizeof(presponse->version_info), 0x0,	/* dst */

+					     reply->data, reply->len,

+					     sizeof(GxfpPkgHeader) + 0x01,		/* src */

+					     sizeof(GxfpVersionInfo), error))

+				return FALSE;

+		}

 		/* continue after ack received */

-		if (header.cmd0 == GX_CMD_ACK && data_reply)

+		if (header_cmd0 == GX_CMD_ACK && data_reply)

 			continue;

 		break;

 	}

@@ -176,36 +207,27 @@ fu_goodixmoc_device_cmd_xfer (FuGoodixMocDevice	*device,

 			      gboolean		 data_reply,

 			      GError		**error)

 {

-	GUsbDevice *usb_device = fu_usb_device_get_dev (FU_USB_DEVICE(device));

-	if (!goodixmoc_device_cmd_send (usb_device, cmd0, cmd1, type, req, error))

+	FuGoodixMocDevice *self = FU_GOODIXMOC_DEVICE(device);

+	if (!goodixmoc_device_cmd_send (self, cmd0, cmd1, type, req, error))

 		return FALSE;

-	return goodixmoc_device_cmd_recv (usb_device, presponse, data_reply, error);

+	return goodixmoc_device_cmd_recv (self, presponse, data_reply, error);

 }

-static gchar *

-fu_goodixmoc_device_get_version (FuGoodixMocDevice *self, GError **error)

+static gboolean

+fu_goodixmoc_device_setup_version (FuGoodixMocDevice *self, GError **error)

 {

 	GxfpCmdResp rsp = { 0 };

-	gchar ver[9] = { 0 };

-	guint8 dummy = 0;

+	g_autofree gchar *version = NULL;

 	g_autoptr(GByteArray) req = g_byte_array_new ();

-	fu_byte_array_append_uint8 (req, dummy);

+	fu_byte_array_append_uint8 (req, 0);	/* dummy */

 	if (!fu_goodixmoc_device_cmd_xfer (self, GX_CMD_VERSION, GX_CMD1_DEFAULT,

-					  GX_PKG_TYPE_EOP,

-					  req,

-					  &rsp,

-					  TRUE,

-					  error))

-		return NULL;

-	if (!fu_memcpy_safe ((guint8 *) ver, sizeof(ver), 0x0,

-			      rsp.version_info.fwversion,

-			      sizeof(rsp.version_info.fwversion),

-			      0x0,

-			      sizeof(rsp.version_info.fwversion),

-			      error))

-		return NULL;

-	return g_strndup (ver, sizeof(ver));

+					  GX_PKG_TYPE_EOP, req, &rsp, TRUE, error))

+		return FALSE;

+	version = g_strndup ((const gchar *) rsp.version_info.fwversion,

+			     sizeof(rsp.version_info.fwversion));

+	fu_device_set_version (FU_DEVICE (self), version);

+	return TRUE;

 }

 static gboolean

@@ -281,15 +303,13 @@ fu_goodixmoc_device_open (FuUsbDevice *device, GError **error)

 static gboolean

 fu_goodixmoc_device_setup (FuDevice *device, GError **error)

 {

-	FuGoodixMocDevice *self = FU_GOODIXMOC_DEVICE(device);

-	g_autofree gchar *version = NULL;

+	FuGoodixMocDevice *self = FU_GOODIXMOC_DEVICE (device);

-	version = fu_goodixmoc_device_get_version (self, error);

-	if (version == NULL) {

+	/* ensure version */

+	if (!fu_goodixmoc_device_setup_version (self, error)) {

 		g_prefix_error (error, "failed to get firmware version: ");

 		return FALSE;

 	}

-	fu_device_set_version (device, version);

 	/* success */

 	return TRUE;

diff --git plugins/goodix-moc/meson.build plugins/goodix-moc/meson.build

index 4e1287e4..178b35d8 100644

--- plugins/goodix-moc/meson.build

+++ plugins/goodix-moc/meson.build

@@ -9,7 +9,6 @@ install_data([

 shared_module('fu_plugin_goodixmoc',

   fu_hash,

   sources : [

-    'fu-goodixmoc-common.c',

     'fu-goodixmoc-device.c',

     'fu-plugin-goodixmoc.c',

   ],

-- 

2.29.2