| Message ID | 20200810050532.640075-1-yepeilin.cs@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [Linux-kernel-mentees] v4l2-tpg: Fix global-out-of-bounds read in precalculate_color() | expand |
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? That said, it makes sense that precalculate_color() avoids reading out-of-bounds. > > Fixes: 63881df94d3e ("[media] vivid: add the Test Pattern Generator") > Reported-and-tested-by: syzbot+02d9172bf4c43104cd70@syzkaller.appspotmail.com > Link: https://syzkaller.appspot.com/bug?id=db50123c788e2cc5a9d90de569c398b66293ee48 > Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com> > --- > drivers/media/common/v4l2-tpg/v4l2-tpg-core.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c b/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c > index 50f1e0b28b25..52205fe096f7 100644 > --- a/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c > +++ b/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c > @@ -930,6 +930,9 @@ static void precalculate_color(struct tpg_data *tpg, int k) > /* Implement these operations */ > int tmp_cb, tmp_cr; > > + if (tpg->hue < -128 || tpg->hue > 128) > + return; Rather than returning here, I prefer to just clamp tpg->hue to the valid range. I'd be willing to merge a patch that clamps tpg->hue (it certainly doesn't hurt), but I also would like to understand how it can be out of range in the first place. I have the feeling that this is a symptom of another problem elsewhere. Regards, Hans > + > /* First convert to YCbCr */ > > color_to_ycbcr(tpg, r, g, b, &y, &cb, &cr); >
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? Sure, of course! I will try to figure that out first. Thank you, Peilin > That said, it makes sense that precalculate_color() avoids reading out-of-bounds. > > > > > Fixes: 63881df94d3e ("[media] vivid: add the Test Pattern Generator") > > Reported-and-tested-by: syzbot+02d9172bf4c43104cd70@syzkaller.appspotmail.com > > Link: https://syzkaller.appspot.com/bug?id=db50123c788e2cc5a9d90de569c398b66293ee48 > > Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com> > > --- > > drivers/media/common/v4l2-tpg/v4l2-tpg-core.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c b/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c > > index 50f1e0b28b25..52205fe096f7 100644 > > --- a/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c > > +++ b/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c > > @@ -930,6 +930,9 @@ static void precalculate_color(struct tpg_data *tpg, int k) > > /* Implement these operations */ > > int tmp_cb, tmp_cr; > > > > + if (tpg->hue < -128 || tpg->hue > 128) > > + return; > > Rather than returning here, I prefer to just clamp tpg->hue to the valid range. > > I'd be willing to merge a patch that clamps tpg->hue (it certainly doesn't hurt), > but I also would like to understand how it can be out of range in the first place. > I have the feeling that this is a symptom of another problem elsewhere. > > Regards, > > Hans > > > + > > /* First convert to YCbCr */ > > > > color_to_ycbcr(tpg, r, g, b, &y, &cb, &cr); > > >
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. Thank you for the suggestion! Peilin Ye
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 >
On Fri, Aug 21, 2020 at 12:12:28PM +0200, Hans Verkuil wrote: > 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. Ah, now it all makes sense! Thank you! I will do them now. Peilin
diff --git a/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c b/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c index 50f1e0b28b25..52205fe096f7 100644 --- a/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c +++ b/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c @@ -930,6 +930,9 @@ static void precalculate_color(struct tpg_data *tpg, int k) /* Implement these operations */ int tmp_cb, tmp_cr; + if (tpg->hue < -128 || tpg->hue > 128) + return; + /* First convert to YCbCr */ color_to_ycbcr(tpg, r, g, b, &y, &cb, &cr);
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. Fixes: 63881df94d3e ("[media] vivid: add the Test Pattern Generator") Reported-and-tested-by: syzbot+02d9172bf4c43104cd70@syzkaller.appspotmail.com Link: https://syzkaller.appspot.com/bug?id=db50123c788e2cc5a9d90de569c398b66293ee48 Signed-off-by: Peilin Ye <yepeilin.cs@gmail.com> --- drivers/media/common/v4l2-tpg/v4l2-tpg-core.c | 3 +++ 1 file changed, 3 insertions(+)