Hi Helge, thanks for your comments.
mclk_khz is derived from the PRAMDAC MCLK PLL and then passed into the NV3 arbitration code. In nv3_arb() it is used as a divisor, so a zero value will always be fatal. For this fix I tried to keep it small and safe for stable. Putting the guard in nv3_arb() makes sure we never hit a divide-by-zero no matter how we got there. It also keeps the change local to the code that actually needs the invariant mclk_khz != 0. Adding checks in rivafb_set_par() (or other callers in the trace) would either duplicate validation in places that do not compute mclk_khz, or require pushing error handling through several layers. About initializing mclk_khz to some default: I would rather not guess a clock. A made-up value can lead to wrong FIFO arbitration settings. If arbitration cannot be computed, bailing out and using the existing conservative fallback is safer. I agree it could be worth adding earlier validation of the PLL-derived clock as a follow-up. For the stable fix, I prefer the minimal guard at the division site. Best regards, Guangshuo Helge Deller <[email protected]> 于2025年12月9日周二 06:02写道： > > On 12/7/25 08:25, Guangshuo Li wrote: > > A userspace program can trigger the RIVA NV3 arbitration code by > > calling the FBIOPUT_VSCREENINFO ioctl on /dev/fb*. When doing so, > > the driver recomputes FIFO arbitration parameters in nv3_arb(), using > > state->mclk_khz (derived from the PRAMDAC MCLK PLL) as a divisor > > without validating it first. > > > > In a normal setup, state->mclk_khz is provided by the real hardware > > and is non-zero. However, an attacker can construct a malicious or > > misconfigured device (e.g. a crafted/emulated PCI device) that exposes > > a bogus PLL configuration, causing state->mclk_khz to become zero. > > Once nv3_get_param() calls nv3_arb(), the division by state->mclk_khz in > > the gns calculation causes a divide error and crashes the kernel. > > > > Fix this by checking whether state->mclk_khz is zero and bailing out before > > doing the division. > > > > The following log reveals it: > > > > rivafb: setting virtual Y resolution to 2184 > > divide error: 0000 [#1] PREEMPT SMP KASAN PTI > > CPU: 0 PID: 2187 Comm: syz-executor.0 Not tainted 5.18.0-rc1+ #1 > > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS > > rel-1.12.0-59-gc9ba5276e321-prebuilt.qemu.org 04/01/2014 > > RIP: 0010:nv3_arb drivers/video/fbdev/riva/riva_hw.c:439 [inline] > > RIP: 0010:nv3_get_param+0x3ab/0x13b0 drivers/video/fbdev/riva/riva_hw.c:546 > > Code: c1 e8 03 42 0f b6 14 38 48 89 f8 83 e0 07 83 c0 03 38 d0 7c 08 84 d2 > > 0f 85 b7 0e 00 00 41 8b 46 18 01 d8 69 c0 40 42 0f 00 99 <41> f7 fc 48 63 > > c8 4c 89 e8 48 c1 e8 03 42 0f b6 14 38 4c 89 e8 83 > > RSP: 0018:ffff888013b2f318 EFLAGS: 00010206 > > RAX: 0000000001d905c0 RBX: 0000000000000016 RCX: 0000000000040000 > > RDX: 0000000000000000 RSI: 0000000000000080 RDI: ffff888013b2f6f0 > > RBP: 0000000000000002 R08: ffffffff82226288 R09: 0000000000000001 > > R10: 0000000000000000 R11: 0000000000000001 R12: 0000000000000000 > > R13: ffff888013b2f4d8 R14: ffff888013b2f6d8 R15: dffffc0000000000 > > Call Trace: > > nv3CalcArbitration.constprop.0+0x255/0x460 > > drivers/video/fbdev/riva/riva_hw.c:603 > > nv3UpdateArbitrationSettings drivers/video/fbdev/riva/riva_hw.c:637 > > [inline] > > CalcStateExt+0x447/0x1b90 drivers/video/fbdev/riva/riva_hw.c:1246 > > riva_load_video_mode+0x8a9/0xea0 drivers/video/fbdev/riva/fbdev.c:779 > > rivafb_set_par+0xc0/0x5f0 drivers/video/fbdev/riva/fbdev.c:1196 > > Doesn't it make sense to check mclk_khz (or the various variables which > lead to mclk_khz) in rivafb_set_par() or any of the other functions mentioned > in this trace? > If in doubt, mclk_khz could be initialized to a sane value? > > Helge > > > > fb_set_var+0x604/0xeb0 drivers/video/fbdev/core/fbmem.c:1033 > > do_fb_ioctl+0x234/0x670 drivers/video/fbdev/core/fbmem.c:1109 > > fb_ioctl+0xdd/0x130 drivers/video/fbdev/core/fbmem.c:1188 > > __x64_sys_ioctl+0x122/0x190 fs/ioctl.c:856 > > > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > > Cc: [email protected] > > Signed-off-by: Guangshuo Li <[email protected]> > > --- > > drivers/video/fbdev/riva/riva_hw.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/video/fbdev/riva/riva_hw.c > > b/drivers/video/fbdev/riva/riva_hw.c > > index 8b829b720064..d70c6c4d28e8 100644 > > --- a/drivers/video/fbdev/riva/riva_hw.c > > +++ b/drivers/video/fbdev/riva/riva_hw.c > > @@ -436,6 +436,9 @@ static char nv3_arb(nv3_fifo_info * res_info, > > nv3_sim_state * state, nv3_arb_in > > vmisses = 2; > > eburst_size = state->memory_width * 1; > > mburst_size = 32; > > + if (!state->mclk_khz) > > + return (0); > > + > > gns = 1000000 * (gmisses*state->mem_page_miss + > > state->mem_latency)/state->mclk_khz; > > ainfo->by_gfacc = gns*ainfo->gdrain_rate/1000000; > > ainfo->wcmocc = 0; >
