[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@xxxxxxxxxxxxxxx # v6.12+
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Michael Bommarito <michael.bommarito@xxxxxxxxx>
---
v2: correct the Fixes attribution per Takashi Sakamoto's review
    (https://lore.kernel.org/all/20260615111122.GA176369@xxxxxxxxxxxxx/).
    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@xxxxxxxxx/

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