[Linux-kernel-mentees] [PATCH] v4l2-tpg: Fix global-out-of-bounds read in precalculate_color()

[Peilin Ye]

#syz test: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-and-tested-by: syzbot+02d917...@syzkaller.appspotmail.com

Tested on:

commit: fc80c51f Merge tag 'kbuild-v5.9' of git://git.kernel.org/p..
git tree: upstream
kernel config: https://syzkaller.appspot.com/x/.config?x=70e5ed1856a8c7ad
dashboard link: https://syzkaller.appspot.com/bug?extid=02d9172bf4c43104cd70
compiler: gcc (GCC) 10.1.0-syz 20200507
patch: https://syzkaller.appspot.com/x/patch.diff?x=1716a9aa900000

Note: testing is done by a robot and is best-effort only.

[Peilin Ye]

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+02d917...@syzkaller.appspotmail.com
Link: https://syzkaller.appspot.com/bug?id=db50123c788e2cc5a9d90de569c398b66293ee48
Signed-off-by: Peilin Ye <yepei...@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;
+
/* First convert to YCbCr */

color_to_ycbcr(tpg, r, g, b, &y, &cb, &cr);
--
2.25.1

[Hans Verkuil]

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+02d917...@syzkaller.appspotmail.com
> Link: https://syzkaller.appspot.com/bug?id=db50123c788e2cc5a9d90de569c398b66293ee48
> Signed-off-by: Peilin Ye <yepei...@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

[Peilin Ye]

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

[Peilin Ye]

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

[Hans Verkuil]

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

[Peilin Ye]

Ah, now it all makes sense! Thank you! I will do them now.

Peilin

[Peilin Ye]

vivid_meta_out_process() is setting `brightness`, `contrast`, `saturation`
and `hue` using tpg_s_*(). This is wrong, since tpg_s_*() do not provide
range checks. Using tpg_s_*() here also makes the control framework
out-of-sync with the actual values. Use v4l2_ctrl_s_ctrl() instead.

This issue has been reported by syzbot as an out-of-bounds read bug in
precalculate_color().

Fixes: 746facd39370 ("media: vivid: Add metadata output support")
Reported-and-tested-by: syzbot+02d917...@syzkaller.appspotmail.com
Link: https://syzkaller.appspot.com/bug?extid=02d9172bf4c43104cd70
Suggested-by: Hans Verkuil <hver...@xs4all.nl>

Signed-off-by: Peilin Ye <yepei...@gmail.com>
---

I'm not very sure how to name this patch since we are fixing a bug in
v4l2-tpg by modifying vivid.

Change in v2:
- Solve the root of the problem instead of adding more boundary checks
in precalculate_color(). (Suggested by Hans Verkuil
<hver...@xs4all.nl>)

drivers/media/test-drivers/vivid/vivid-meta-out.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/media/test-drivers/vivid/vivid-meta-out.c b/drivers/media/test-drivers/vivid/vivid-meta-out.c
index ff8a039aba72..95835b52b58f 100644
--- a/drivers/media/test-drivers/vivid/vivid-meta-out.c
+++ b/drivers/media/test-drivers/vivid/vivid-meta-out.c
@@ -164,10 +164,11 @@ void vivid_meta_out_process(struct vivid_dev *dev,
{
struct vivid_meta_out_buf *meta = vb2_plane_vaddr(&buf->vb.vb2_buf, 0);

- tpg_s_brightness(&dev->tpg, meta->brightness);
- tpg_s_contrast(&dev->tpg, meta->contrast);
- tpg_s_saturation(&dev->tpg, meta->saturation);
- tpg_s_hue(&dev->tpg, meta->hue);
+ v4l2_ctrl_s_ctrl(dev->brightness, meta->brightness);
+ v4l2_ctrl_s_ctrl(dev->contrast, meta->contrast);
+ v4l2_ctrl_s_ctrl(dev->saturation, meta->saturation);
+ v4l2_ctrl_s_ctrl(dev->hue, meta->hue);
+
dprintk(dev, 2, " %s brightness %u contrast %u saturation %u hue %d\n",
__func__, meta->brightness, meta->contrast,
meta->saturation, meta->hue);
--
2.25.1