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