[PATCH v3 0/2] firewire: core: validate descriptor and sub-block lengths in fw_core_add_descriptor()

[PATCH v3 0/2] firewire: core: validate descriptor and sub-block lengths in fw_core_add_descriptor()

[Sreeraj S Kurup]

This two-patch series addresses potential out-of-bounds memory
accesses when parsing Config ROM descriptors in
fw_core_add_descriptor().

Patch 1 adds overall length validation using the in_range() macro
to ensure descriptors fit within standard IEEE 1394 Config ROM
limits (256 quadlets).

Patch 2 validates individual sub-block header lengths during
iteration to prevent reading past allocated buffer boundaries on
malformed inputs.

v2 -> v3:
 - Split original single patch into two distinct commits for cleaner
   review as requested by Takashi Sakamoto.
 - Simplified overall length check using the in_range() macro.

Sreeraj S Kurup (2):
  firewire: core: validate overall descriptor length in
    fw_core_add_descriptor()
  firewire: core: validate sub-block lengths in fw_core_add_descriptor()

 drivers/firewire/core-card.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

-- 
2.54.0

[PATCH V3 v3 1/2] firewire: core: validate overall descriptor length in fw_core_add_descriptor()

[Sreeraj S Kurup]

In fw_core_add_descriptor(), incoming descriptor structures are processed
without checking whether the descriptor's specified length falls within
valid boundaries. An empty descriptor (length 0) or an oversized descriptor
exceeding the IEEE 1394 Config ROM capacity can lead to invalid processing.

Add bounds checking at the start of fw_core_add_descriptor() using the
in_range() helper macro to reject descriptors with length 0 or length
exceeding 256 quadlets (the standard maximum Configuration ROM size).

Signed-off-by: Sreeraj S Kurup <sreekuttan2156239@gmail.com>
---
 drivers/firewire/core-card.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
index a754c6366b97..eaec54ea287a 100644
--- a/drivers/firewire/core-card.c
+++ b/drivers/firewire/core-card.c
@@ -16,6 +16,7 @@
 #include <linux/list.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/minmax.h>
 #include <linux/spinlock.h>
 #include <linux/workqueue.h>
 
@@ -167,11 +168,10 @@ int fw_core_add_descriptor(struct fw_descriptor *desc)
 {
 	size_t i;
 
-	/*
-	 * Check descriptor is valid; the length of all blocks in the
-	 * descriptor has to add up to exactly the length of the
-	 * block.
-	 */
+	/* Reject empty descriptors or those exceeding max Config ROM size (256 quadlets) */
+	if (!in_range(desc->length, 1, 256))
+		return -EINVAL;
+
 	i = 0;
 	while (i < desc->length)
 		i += (desc->data[i] >> 16) + 1;
-- 
2.54.0

[PATCH V3 v3 2/2] firewire: core: validate sub-block lengths in fw_core_add_descriptor()

[Sreeraj S Kurup]

When traversing internal block structures of a descriptor in
fw_core_add_descriptor(), each sub-block header specifies its own length
in the upper 16 bits of its header quadlet.

If a malformed or corrupted descriptor provides a sub-block length that
exceeds the remaining total length of the descriptor buffer, the parsing
loop advances past the allocated boundary of desc->data, leading to an
out-of-bounds read access.

Validate each sub-block's length against the remaining descriptor size
before advancing the offset pointer to ensure loop bounds safety.

Signed-off-by: Sreeraj S Kurup <sreekuttan2156239@gmail.com>
---
 drivers/firewire/core-card.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/firewire/core-card.c b/drivers/firewire/core-card.c
index eaec54ea287a..7b5d324aa7cc 100644
--- a/drivers/firewire/core-card.c
+++ b/drivers/firewire/core-card.c
@@ -173,9 +173,25 @@ int fw_core_add_descriptor(struct fw_descriptor *desc)
 		return -EINVAL;
 
 	i = 0;
-	while (i < desc->length)
-		i += (desc->data[i] >> 16) + 1;
+	/*
+	 * 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;
+
+		i += block_len + 1;
+	}
 
+	/* The sum of sub-block lengths must match total descriptor length */
 	if (i != desc->length)
 		return -EINVAL;
 
-- 
2.54.0

Re: [PATCH v3 0/2] firewire: core: validate descriptor and sub-block lengths in fw_core_add_descriptor()

[Takashi Sakamoto]

Hi,

On Sat, Jul 25, 2026 at 03:52:53PM +0000, Sreeraj S Kurup wrote:
> This two-patch series addresses potential out-of-bounds memory
> accesses when parsing Config ROM descriptors in
> fw_core_add_descriptor().
> 
> Patch 1 adds overall length validation using the in_range() macro
> to ensure descriptors fit within standard IEEE 1394 Config ROM
> limits (256 quadlets).
> 
> Patch 2 validates individual sub-block header lengths during
> iteration to prevent reading past allocated buffer boundaries on
> malformed inputs.
> 
> v2 -> v3:
>  - Split original single patch into two distinct commits for cleaner
>    review as requested by Takashi Sakamoto.
>  - Simplified overall length check using the in_range() macro.
> 
> Sreeraj S Kurup (2):
>   firewire: core: validate overall descriptor length in
>     fw_core_add_descriptor()
>   firewire: core: validate sub-block lengths in fw_core_add_descriptor()
> 
>  drivers/firewire/core-card.c | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)

Applied to for-next branch.

By the way, how do you think about the following change in your previous
patches? It seems to be omitted from current patchset.

======== 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;
+     }
 }
======== 8< --------


Thanks

Takashi Sakamoto

Re: [PATCH v3 0/2] firewire: core: validate descriptor and sub-block lengths in fw_core_add_descriptor()

[Takashi Sakamoto]

Hi,

On Mon, Jul 27, 2026 at 05:42:31PM +0000, Sreeraj S Kurup wrote:
> Hi Takashi,
> 
> Thanks for applying the series to the for-next branch!
> 
> Regarding the change in generate_config_rom():
> 
> I dropped that specific change from this patchset to keep the series 
> strictly focused on the out-of-bounds issue in fw_core_add_descriptor(). 
> 
> `WARN_ON(j != config_rom_length)` acts as an assertion that the ROM block 
> traversal matched the expected length. Replacing it with pr_warn() and 
> clamping `config_rom_length = j` avoids a kernel warning splat, but if 
> j != config_rom_length occurs, it indicates an unexpected state during 
> ROM generation rather than a standard runtime condition.
> 
> If you feel handling length mismatches gracefully via pr_warn() and updating 
> config_rom_length is preferred over the WARN_ON assertion, I would be happy 
> to send a follow-up patch for it.

Hm. The case that j does not equals to config_rom_length seems to be a
programming mistake and should be fixed immediately. So let me leave it
as is without any mitigation.


Thanks

Takashi Sakamoto

Re: [PATCH v3 0/2] firewire: core: validate descriptor and sub-block lengths in fw_core_add_descriptor()

[Sreeraj S Kurup]

Hi Takashi,

Thanks for applying the series to the for-next branch!

Regarding the change in generate_config_rom():

I dropped that specific change from this patchset to keep the series 
strictly focused on the out-of-bounds issue in fw_core_add_descriptor(). 

`WARN_ON(j != config_rom_length)` acts as an assertion that the ROM block 
traversal matched the expected length. Replacing it with pr_warn() and 
clamping `config_rom_length = j` avoids a kernel warning splat, but if 
j != config_rom_length occurs, it indicates an unexpected state during 
ROM generation rather than a standard runtime condition.

If you feel handling length mismatches gracefully via pr_warn() and updating 
config_rom_length is preferred over the WARN_ON assertion, I would be happy 
to send a follow-up patch for it.

Thanks,
Sreeraj