[PATCH] firewire: core: bound traversal stack in read_config_rom()

[PATCH] firewire: core: bound traversal stack in read_config_rom()

[Aleksandr Shabelnikov]

read_config_rom() walks Configuration ROM directories using an explicit
stack but pushes new entries without a bound check:

    stack[sp++] = i + rom[i];

A malicious or malformed Configuration ROM can construct in-range cyclic
directory references so that the traversal keeps enqueueing, growing the
stack past its allocated depth. rom[] and stack[] are allocated adjacent
in a single kmalloc() block, so this leads to a heap out-of-bounds write.

Add a hard bound check before every push. While this does not itself
implement cycle detection, it prevents memory corruption and limits the
impact to a clean failure (-EOVERFLOW).

Reported-by: Aleksandr Shabelnikov <mistermidi@gmail.com>
Suggested-by: Aleksandr Shabelnikov <mistermidi@gmail.com>

Signed-off-by: Aleksandr Shabelnikov <mistermidi@gmail.com>
---
 drivers/firewire/core-device.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c
index aeacd4cfd694..f9953a292541 100644
--- a/drivers/firewire/core-device.c
+++ b/drivers/firewire/core-device.c
@@ -581,6 +581,7 @@ static int read_config_rom(struct fw_device *device, int generation)
 	const u32 *old_rom, *new_rom;
 	u32 *rom, *stack;
 	u32 sp, key;
+	u32 tgt; /* target index for referenced block */
 	int i, end, length, ret;
 
 	rom = kmalloc(sizeof(*rom) * MAX_CONFIG_ROM_SIZE +
@@ -702,7 +703,8 @@ static int read_config_rom(struct fw_device *device, int generation)
 			 * fake immediate entry so that later iterators over
 			 * the ROM don't have to check offsets all the time.
 			 */
-			if (i + (rom[i] & 0xffffff) >= MAX_CONFIG_ROM_SIZE) {
+			tgt = i + (rom[i] & 0xffffff);
+			if (tgt >= MAX_CONFIG_ROM_SIZE) {
 				fw_err(card,
 				       "skipped unsupported ROM entry %x at %llx\n",
 				       rom[i],
@@ -710,7 +712,12 @@ static int read_config_rom(struct fw_device *device, int generation)
 				rom[i] = 0;
 				continue;
 			}
-			stack[sp++] = i + rom[i];
+			/* Bound check to prevent traversal stack overflow
+			 * due to malformed cyclic ROM
+			 */
+			if (sp >= MAX_CONFIG_ROM_SIZE) {
+				ret = -EOVERFLOW;
+				goto out;
+			}
+			stack[sp++] = (rom[i] & 0xc0000000) | tgt;
 		}
 		if (length < i)
 			length = i;
-- 
2.50.1 (Apple Git-155)

Re: [PATCH] firewire: core: bound traversal stack in read_config_rom()

[Greg KH]

On Mon, Sep 01, 2025 at 07:15:47PM +0200, Aleksandr Shabelnikov wrote:
> read_config_rom() walks Configuration ROM directories using an explicit
> stack but pushes new entries without a bound check:
> 
>     stack[sp++] = i + rom[i];
> 
> A malicious or malformed Configuration ROM can construct in-range cyclic
> directory references so that the traversal keeps enqueueing, growing the
> stack past its allocated depth. rom[] and stack[] are allocated adjacent
> in a single kmalloc() block, so this leads to a heap out-of-bounds write.
> 
> Add a hard bound check before every push. While this does not itself
> implement cycle detection, it prevents memory corruption and limits the
> impact to a clean failure (-EOVERFLOW).
> 
> Reported-by: Aleksandr Shabelnikov <mistermidi@gmail.com>
> Suggested-by: Aleksandr Shabelnikov <mistermidi@gmail.com>
> 
> Signed-off-by: Aleksandr Shabelnikov <mistermidi@gmail.com>

Nit, you only need the last one "reported-by" and "suggested-by" don't
mean anything when it is you as the author and signed-off-by line :)

thanks,

greg k-h