Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [Linux-kernel-mentees] [PATCH] v4l2-tpg: Fix global-out-of-bounds
 read in precalculate_color()
To:     Peilin Ye <yepeilin.cs@gmail.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-media@vger.kernel.org,
        linux-kernel-mentees@lists.linuxfoundation.org,
        syzkaller-bugs@googlegroups.com, linux-kernel@vger.kernel.org
References: <20200810050532.640075-1-yepeilin.cs@gmail.com>
 <774bcf2d-c8eb-086a-3569-c7e530d8a63d@xs4all.nl>
 <20200821094813.GB480216@PWN>
From:   Hans Verkuil <hverkuil@xs4all.nl>
Message-ID: <eab860f3-5520-72cd-3bb1-f990f7f9ea6f@xs4all.nl>
Date:   Fri, 21 Aug 2020 12:12:28 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <20200821094813.GB480216@PWN>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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
>