[PATCH v2] firewire: core: validate descriptor bounds in fw_core_add_descriptor()

[PATCH v2] firewire: core: validate descriptor bounds in fw_core_add_descriptor()

[Sreeraj S Kurup]

In fw_core_add_descriptor(), incoming descriptor data is processed without
prior bounds verification of individual sub-block headers. A malformed or
corrupted descriptor containing an oversized block length field can cause
the parsing loop to read past the end of the desc->data buffer, leading to
an out-of-bounds memory access.

Add validation logic at the start of fw_core_add_descriptor() to:
1. Reject empty descriptors (length == 0) or descriptors exceeding the
   maximum configuration ROM size (256 quadlets).
2. Validate each sub-block header's embedded length against the remaining
   descriptor length before advancing the loop pointer.
3. Ensure the inner loop explicitly increments by (block_len + 1) quadlets
   to prevent hangs or buffer overflows.

Signed-off-by: Sreeraj S Kurup <sreekuttan2156239@gmail.com>
---
v2:
 - Fixed tab indentation to 8-space tabs per kernel coding style.
 - Added explicit loop increment (i += block_len + 1) to prevent kernel hangs.
 - Isolated changes strictly to drivers/firewire/core-card.c.
 - Added descriptor bounds validation in fw_core_add_descriptor().

 drivers/firewire/core-card.c | 57 +++++++++++++++++++++---------------
 1 file changed, 34 insertions(+), 23 deletions(-)

diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
index 97439da6f480..cb8ce491fe9d 100644
--- a/drivers/firewire/core-card.c
+++ b/drivers/firewire/core-card.c
@@ -169,36 +169,47 @@ static size_t required_space(struct fw_descriptor *desc)
 
 int fw_core_add_descriptor(struct fw_descriptor *desc)
 {
-    size_t i;
+	size_t i;
 
-    /* Extra validation: reject empty or oversized descriptors */
-    if (desc->length == 0 || desc->length > 256)
-        return -EINVAL;
+	/* Reject empty or oversized descriptors early */
+	if (desc->length == 0 || desc->length > 256)
+		return -EINVAL;
+	i = 0;
+	/*
+	 * Validate internal block structures within the descriptor. Each sub-block
+	 * encodes its length in the top 16 bits of its header quadlet.
+	 */
+	while (i < desc->length) {
+		u16 block_len = desc->data[i] >> 16;
+
+		/*
+		 * Guard against corrupted descriptors where an individual block length
+		 * claims to extend past the allocated end of desc->data, avoiding
+		 * out-of-bounds reads.
+		 */
+		if (block_len >= desc->length - i)
+			return -EINVAL;
 
-    /*
-     * Check descriptor is valid, the length of all blocks in the
-     * descriptor has to add up to exactly the length of the block.
-     */
-    i = 0;
-    while (i < desc->length)
-        i += (desc->data[i] >> 16) + 1;
+		i += block_len + 1;
+	}
 
-    if (i != desc->length)
-        return -EINVAL;
+	/* The sum of sub-block lengths must match total descriptor length */
+	if (i != desc->length)
+		return -EINVAL;
 
-    guard(mutex)(&card_mutex);
+	guard(mutex)(&card_mutex);
 
-    if (config_rom_length + required_space(desc) > 256)
-        return -EBUSY;
+	if (config_rom_length + required_space(desc) > 256)
+		return -EBUSY;
 
-    list_add_tail(&desc->link, &descriptor_list);
-    config_rom_length += required_space(desc);
-    descriptor_count++;
-    if (desc->immediate > 0)
-        descriptor_count++;
-    update_config_roms();
+	list_add_tail(&desc->link, &descriptor_list);
+	config_rom_length += required_space(desc);
+	descriptor_count++;
+	if (desc->immediate > 0)
+		descriptor_count++;
+	update_config_roms();
 
-    return 0;
+	return 0;
 }
 EXPORT_SYMBOL(fw_core_add_descriptor);
 
-- 
2.54.0

Re: [PATCH v2] firewire: core: validate descriptor bounds in fw_core_add_descriptor()

[Takashi Sakamoto]

Hi,

Sorry to be late for reply.

On Fri, Jul 24, 2026 at 10:14:06AM +0000, Sreeraj S Kurup wrote:
> In fw_core_add_descriptor(), incoming descriptor data is processed without
> prior bounds verification of individual sub-block headers. A malformed or
> corrupted descriptor containing an oversized block length field can cause
> the parsing loop to read past the end of the desc->data buffer, leading to
> an out-of-bounds memory access.
> 
> Add validation logic at the start of fw_core_add_descriptor() to:
> 1. Reject empty descriptors (length == 0) or descriptors exceeding the
>    maximum configuration ROM size (256 quadlets).
> 2. Validate each sub-block header's embedded length against the remaining
>    descriptor length before advancing the loop pointer.
> 3. Ensure the inner loop explicitly increments by (block_len + 1) quadlets
>    to prevent hangs or buffer overflows.
> 
> Signed-off-by: Sreeraj S Kurup <sreekuttan2156239@gmail.com>
> ---
> v2:
>  - Fixed tab indentation to 8-space tabs per kernel coding style.
>  - Added explicit loop increment (i += block_len + 1) to prevent kernel hangs.
>  - Isolated changes strictly to drivers/firewire/core-card.c.
>  - Added descriptor bounds validation in fw_core_add_descriptor().
> 
>  drivers/firewire/core-card.c | 57 +++++++++++++++++++++---------------
>  1 file changed, 34 insertions(+), 23 deletions(-)
 
This v2 patch conflicts on my tree (7.2-rc4). I guess that v2 was
written on the tree to which v1 patch is applied, so my comments are
provided to the squashed patch below.

======== 8< --------
diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
index a754c6366b97..cb8ce491fe9d 100644
--- a/drivers/firewire/core-card.c
+++ b/drivers/firewire/core-card.c
@@ -143,7 +143,11 @@ static void generate_config_rom(struct fw_card *card, __be32 *config_rom)
 	for (i = 0; i < j; i += length + 1)
 		length = fw_compute_block_crc(config_rom + i);
 
-	WARN_ON(j != config_rom_length);
+     if (j != config_rom_length) {
+        pr_warn("FireWire ROM length mismatch: expected %zu, got %d\n",
+            config_rom_length, j);
+        config_rom_length = j;
+     }
 }
 
 static void update_config_roms(void)
@@ -167,15 +171,29 @@ int fw_core_add_descriptor(struct fw_descriptor *desc)
 {
 	size_t i;
 
+	/* Reject empty or oversized descriptors early */
+	if (desc->length == 0 || desc->length > 256)

For the above check, in_range() macro in include/linux/minmax.h is also
available.

+		return -EINVAL;
+	i = 0;
 	/*
-	 * Check descriptor is valid; the length of all blocks in the
-	 * descriptor has to add up to exactly the length of the
-	 * block.
+	 * Validate internal block structures within the descriptor. Each sub-block
+	 * encodes its length in the top 16 bits of its header quadlet.
 	 */
-	i = 0;
-	while (i < desc->length)
-		i += (desc->data[i] >> 16) + 1;
+	while (i < desc->length) {
+		u16 block_len = desc->data[i] >> 16;
+
+		/*
+		 * Guard against corrupted descriptors where an individual block length
+		 * claims to extend past the allocated end of desc->data, avoiding
+		 * out-of-bounds reads.
+		 */
+		if (block_len >= desc->length - i)
+			return -EINVAL;
+
+		i += block_len + 1;
+	}
 
+	/* The sum of sub-block lengths must match total descriptor length */
 	if (i != desc->length)
 		return -EINVAL;
======== 8< --------

I think the above changes could be split into two patches:

* Overall length validation.
* Descriptor length validation.


Thanks

Takashi Sakamoto