[PATCH] firewire: core: propagate cycle-timer read failure in legacy ioctl

[PATCH] firewire: core: propagate cycle-timer read failure in legacy ioctl

[Michael Bommarito]

The legacy FW_CDEV_IOC_GET_CYCLE_TIMER handler ioctl_get_cycle_timer()
reads into a local struct fw_cdev_get_cycle_timer2 via
ioctl_get_cycle_timer2() and then copies tv_sec, tv_nsec and cycle_timer
into the user-visible output, but it ignores the helper's return value.
Since commit bacf921c42bb ("firewire: core: use guard macro to disable
local IRQ") the helper returns early on fw_card_read_cycle_time()
failure (for example -ENODEV after the dummy card driver is swapped in
during controller removal) without writing those fields, so the wrapper
copies out the uninitialized local: 12 of the 16 user-visible bytes are
uninitialized kernel stack. The non-legacy ioctl already returns the
error on this path.

Propagate the helper return value so no output is copied on failure.

Reproduced under KMSAN; the legacy wrapper no longer returns
uninitialized cycle-timer bytes after this change.

Fixes: bacf921c42bb ("firewire: core: use guard macro to disable local IRQ")
Cc: stable@vger.kernel.org # v6.12+
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
---
Reproduction (v7.1-rc4, x86_64 QEMU, CONFIG_KMSAN=y, stack auto-init off):

With fw_card_read_cycle_time() forced to fail, the stock legacy handler
returns 0 and KMSAN reports the uninitialized local being stored to the
output:

  BUG: KMSAN: uninit-value in ioctl_get_cycle_timer
  Local variable ct2 created at: ioctl_get_cycle_timer

The leaked output is local_time (8 bytes, bytes 0-7, computed from the
uninitialized tv_sec/tv_nsec) and cycle_timer (4 bytes, bytes 8-11,
copied verbatim). After this change the handler returns -ENODEV and the
KMSAN reports are gone; a success-path control stays clean.

 drivers/firewire/core-cdev.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index f791db4c8dfff..1d80e30cb681e 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -1283,9 +1283,12 @@ static int ioctl_get_cycle_timer(struct client *client, union ioctl_arg *arg)
 {
 	struct fw_cdev_get_cycle_timer *a = &arg->get_cycle_timer;
 	struct fw_cdev_get_cycle_timer2 ct2;
+	int ret;
 
 	ct2.clk_id = CLOCK_REALTIME;
-	ioctl_get_cycle_timer2(client, (union ioctl_arg *)&ct2);
+	ret = ioctl_get_cycle_timer2(client, (union ioctl_arg *)&ct2);
+	if (ret < 0)
+		return ret;
 
 	a->local_time = ct2.tv_sec * USEC_PER_SEC + ct2.tv_nsec / NSEC_PER_USEC;
 	a->cycle_timer = ct2.cycle_timer;

base-commit: e24a98d6884a6e4203a77a94f070a59fcab95208
-- 
2.53.0

Re: [PATCH] firewire: core: propagate cycle-timer read failure in legacy ioctl

[Takashi Sakamoto]

Hi,

Thanks for the patch, however I can see some subtle gotchas.

On Sun, Jun 14, 2026 at 11:15:43AM -0400, Michael Bommarito wrote:
> The legacy FW_CDEV_IOC_GET_CYCLE_TIMER handler ioctl_get_cycle_timer()
> reads into a local struct fw_cdev_get_cycle_timer2 via
> ioctl_get_cycle_timer2() and then copies tv_sec, tv_nsec and cycle_timer
> into the user-visible output, but it ignores the helper's return value.
> Since commit bacf921c42bb ("firewire: core: use guard macro to disable
> local IRQ") the helper returns early on fw_card_read_cycle_time()
> failure (for example -ENODEV after the dummy card driver is swapped in
> during controller removal) without writing those fields, so the wrapper
> copies out the uninitialized local: 12 of the 16 user-visible bytes are
> uninitialized kernel stack. The non-legacy ioctl already returns the
> error on this path.
> 
> Propagate the helper return value so no output is copied on failure.
> 
> Reproduced under KMSAN; the legacy wrapper no longer returns
> uninitialized cycle-timer bytes after this change.
> 
> Fixes: bacf921c42bb ("firewire: core: use guard macro to disable local IRQ")
> Cc: stable@vger.kernel.org # v6.12+
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
> ---
> Reproduction (v7.1-rc4, x86_64 QEMU, CONFIG_KMSAN=y, stack auto-init off):
> 
> With fw_card_read_cycle_time() forced to fail, the stock legacy handler
> returns 0 and KMSAN reports the uninitialized local being stored to the
> output:
> 
>   BUG: KMSAN: uninit-value in ioctl_get_cycle_timer
>   Local variable ct2 created at: ioctl_get_cycle_timer
> 
> The leaked output is local_time (8 bytes, bytes 0-7, computed from the
> uninitialized tv_sec/tv_nsec) and cycle_timer (4 bytes, bytes 8-11,
> copied verbatim). After this change the handler returns -ENODEV and the
> KMSAN reports are gone; a success-path control stays clean.
> 
>  drivers/firewire/core-cdev.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

The issued function, ioctl_get_cycle_timer() was introduced by a commit
abfe5a01ef1e ("firewire: cdev: add more flexible cycle timer ioctl")[1].
We can see in this time the function has already ignored the return value
of get_cycle_timer2(). The issue is independent of the later commit
bacf921c42bb ("firewire: core: use guard macro to disable local IRQ").

Besides, userspace applications can unbind 1394 OHCI driver in any time,
so it is possible to use the vulnerability reading small part of kernel
stack. (Understandably, if the attacker is successful to install 1394
OHCI device to the target hardware, he or she can accomplish more jobs
regardless of the issue.)

Anyway, the leak of kernel stack to userspace is not preferable itself.
I would like to see your revised patch, writing on your own.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=abfe5a01ef1e4


Thanks

Takashi Sakamoto

[PATCH v2] firewire: core: propagate cycle-timer read failure in legacy ioctl

[Michael Bommarito]

The legacy FW_CDEV_IOC_GET_CYCLE_TIMER handler ioctl_get_cycle_timer()
reads into a local struct fw_cdev_get_cycle_timer2 via
ioctl_get_cycle_timer2() and copies tv_sec, tv_nsec and cycle_timer into
the user-visible output, but it has ignored the helper's return value
since the ioctl was introduced. When ioctl_get_cycle_timer2() returns
early on an fw_card_read_cycle_time() failure (for example -ENODEV after
the dummy card driver is swapped in during controller removal) it does
not populate the output, so the wrapper copies out the uninitialized
local: 12 of the 16 user-visible bytes are uninitialized kernel stack.
The non-legacy ioctl already returns the error on this path. A local
user can reach this by unbinding the 1394 OHCI driver while holding the
character device open.

Propagate the helper return value so no output is copied on failure.

The present uninitialized read is reachable since v6.12: commit
bacf921c42bb ("firewire: core: use guard macro to disable local IRQ")
changed the helper's error path to return early and skip the field
writes, leaving the wrapper's local uninitialized. The stable backport
is therefore limited to v6.12 and later even though the ignored return
value dates back to the original ioctl.

Reproduced under KMSAN; the legacy wrapper no longer returns
uninitialized cycle-timer bytes after this change.

Fixes: abfe5a01ef1e ("firewire: cdev: add more flexible cycle timer ioctl")
Cc: stable@vger.kernel.org # v6.12+
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
---
v2: correct the Fixes attribution per Takashi Sakamoto's review
    (https://lore.kernel.org/all/20260615111122.GA176369@sakamocchi.jp/).
    ioctl_get_cycle_timer() has ignored ioctl_get_cycle_timer2()'s return
    value since it was added in abfe5a01ef1e, so the Fixes tag now points
    there rather than at bacf921c42bb. Reworded the changelog accordingly
    and added Sakamoto's reachability note (unbinding the 1394 OHCI driver
    at runtime). The stable range stays v6.12+ because the helper's error
    path only began returning early and skipping the field writes in
    bacf921c42bb; earlier kernels wrote the fields on that path. No code
    change from v1.
    v1: https://lore.kernel.org/all/20260614151543.2976015-1-michael.bommarito@gmail.com/

Reproduction (v7.1-rc4, x86_64 QEMU, CONFIG_KMSAN=y, stack auto-init off):

With fw_card_read_cycle_time() forced to fail, the stock legacy handler
returns 0 and KMSAN reports the uninitialized local being stored to the
output:

  BUG: KMSAN: uninit-value in ioctl_get_cycle_timer
  Local variable ct2 created at: ioctl_get_cycle_timer

The leaked output is local_time (8 bytes, bytes 0-7, computed from the
uninitialized tv_sec/tv_nsec) and cycle_timer (4 bytes, bytes 8-11,
copied verbatim). After this change the handler returns -ENODEV and the
KMSAN reports are gone; a success-path control stays clean.

 drivers/firewire/core-cdev.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index f791db4c8dfff..1d80e30cb681e 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -1283,9 +1283,12 @@ static int ioctl_get_cycle_timer(struct client *client, union ioctl_arg *arg)
 {
 	struct fw_cdev_get_cycle_timer *a = &arg->get_cycle_timer;
 	struct fw_cdev_get_cycle_timer2 ct2;
+	int ret;
 
 	ct2.clk_id = CLOCK_REALTIME;
-	ioctl_get_cycle_timer2(client, (union ioctl_arg *)&ct2);
+	ret = ioctl_get_cycle_timer2(client, (union ioctl_arg *)&ct2);
+	if (ret < 0)
+		return ret;
 
 	a->local_time = ct2.tv_sec * USEC_PER_SEC + ct2.tv_nsec / NSEC_PER_USEC;
 	a->cycle_timer = ct2.cycle_timer;

base-commit: e24a98d6884a6e4203a77a94f070a59fcab95208
-- 
2.53.0