Message-ID: <20250903132048.GA77442@workstation.local>
Date: Wed, 3 Sep 2025 22:20:48 +0900
From: Takashi Sakamoto <o-takashi@...amocchi.jp>
To: Aleksandr Shabelnikov <mistermidi@...il.com>
Cc: linux1394-devel@...ts.sourceforge.net, linux-kernel@...r.kernel.org,
	gregkh@...uxfoundation.org
Subject: Re: [PATCH v2] firewire: core: bound traversal stack in
 read_config_rom()

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@...il.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

Powered by blists - more mailing lists