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

[PATCH v2] 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).

Signed-off-by: Aleksandr Shabelnikov <mistermidi@gmail.com>
---
v2:
  - Drop Reported-by / Suggested-by trailers (per Greg KH)
---
 drivers/firewire/core-device.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c
index aeacd4cfd694..fdf15df977f0 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,14 @@ 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 v2] firewire: core: bound traversal stack in read_config_rom()

[Takashi Sakamoto]

Hi,

Thanks for the patch.

On Tue, Sep 02, 2025 at 11:27:45AM +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).
> 
> Signed-off-by: Aleksandr Shabelnikov <mistermidi@gmail.com>
> ---
> v2:
>   - Drop Reported-by / Suggested-by trailers (per Greg KH)
> ---
>  drivers/firewire/core-device.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)

For this kind of issue, I always hesitate to accept such changes, since
they addresses to an unreal problem. Moreover, IEEE 1394 is already a
legacy technology, and has been abandoned by vendors and manufacturers.
It is hardly plausible that such malicious content of configuration ROM
would be spread widely in recent years.

Nevertheless, from the perspective of building a robust software stack,
I can recognize the merit of your proposal. For this aim, I suggest you
consider working with KUnit[1].

The following change allows us to provide a customized read function to
the relevant function in any KUnit test suite. You can find some existing
examples of Kunit tests in the following files:

* drivers/firewire/device-attribute-test.c
* drivers/firewire/ohci-serdes-test.c
* drivers/firewire/packet-serdes-test.c
* drivers/firewire/self-id-sequence-helper-test.c
* drivers/firewire/uapi-test.c

Contributions to this subsystem may not provide a strong advantage to
your career as a software engineer. However, knowledge and experience
with the KUnit framework will certainly be valuable and beneficial. If
you are still motivated, I encourage you to give it a try.

[1] https://docs.kernel.org/dev-tools/kunit/index.html

```
$ git diff
diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c
index 4125e9e8..0987f7fe 100644
--- a/drivers/firewire/core-device.c
+++ b/drivers/firewire/core-device.c
@@ -575,7 +575,8 @@ static int read_rom(struct fw_device *device,
  * are reading the ROM may have changed the ROM during the reset.
  * Returns either a result code or a negative error code.
  */
-static int read_config_rom(struct fw_device *device, int generation)
+static int read_config_rom(struct fw_device *device, int generation,
+                          int (*read_fn)(struct fw_device *, int, int, u32 *))
 {
        struct fw_card *card = device->card;
        const u32 *old_rom, *new_rom;
@@ -595,7 +596,7 @@ static int read_config_rom(struct fw_device *device, int generation)

        /* First read the bus info block. */
        for (i = 0; i < 5; i++) {
-               ret = read_rom(device, generation, i, &rom[i]);
+               ret = read_fn(device, generation, i, &rom[i]);
                if (ret != RCODE_COMPLETE)
                        goto out;
                /*
@@ -633,7 +634,7 @@ static int read_config_rom(struct fw_device *device, int generation)
                        device->max_speed = card->link_speed;

                while (device->max_speed > SCODE_100) {
-                       if (read_rom(device, generation, 0, &dummy) ==
+                       if (read_fn(device, generation, 0, &dummy) ==
                            RCODE_COMPLETE)
                                break;
                        device->max_speed--;
@@ -665,7 +666,7 @@ static int read_config_rom(struct fw_device *device, int generation)
                }

                /* Read header quadlet for the block to get the length. */
-               ret = read_rom(device, generation, i, &rom[i]);
+               ret = read_fn(device, generation, i, &rom[i]);
                if (ret != RCODE_COMPLETE)
                        goto out;
                end = i + (rom[i] >> 16) + 1;
@@ -689,7 +690,7 @@ static int read_config_rom(struct fw_device *device, int generation)
                 * it references another block, and push it in that case.
                 */
                for (; i < end; i++) {
-                       ret = read_rom(device, generation, i, &rom[i]);
+                       ret = read_fn(device, generation, i, &rom[i]);
                        if (ret != RCODE_COMPLETE)
                                goto out;

@@ -1014,7 +1015,7 @@ static void fw_device_init(struct work_struct *work)
         * device.
         */

-       ret = read_config_rom(device, device->generation);
+       ret = read_config_rom(device, device->generation, read_rom);
        if (ret != RCODE_COMPLETE) {
                if (device->config_rom_retries < MAX_RETRIES &&
                    atomic_read(&device->state) == FW_DEVICE_INITIALIZING) {
@@ -1207,7 +1208,7 @@ static void fw_device_refresh(struct work_struct *work)
         */
        device_for_each_child(&device->device, NULL, shutdown_unit);

-       ret = read_config_rom(device, device->generation);
+       ret = read_config_rom(device, device->generation, read_rom);
        if (ret != RCODE_COMPLETE)
                goto failed_config_rom;

``


Thanks

Takashi Sakamoto

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

[Takashi Sakamoto]

On Wed, Sep 03, 2025 at 10:20:48PM +0900, Takashi Sakamoto wrote:
> Contributions to this subsystem may not provide a strong advantage to
> your career as a software engineer. However, knowledge and experience
> with the KUnit framework will certainly be valuable and beneficial. If
> you are still motivated, I encourage you to give it a try.

My collection of configuration ROM would be helpful to see the actual
content. They includes the ones from some music instruments:

* https://github.com/takaswie/am-config-roms/

When parsing the content, 'config-rom-pretty-printer' would be also
helpful, however I note that it has no support for "Extended_ROM entry"
in clause 7.7.18 of IEEE 1212:

https://git.kernel.org/pub/scm/utils/ieee1394/linux-firewire-utils.git/


Regards

Takashi Sakamoto

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

[Aleksandr Shabelnikov]

Thank you kindly for your time and review. I think it’s an excellent idea to
add KUnit coverage, and I will definitely give it a try.

Best regards,
Aleksandr