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

[Takashi Sakamoto <o-takashi@sakamocchi.jp>]

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