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@xxxxxxxxxxxxxxx # v6.12+
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Michael Bommarito <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(-)

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