On 21/08/2020 11:48, Peilin Ye wrote: > Hi Mr. Verkuil, > > On Wed, Aug 19, 2020 at 04:26:28PM +0200, Hans Verkuil wrote: >> Hi Peilin, >> >> On 10/08/2020 07:05, Peilin Ye wrote: >>> precalculate_color() is reading out of `sin` since `tpg->hue` is not being >>> properly checked. Fix it. `cos` is safe, as long as `tpg->hue` is higher >>> than or equal to -192. >> >> Thank you for this patch, but there is something I don't understand, namely >> just *how* tpg->hue can be out-of-range. >> >> From what I can see vivid sets hue via tpg_s_hue() when the V4L2_CID_HUE control >> is set. But that control has a range of -128...128, so ctrl->val should always be in >> that range. >> >> I would really like to know 1) what the value of tpg->hue actually is when it goes >> out of range, and 2) who is changing it to that value. Can you do a bit more digging? > > The value of `tpg->hue` was -20551. It came from the userspace, see the > "\xb9\xaf" on line 500 of the reproducer: > > https://syzkaller.appspot.com/text?tag=ReproC&x=14b49e71e00000 > > NONFAILING(memcpy((void*)0x20000200, "/dev/video6\000\000", 13)); > res = syscall(__NR_openat, 0xffffffffffffff9cul, 0x20000200ul, 2ul, 0ul); > if (res != -1) > r[0] = res; > NONFAILING(memcpy((void*)0x20000140, > "\x4d\x43\x66\x34\xfd\x89\xb9\xaf\x0d\x59\xa2\x83\x4c\xfd" > ^^^^ ^^^^ ^^^^ ^^^^^^^^ > "\x3e\x64\x7c\x96\xcd\x59\xf2\x3a\x18\xa3\x81\x49\x22\xc0" > "\xc1\xbf\x02\xa5\x50\x5f\xcb\x48\x92\x0e\xf3\xdc\xff\x85" > "\xb7\x84\x21\xab\xef\x31\x3d\xb1\xb6\x5d\xbf\x07\x8e\xee" > "\x5e\x7c\x73\x32\xf4\x9d\x1e\x62\x6b\x6a\xa0\x74\x73\xe6" > "\xca\x1b\xdb\x7a\xca\x76\xd8\x37\xb8\xd9", > 80)); > syscall(__NR_write, r[0], 0x20000140ul, 8ul); > > I guess the root cause is a race condition in the vivid test driver, > which completely corrupted `tpg`. I see bytes like "\x4d", "\x66" and > "\xfd" around `tpg->hue`, too. > > The reproducer does two things: the above write() on /dev/video6, and a > preadv() on /dev/video3: > > NONFAILING(*(uint64_t*)0x20000800 = 0x20000000); > NONFAILING(*(uint64_t*)0x20000808 = 0x1f); > NONFAILING(*(uint64_t*)0x20000810 = 0); > NONFAILING(*(uint64_t*)0x20000818 = 0); > NONFAILING(*(uint64_t*)0x20000820 = 0); > NONFAILING(*(uint64_t*)0x20000828 = 0); > NONFAILING(*(uint64_t*)0x20000830 = 0); > NONFAILING(*(uint64_t*)0x20000838 = 0); > NONFAILING(*(uint64_t*)0x20000840 = 0); > NONFAILING(*(uint64_t*)0x20000848 = 0); > syscall(__NR_preadv, r[1], 0x20000800ul, 5ul, 0ul); > > I commented out this preadv(), then the reproducer didn't cause any > crash. Unfortunately I don't know the code well enough in order to > figure out exactly why...At this point of time I'd like to send you an > v2 as you suggested, it should work as a mitigation. Arrgh! I know what this is. /dev/video6 corresponds to the Metadata output device of vivid, and that metadata format sets brightness, contrast, saturation and hue: struct vivid_meta_out_buf { u16 brightness; u16 contrast; u16 saturation; s16 hue; }; vivid_meta_out_process() calls tpg_s_* functions to set these values. But this is wrong, it should set the corresponding V4L2 controls instead since calling these tpg_s_* functions bypasses all range checks. It also will not update the controls themselves, so they are out-of-sync with the actual values. I.e. the test pattern generator uses different values compared to the values in the controls. So two patches are needed: 1) a patch for include/media/tpg/v4l2-tpg.h where tpg_s_hue will clamp the hue value to the valid range. This to prevent anyone else from setting invalid hue values in the tpg. 2) a patch for drivers/media/test-drivers/vivid/vivid-meta-out.c where, instead of calling the tpg_s_* functions in vivid_meta_out_process(), it calls instead: v4l2_ctrl_s_ctrl(dev->brightness, meta->brightness); v4l2_ctrl_s_ctrl(dev->contrast, meta->contrast); etc. Do patch 2 first and test with syzkaller to check that by going through the controls this issue is resolved. Since with that approach the tpg should always get valid hue values. Regards, Hans > > Thank you for the suggestion! > > Peilin Ye >
