From 05e3377d11efe84d4da7ab8a27f5e460ea2a4b72 Mon Sep 17 00:00:00 2001 From: Richard Hughes Date: Tue, 8 Dec 2020 18:37:02 +0000 Subject: [PATCH] trivial: Add fu_common_bytes_new_offset() This is a safer version of g_bytes_new_from_bytes() which returns a GError if the offsets are invalid rather than emitting a critical warning. This prevents a critical warning and potential crash when parsing invalid bcm57xx firmware. --- libfwupdplugin/fu-common.c | 36 +++++++++++++++++++ libfwupdplugin/fu-common.h | 4 +++ libfwupdplugin/fu-dfu-firmware.c | 4 ++- libfwupdplugin/fu-firmware-image.c | 13 +++++-- libfwupdplugin/fu-fmap-firmware.c | 10 ++++-- libfwupdplugin/fwupdplugin.map | 6 ++++ plugins/bcm57xx/fu-bcm57xx-dict-image.c | 6 +++- plugins/bcm57xx/fu-bcm57xx-firmware.c | 40 +++++++++++++++++---- plugins/bcm57xx/fu-bcm57xx-stage1-image.c | 6 +++- plugins/bcm57xx/fu-bcm57xx-stage2-image.c | 6 +++- plugins/cros-ec/fu-cros-ec-usb-device.c | 9 +++-- plugins/dfu/dfu-target-stm.c | 19 +++++++--- plugins/dfu/dfu-target.c | 7 +++- plugins/ebitdo/fu-ebitdo-firmware.c | 14 ++++++-- plugins/thunderbolt/fu-thunderbolt-device.c | 9 +++-- plugins/uefi/fu-uefi-device.c | 4 +-- 16 files changed, 161 insertions(+), 32 deletions(-) diff --git a/libfwupdplugin/fu-common.c b/libfwupdplugin/fu-common.c index eb1508765..a3cc2d82a 100644 --- a/libfwupdplugin/fu-common.c +++ b/libfwupdplugin/fu-common.c @@ -1632,6 +1632,42 @@ fu_common_bytes_pad (GBytes *bytes, gsize sz) return g_bytes_ref (bytes); } +/** + * fu_common_bytes_new_offset: + * @bytes: a #GBytes + * @offset: where subsection starts at + * @length: length of subsection + * @error: A #GError or %NULL + * + * Creates a #GBytes which is a subsection of another #GBytes. + * + * Return value: (transfer full): a #GBytes, or #NULL if range is invalid + * + * Since: 1.5.4 + **/ +GBytes * +fu_common_bytes_new_offset (GBytes *bytes, + gsize offset, + gsize length, + GError **error) +{ + g_return_val_if_fail (bytes != NULL, NULL); + + /* sanity check */ + if (offset + length > g_bytes_get_size (bytes)) { + g_set_error (error, + G_IO_ERROR, + G_IO_ERROR_INVALID_DATA, + "cannot create bytes @0x%02x for 0x%02x " + "as buffer only 0x%04x bytes in size", + (guint) offset, + (guint) length, + (guint) g_bytes_get_size (bytes)); + return NULL; + } + return g_bytes_new_from_bytes (bytes, offset, length); +} + /** * fu_common_realpath: * @filename: a filename diff --git a/libfwupdplugin/fu-common.h b/libfwupdplugin/fu-common.h index 8a454fa0d..5896207ee 100644 --- a/libfwupdplugin/fu-common.h +++ b/libfwupdplugin/fu-common.h @@ -155,6 +155,10 @@ gboolean fu_common_bytes_compare_raw (const guint8 *buf1, GError **error); GBytes *fu_common_bytes_pad (GBytes *bytes, gsize sz); +GBytes *fu_common_bytes_new_offset (GBytes *bytes, + gsize offset, + gsize length, + GError **error); gsize fu_common_strwidth (const gchar *text); gboolean fu_memcpy_safe (guint8 *dst, gsize dst_sz, diff --git a/libfwupdplugin/fu-dfu-firmware.c b/libfwupdplugin/fu-dfu-firmware.c index 17c28d14c..8247e2a73 100644 --- a/libfwupdplugin/fu-dfu-firmware.c +++ b/libfwupdplugin/fu-dfu-firmware.c @@ -264,7 +264,9 @@ fu_dfu_firmware_parse (FuFirmware *firmware, } /* success */ - contents = g_bytes_new_from_bytes (fw, 0, len - ftr.len); + contents = fu_common_bytes_new_offset (fw, 0, len - ftr.len, error); + if (contents == NULL) + return FALSE; image = fu_firmware_image_new (contents); fu_firmware_add_image (firmware, image); return TRUE; diff --git a/libfwupdplugin/fu-firmware-image.c b/libfwupdplugin/fu-firmware-image.c index f4f1795bc..e0b347bbe 100644 --- a/libfwupdplugin/fu-firmware-image.c +++ b/libfwupdplugin/fu-firmware-image.c @@ -479,11 +479,18 @@ fu_firmware_image_write_chunk (FuFirmwareImage *self, /* if we have less data than requested */ chunk_left = g_bytes_get_size (priv->bytes) - offset; - if (chunk_sz_max > chunk_left) - return g_bytes_new_from_bytes (priv->bytes, offset, chunk_left); + if (chunk_sz_max > chunk_left) { + return fu_common_bytes_new_offset (priv->bytes, + offset, + chunk_left, + error); + } /* check chunk */ - return g_bytes_new_from_bytes (priv->bytes, offset, chunk_sz_max); + return fu_common_bytes_new_offset (priv->bytes, + offset, + chunk_sz_max, + error); } void diff --git a/libfwupdplugin/fu-fmap-firmware.c b/libfwupdplugin/fu-fmap-firmware.c index 2683b1567..48412f2ea 100644 --- a/libfwupdplugin/fu-fmap-firmware.c +++ b/libfwupdplugin/fu-fmap-firmware.c @@ -6,6 +6,7 @@ #include "config.h" +#include "fu-common.h" #include "fu-fmap-firmware.h" #define FMAP_SIGNATURE "__FMAP__" @@ -217,9 +218,12 @@ fu_fmap_firmware_parse (FuFirmware *firmware, g_autoptr(GBytes) bytes = NULL; img = fu_firmware_image_new (NULL); - bytes = g_bytes_new_from_bytes (fw, - (gsize) area->offset, - (gsize) area->size); + bytes = fu_common_bytes_new_offset (fw, + (gsize) area->offset, + (gsize) area->size, + error); + if (bytes == NULL) + return FALSE; fu_firmware_image_set_id (img, (const gchar *) area->name); fu_firmware_image_set_idx (img, i + 1); fu_firmware_image_set_addr (img, (guint64) area->offset); diff --git a/libfwupdplugin/fwupdplugin.map b/libfwupdplugin/fwupdplugin.map index 3ba240d7a..e3d5440b5 100644 --- a/libfwupdplugin/fwupdplugin.map +++ b/libfwupdplugin/fwupdplugin.map @@ -693,3 +693,9 @@ LIBFWUPDPLUGIN_1.5.3 { fu_udev_device_get_driver; local: *; } LIBFWUPDPLUGIN_1.5.2; + +LIBFWUPDPLUGIN_1.5.4 { + global: + fu_common_bytes_new_offset; + local: *; +} LIBFWUPDPLUGIN_1.5.3; diff --git a/plugins/bcm57xx/fu-bcm57xx-dict-image.c b/plugins/bcm57xx/fu-bcm57xx-dict-image.c index cddc74933..ee8afa89b 100644 --- a/plugins/bcm57xx/fu-bcm57xx-dict-image.c +++ b/plugins/bcm57xx/fu-bcm57xx-dict-image.c @@ -40,7 +40,11 @@ fu_bcm57xx_dict_image_parse (FuFirmwareImage *image, if (!fu_bcm57xx_verify_crc (fw, error)) return FALSE; } - fw_nocrc = g_bytes_new_from_bytes (fw, 0x0, g_bytes_get_size (fw) - sizeof(guint32)); + fw_nocrc = fu_common_bytes_new_offset (fw, 0x0, + g_bytes_get_size (fw) - sizeof(guint32), + error); + if (fw_nocrc == NULL) + return FALSE; fu_firmware_image_set_bytes (image, fw_nocrc); return TRUE; } diff --git a/plugins/bcm57xx/fu-bcm57xx-firmware.c b/plugins/bcm57xx/fu-bcm57xx-firmware.c index cd728c786..4f2019511 100644 --- a/plugins/bcm57xx/fu-bcm57xx-firmware.c +++ b/plugins/bcm57xx/fu-bcm57xx-firmware.c @@ -130,7 +130,9 @@ fu_bcm57xx_firmware_parse_stage1 (FuBcm57xxFirmware *self, } /* verify CRC */ - blob = g_bytes_new_from_bytes (fw, stage1_off, stage1_sz); + blob = fu_common_bytes_new_offset (fw, stage1_off, stage1_sz, error); + if (blob == NULL) + return NULL; if (!fu_firmware_image_parse (img, blob, flags, error)) return NULL; @@ -174,7 +176,9 @@ fu_bcm57xx_firmware_parse_stage2 (FuBcm57xxFirmware *self, } /* verify CRC */ - blob = g_bytes_new_from_bytes (fw, stage2_off + 0x8, stage2_sz); + blob = fu_common_bytes_new_offset (fw, stage2_off + 0x8, stage2_sz, error); + if (blob == NULL) + return NULL; if (!fu_firmware_image_parse (img, blob, flags, error)) return NULL; @@ -240,7 +244,9 @@ fu_bcm57xx_firmware_parse_dict (FuBcm57xxFirmware *self, GBytes *fw, guint idx, (guint) dict_sz, (guint) dict_off); return FALSE; } - blob = g_bytes_new_from_bytes (fw, dict_off, dict_sz); + blob = fu_common_bytes_new_offset (fw, dict_off, dict_sz, error); + if (blob == NULL) + return FALSE; if (!fu_firmware_image_parse (img, blob, flags, error)) return FALSE; @@ -312,14 +318,24 @@ fu_bcm57xx_firmware_parse (FuFirmware *firmware, self->source_padchar = buf[bufsz - 1]; /* NVRAM header */ - blob_header = g_bytes_new_from_bytes (fw, BCM_NVRAM_HEADER_BASE, BCM_NVRAM_HEADER_SZ); + blob_header = fu_common_bytes_new_offset (fw, + BCM_NVRAM_HEADER_BASE, + BCM_NVRAM_HEADER_SZ, + error); + if (blob_header == NULL) + return FALSE; if (!fu_bcm57xx_firmware_parse_header (self, blob_header, error)) { g_prefix_error (error, "failed to parse header: "); return FALSE; } /* info */ - blob_info = g_bytes_new_from_bytes (fw, BCM_NVRAM_INFO_BASE, BCM_NVRAM_INFO_SZ); + blob_info = fu_common_bytes_new_offset (fw, + BCM_NVRAM_INFO_BASE, + BCM_NVRAM_INFO_SZ, + error); + if (blob_info == NULL) + return FALSE; img_info = fu_bcm57xx_firmware_parse_info (self, blob_info, error); if (img_info == NULL) { g_prefix_error (error, "failed to parse info: "); @@ -329,14 +345,24 @@ fu_bcm57xx_firmware_parse (FuFirmware *firmware, fu_firmware_add_image (firmware, img_info); /* VPD */ - blob_vpd = g_bytes_new_from_bytes (fw, BCM_NVRAM_VPD_BASE, BCM_NVRAM_VPD_SZ); + blob_vpd = fu_common_bytes_new_offset (fw, + BCM_NVRAM_VPD_BASE, + BCM_NVRAM_VPD_SZ, + error); + if (blob_vpd == NULL) + return FALSE; img_vpd = fu_firmware_image_new (blob_vpd); fu_firmware_image_set_id (img_vpd, "vpd"); fu_firmware_image_set_offset (img_vpd, BCM_NVRAM_VPD_BASE); fu_firmware_add_image (firmware, img_vpd); /* info2 */ - blob_info2 = g_bytes_new_from_bytes (fw, BCM_NVRAM_INFO2_BASE, BCM_NVRAM_INFO2_SZ); + blob_info2 = fu_common_bytes_new_offset (fw, + BCM_NVRAM_INFO2_BASE, + BCM_NVRAM_INFO2_SZ, + error); + if (blob_info2 == NULL) + return FALSE; img_info2 = fu_firmware_image_new (blob_info2); fu_firmware_image_set_id (img_info2, "info2"); fu_firmware_image_set_offset (img_info2, BCM_NVRAM_INFO2_BASE); diff --git a/plugins/bcm57xx/fu-bcm57xx-stage1-image.c b/plugins/bcm57xx/fu-bcm57xx-stage1-image.c index 94fa6c884..28c353b5c 100644 --- a/plugins/bcm57xx/fu-bcm57xx-stage1-image.c +++ b/plugins/bcm57xx/fu-bcm57xx-stage1-image.c @@ -69,7 +69,11 @@ fu_bcm57xx_stage1_image_parse (FuFirmwareImage *image, fu_firmware_image_set_version (image, veritem->version); } - fw_nocrc = g_bytes_new_from_bytes (fw, 0x0, g_bytes_get_size (fw) - sizeof(guint32)); + fw_nocrc = fu_common_bytes_new_offset (fw, 0x0, + g_bytes_get_size (fw) - sizeof(guint32), + error); + if (fw_nocrc == NULL) + return FALSE; fu_firmware_image_set_bytes (image, fw_nocrc); return TRUE; } diff --git a/plugins/bcm57xx/fu-bcm57xx-stage2-image.c b/plugins/bcm57xx/fu-bcm57xx-stage2-image.c index 20025f1f2..577cbb1a1 100644 --- a/plugins/bcm57xx/fu-bcm57xx-stage2-image.c +++ b/plugins/bcm57xx/fu-bcm57xx-stage2-image.c @@ -28,7 +28,11 @@ fu_bcm57xx_stage2_image_parse (FuFirmwareImage *image, if (!fu_bcm57xx_verify_crc (fw, error)) return FALSE; } - fw_nocrc = g_bytes_new_from_bytes (fw, 0x0, g_bytes_get_size (fw) - sizeof(guint32)); + fw_nocrc = fu_common_bytes_new_offset (fw, 0x0, + g_bytes_get_size (fw) - sizeof(guint32), + error); + if (fw_nocrc == NULL) + return FALSE; fu_firmware_image_set_bytes (image, fw_nocrc); return TRUE; } diff --git a/plugins/cros-ec/fu-cros-ec-usb-device.c b/plugins/cros-ec/fu-cros-ec-usb-device.c index b9a2de7dd..5bf6f7e1a 100644 --- a/plugins/cros-ec/fu-cros-ec-usb-device.c +++ b/plugins/cros-ec/fu-cros-ec-usb-device.c @@ -479,9 +479,12 @@ fu_cros_ec_usb_device_transfer_block (FuDevice *device, gpointer user_data, return FALSE; } - block_bytes = g_bytes_new_from_bytes (block_info->image_bytes, - block_info->offset, - block_info->payload_size); + block_bytes = fu_common_bytes_new_offset (block_info->image_bytes, + block_info->offset, + block_info->payload_size, + error); + if (block_bytes == NULL) + return FALSE; chunks = fu_chunk_array_new_from_bytes (block_bytes, 0x00, 0x00, diff --git a/plugins/dfu/dfu-target-stm.c b/plugins/dfu/dfu-target-stm.c index 3857f9c05..faf027d10 100644 --- a/plugins/dfu/dfu-target-stm.c +++ b/plugins/dfu/dfu-target-stm.c @@ -194,10 +194,16 @@ dfu_target_stm_upload_element (DfuTarget *target, /* create new image */ contents = dfu_utils_bytes_join_array (chunks); - if (expected_size > 0) - contents_truncated = g_bytes_new_from_bytes (contents, 0, expected_size); - else + if (expected_size > 0) { + contents_truncated = fu_common_bytes_new_offset (contents, + 0, + expected_size, + error); + if (contents_truncated == NULL) + return NULL; + } else { contents_truncated = g_bytes_ref (contents); + } element = dfu_element_new (); dfu_element_set_contents (element, contents_truncated); dfu_element_set_address (element, address); @@ -347,7 +353,12 @@ dfu_target_stm_download_element (DfuTarget *target, length = g_bytes_get_size (bytes) - offset; if (length > transfer_size) length = transfer_size; - bytes_tmp = g_bytes_new_from_bytes (bytes, offset, length); + bytes_tmp = fu_common_bytes_new_offset (bytes, + offset, + length, + error); + if (bytes_tmp == NULL) + return FALSE; g_debug ("writing sector at 0x%04x (0x%" G_GSIZE_FORMAT ")", offset_dev, g_bytes_get_size (bytes_tmp)); diff --git a/plugins/dfu/dfu-target.c b/plugins/dfu/dfu-target.c index e9d13d856..b4099e1d8 100644 --- a/plugins/dfu/dfu-target.c +++ b/plugins/dfu/dfu-target.c @@ -1176,7 +1176,12 @@ dfu_target_download_element_dfu (DfuTarget *target, length = g_bytes_get_size (bytes) - offset; if (length > transfer_size) length = transfer_size; - bytes_tmp = g_bytes_new_from_bytes (bytes, offset, length); + bytes_tmp = fu_common_bytes_new_offset (bytes, + offset, + length, + error); + if (bytes_tmp == NULL) + return FALSE; } else { bytes_tmp = g_bytes_new (NULL, 0); } diff --git a/plugins/ebitdo/fu-ebitdo-firmware.c b/plugins/ebitdo/fu-ebitdo-firmware.c index 7dbae531e..facd982c8 100644 --- a/plugins/ebitdo/fu-ebitdo-firmware.c +++ b/plugins/ebitdo/fu-ebitdo-firmware.c @@ -6,6 +6,7 @@ #include "config.h" +#include "fu-common.h" #include "fu-ebitdo-firmware.h" struct _FuEbitdoFirmware { @@ -77,13 +78,22 @@ fu_ebitdo_firmware_parse (FuFirmware *firmware, fu_firmware_set_version (firmware, version); /* add header */ - fw_hdr = g_bytes_new_from_bytes (fw, 0x0, sizeof(FuEbitdoFirmwareHeader)); + fw_hdr = fu_common_bytes_new_offset (fw, 0x0, + sizeof(FuEbitdoFirmwareHeader), + error); + if (fw_hdr == NULL) + return FALSE; fu_firmware_image_set_id (img_hdr, FU_FIRMWARE_IMAGE_ID_HEADER); fu_firmware_image_set_bytes (img_hdr, fw_hdr); fu_firmware_add_image (firmware, img_hdr); /* add payload */ - fw_payload = g_bytes_new_from_bytes (fw, sizeof(FuEbitdoFirmwareHeader), payload_len); + fw_payload = fu_common_bytes_new_offset (fw, + sizeof(FuEbitdoFirmwareHeader), + payload_len, + error); + if (fw_payload == NULL) + return FALSE; fu_firmware_image_set_id (img_payload, FU_FIRMWARE_IMAGE_ID_PAYLOAD); fu_firmware_image_set_addr (img_payload, GUINT32_FROM_LE(hdr->destination_addr)); fu_firmware_image_set_bytes (img_payload, fw_payload); diff --git a/plugins/thunderbolt/fu-thunderbolt-device.c b/plugins/thunderbolt/fu-thunderbolt-device.c index 5fa22bdb2..c17cf6f86 100644 --- a/plugins/thunderbolt/fu-thunderbolt-device.c +++ b/plugins/thunderbolt/fu-thunderbolt-device.c @@ -604,9 +604,12 @@ fu_thunderbolt_device_write_data (FuThunderboltDevice *self, do { g_autoptr(GBytes) fw_data = NULL; - fw_data = g_bytes_new_from_bytes (blob_fw, - nwritten, - fw_size - nwritten); + fw_data = fu_common_bytes_new_offset (blob_fw, + nwritten, + fw_size - nwritten, + error); + if (fw_data == NULL) + return FALSE; n = g_output_stream_write_bytes (os, fw_data, diff --git a/plugins/uefi/fu-uefi-device.c b/plugins/uefi/fu-uefi-device.c index d5179cf31..ed3080bc4 100644 --- a/plugins/uefi/fu-uefi-device.c +++ b/plugins/uefi/fu-uefi-device.c @@ -356,10 +356,10 @@ fu_uefi_device_fixup_firmware (FuDevice *device, GBytes *fw, GError **error) /* ESRT header matches payload */ if (g_strcmp0 (fu_uefi_device_get_guid (self), guid_new) == 0) { g_debug ("ESRT matches payload GUID"); - return g_bytes_new_from_bytes (fw, 0, fw_length); + return g_bytes_ref (fw); /* Type that doesn't require a header */ } else if (!self->requires_header) { - return g_bytes_new_from_bytes (fw, 0, fw_length); + return g_bytes_ref (fw); /* Missing, add a header */ } else { guint header_size = getpagesize();