Re: [Cocci] [PATCH] coccicheck: add a test for repeat copy_from_user

[Kees Cook <keescook@chromium.org>]

On Tue, Jan 10, 2017 at 12:40 AM, Vaishali Thakkar
<vaishali.thakkar@oracle.com> wrote:
> On Tuesday 10 January 2017 01:51 PM, Pengfei Wang wrote:
>>
>>
>>> 在 2017年1月10日，上午1:05，Vaishali Thakkar <vaishali.thakkar@oracle.com> 写道：
>>>
>>> On Tuesday 27 December 2016 11:51 PM, Julia Lawall wrote:
>>>>
>>>> I totally dropped the ball on this.  Many thanks to Vaishali for
>>>> resurrecting it.
>>>>
>>>> Some changes are suggested below.
>>>>
>>>> On Tue, 26 Apr 2016, Kees Cook wrote:
>>>>
>>>>> This is usually a sign of a resized request. This adds a check for
>>>>> potential races or confusions. The check isn't 100% accurate, so it
>>>>> needs some manual review.
>>>>>
>>>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>>>> ---
>>>>> scripts/coccinelle/tests/reusercopy.cocci | 36
>>>>> +++++++++++++++++++++++++++++++
>>>>> 1 file changed, 36 insertions(+)
>>>>> create mode 100644 scripts/coccinelle/tests/reusercopy.cocci
>>>>>
>>>>> diff --git a/scripts/coccinelle/tests/reusercopy.cocci
>>>>> b/scripts/coccinelle/tests/reusercopy.cocci
>>>>> new file mode 100644
>>>>> index 000000000000..53645de8ae95
>>>>> --- /dev/null
>>>>> +++ b/scripts/coccinelle/tests/reusercopy.cocci
>>>>> @@ -0,0 +1,36 @@
>>>>> +/// Recopying from the same user buffer frequently indicates a pattern
>>>>> of
>>>>> +/// Reading a size header, allocating, and then re-reading an entire
>>>>> +/// structure. If the structure's size is not re-validated, this can
>>>>> lead
>>>>> +/// to structure or data size confusions.
>>>>> +///
>>>>> +// Confidence: Moderate
>>>>> +// Copyright: (C) 2016 Kees Cook, Google. License: GPLv2.
>>>>> +// URL: http://coccinelle.lip6.fr/
>>>>> +// Comments:
>>>>> +// Options: -no_includes -include_headers
>>>>
>>>>
>>>> The options could be: --no-include --include-headers
>>>>
>>>> Actually, Coccinelle supports both, but it only officially supports the
>>>> -- versions.
>>>>
>>>>> +
>>>>> +virtual report
>>>>> +virtual org
>>>>
>>>>
>>>> Add, the following for the *s:
>>>>
>>>> virtual context
>>>>
>>>> Then add the following rule:
>>>>
>>>> @ok@
>>>> position p;
>>>> expression src,dest;
>>>> @@
>>>>
>>>> copy_from_user@p(&dest, src, sizeof(dest))
>>>>
>>>>> +
>>>>> +@cfu_twice@
>>>>> +position p;
>>>>
>>>>
>>>> Change this to:
>>>>
>>>> position p != ok.p;
>>>>
>>>>> +identifier src;
>>>>> +expression dest1, dest2, size1, size2, offset;
>>>>> +@@
>>>>> +
>>>>> +*copy_from_user(dest1, src, size1)
>>>>> + ... when != src = offset
>>>>> +     when != src += offset
>>>
>>>
>>> Here, may be we should add few more lines from Pengfei's
>>> script to avoid th potential FPs.
>>>
>>>> Add the following lines:
>>>>
>>>>     when != if (size2 > e1 || ...) { ... return ...; }
>>>>     when != if (size2 > e1 || ...) { ... size2 = e2 ... }
>>>>
>>>> These changes drop cases where the last argument to copy_from_usr is the
>>>> size of the first argument, which seems safe enough, and where there is
>>>> a
>>>> test on the size value that can either update it or abort the function.
>>>> These changes only eliminate false positives, as far as I could tell.
>>>>
>>>> If it would be more convenient, I could just send the complete revised
>>>> patch, or whatever seems convenient.
>>>
>>>
>>> I was also thinking that probably we should also add other user space
>>> memory API functions. May be get_user and strncpy_from_user. Although I'm
>>> not sure how common it is to find such patterns for both of these functions.
>>
>>
>> I strongly recommend you adding get_user() API , which is used pervasively
>> within the kernel just like copy_from user().
>
>
> Sure. I have changed regetuser-wang.cocci from Kees's RFC patches to
> include everything in the pattern matching rule. I'll send that as well.
>
>> In many situations, there is a combination use, get_user() copies first
>> then
>> followed by a copy_from_user() copy. According to our investigation, this
>> typical
>> situation works by get_user() firstly copying a field of a specific struct
>> to check,
>> then copy_from_user() copies in the whole struct to use. Of course, the
>> struct
>> field is fetch twice.
>
>
> Do you mean that there is a problem when we have get_user() followed by
> copy_from_user()? Basically something like
> this:
>
> get_user(..., src.arg) //where src.arg = field of a structure
> ...
> copy_from_user(..., src, ...) //where src is a whole structure
>
> If that is the case then we would need to have one more new script
> or rule for such kind of combinational patterns. Disjunction can
> probably give FPs.

Yup, we need a single script: I just split them into three for comparisons.

-Kees

-- 
Kees Cook
Nexus Security