Re: [Cocci] Coccinelle rule for CVE-2019-18683

Re: [Cocci] Coccinelle rule for CVE-2019-18683

[Markus Elfring]

> Do you have any idea how to improve it?

I see further software development possibilities of varying relevance
also for this script of the semantic patch language.

* The SmPL variables “lock_p”, “unlock_p” and “stop_p” could be declared
  in a more succinct way just by listing them in the same statement.

* The source code search pattern can be too generic.
  How do you think about to consider additional constraints
  for safer data control flow analysis?

* Other operation modes might become helpful.

Regards,
Markus

Re: [Cocci] Coccinelle rule for CVE-2019-18683

[Alexander Popov]

Markus, thanks for your remarks!

On 09.04.2020 11:41, Markus Elfring wrote:
> * The source code search pattern can be too generic.
>   How do you think about to consider additional constraints
>   for safer data control flow analysis?

Could you please elaborate on that?

I used 'exists' keyword to find at least one branch that has
mutex_unlock+kthread_stop+mutex_lock chain.

> * Other operation modes might become helpful.

Thanks! I added 'context' mode, it's very good for this purpose.

Best regards,
Alexander

Re: [Cocci] Coccinelle rule for CVE-2019-18683

[Markus Elfring]

>> * The source code search pattern can be too generic.
>>   How do you think about to consider additional constraints
>>   for safer data control flow analysis?
>
> Could you please elaborate on that?

Julia Lawall chose to mention the design possibility “put when
!= mutex_lock(E) after the ...”.
https://systeme.lip6.fr/pipermail/cocci/2020-April/007107.html
https://lore.kernel.org/cocci/alpine.DEB.2.21.2004091248190.2403@hadrien/


> I used 'exists' keyword to find at least one branch that has
> mutex_unlock+kthread_stop+mutex_lock chain.

Are you informed about development challenges for data flow analysis
(or even escape analysis according to computer science)?

How many experiences can be reused from other known approaches?

Regards,
Markus

Re: [Cocci] Coccinelle rule for CVE-2019-18683

[Julia Lawall]

> >> kthread_stop@stop_p(...)
> >> ...
> >> mutex_lock@lock_p(E)
> >>
> >> @script:python@
> >> stop_p << race.stop_p;
> >> unlock_p << race.unlock_p;
> >> lock_p << race.lock_p;
> >> E << race.E;
> >> @@
> >>
> >> coccilib.report.print_report(unlock_p[0], 'mutex_unlock(' + E + ') here')
> >> coccilib.report.print_report(stop_p[0], 'kthread_stop here')
> >> coccilib.report.print_report(lock_p[0], 'mutex_lock(' + E + ') here\n')
>
> ...
>
> > Based on Jann's suggestion, it seem like it could be interesting to find
> > these locking pauses, and then collect functions that are used in locks
> > and in lock pauses.  If a function is mostly used with locks held, then
> > using it in a lock pause could be a sign of a bug.  I will see if it turns
> > up anything interesting.
>
> Do you mean collecting the behaviour that happens between unlocking and locking
> and then analysing it somehow?

Yes.  I have tried doing what I described, but I'm not sure that the
results are very reliable at the moment.

julia

Re: [Cocci] Coccinelle rule for CVE-2019-18683

[Alexander Popov]

On 09.04.2020 13:53, Julia Lawall wrote:
> On Thu, 9 Apr 2020, Alexander Popov wrote:
>> virtual report
>>
>> @race exists@
>> expression E;
>> position stop_p;
>> position unlock_p;
>> position lock_p;
>> @@
>>
>> mutex_unlock@unlock_p(E)
>> ...
> 
> It would be good to put when != mutex_lock(E) after the ... above.  Your
> rule doesn't actually prevent the lock from being retaken.

Thanks Julia! I used this trick in the second version of the rule that I've just
sent.

>> kthread_stop@stop_p(...)
>> ...
>> mutex_lock@lock_p(E)
>>
>> @script:python@
>> stop_p << race.stop_p;
>> unlock_p << race.unlock_p;
>> lock_p << race.lock_p;
>> E << race.E;
>> @@
>>
>> coccilib.report.print_report(unlock_p[0], 'mutex_unlock(' + E + ') here')
>> coccilib.report.print_report(stop_p[0], 'kthread_stop here')
>> coccilib.report.print_report(lock_p[0], 'mutex_lock(' + E + ') here\n')

...

> Based on Jann's suggestion, it seem like it could be interesting to find
> these locking pauses, and then collect functions that are used in locks
> and in lock pauses.  If a function is mostly used with locks held, then
> using it in a lock pause could be a sign of a bug.  I will see if it turns
> up anything interesting.

Do you mean collecting the behaviour that happens between unlocking and locking
and then analysing it somehow?

Best regards,
Alexander

Re: [Cocci] Coccinelle rule for CVE-2019-18683

[Julia Lawall]

On Thu, 9 Apr 2020, Alexander Popov wrote:

> Hello!
>
> Some time ago I fixed CVE-2019-18683 in the V4L2 subsystem of the Linux kernel.
>
> I created a Coccinelle rule that detects that bug pattern. Let me show it.

Thanks for the discussion :)

>
>
> Bug pattern
> ===========
>
> CVE-2019-18683 refers to three similar vulnerabilities caused by the same
> incorrect approach to locking that is used in vivid_stop_generating_vid_cap(),
> vivid_stop_generating_vid_out(), and sdr_cap_stop_streaming().
>
> For fixes please see the commit 6dcd5d7a7a29c1e4 (media: vivid: Fix wrong
> locking that causes race conditions on streaming stop).
>
> These three functions are called during streaming stopping with vivid_dev.mutex
> locked. And they all do the same mistake while stopping their kthreads, which
> need to lock this mutex as well. See the example from
> vivid_stop_generating_vid_cap():
>     /* shutdown control thread */
>     vivid_grab_controls(dev, false);
>     mutex_unlock(&dev->mutex);
>     kthread_stop(dev->kthread_vid_cap);
>     dev->kthread_vid_cap = NULL;
>     mutex_lock(&dev->mutex);
>
> But when this mutex is unlocked, another vb2_fop_read() can lock it instead of
> the kthread and manipulate the buffer queue. That causes use-after-free.
>
> I created a Coccinelle rule that detects mutex_unlock+kthread_stop+mutex_lock
> within one function.
>
>
> Coccinelle rule
> ===============
>
> virtual report
>
> @race exists@
> expression E;
> position stop_p;
> position unlock_p;
> position lock_p;
> @@
>
> mutex_unlock@unlock_p(E)
> ...

It would be good to put when != mutex_lock(E) after the ... above.  Your
rule doesn't actually prevent the lock from being retaken.

> kthread_stop@stop_p(...)
> ...
> mutex_lock@lock_p(E)
>
> @script:python@
> stop_p << race.stop_p;
> unlock_p << race.unlock_p;
> lock_p << race.lock_p;
> E << race.E;
> @@
>
> coccilib.report.print_report(unlock_p[0], 'mutex_unlock(' + E + ') here')
> coccilib.report.print_report(stop_p[0], 'kthread_stop here')
> coccilib.report.print_report(lock_p[0], 'mutex_lock(' + E + ') here\n')
>
>
> Testing the rule
> ================
>
> I reverted the commit 6dcd5d7a7a29c1e4 and called:
> COCCI=./scripts/coccinelle/kthread_race.cocci make coccicheck MODE=report
>
> The result:
>
> ./drivers/media/platform/vivid/vivid-kthread-out.c:347:1-13: mutex_unlock(& dev
> -> mutex) here
> ./drivers/media/platform/vivid/vivid-kthread-out.c:348:1-13: kthread_stop here
> ./drivers/media/platform/vivid/vivid-kthread-out.c:350:1-11: mutex_lock(& dev ->
> mutex) here
>
> ./drivers/media/platform/vivid/vivid-sdr-cap.c:306:1-13: mutex_unlock(& dev ->
> mutex) here
> ./drivers/media/platform/vivid/vivid-sdr-cap.c:307:1-13: kthread_stop here
> ./drivers/media/platform/vivid/vivid-sdr-cap.c:309:1-11: mutex_lock(& dev ->
> mutex) here
>
> ./drivers/media/platform/vivid/vivid-kthread-cap.c:1001:1-13: mutex_unlock(& dev
> -> mutex) here
> ./drivers/media/platform/vivid/vivid-kthread-cap.c:1002:1-13: kthread_stop here
> ./drivers/media/platform/vivid/vivid-kthread-cap.c:1004:1-11: mutex_lock(& dev
> -> mutex) here
>
> There are no other bugs detected.
>
> Do you have any idea how to improve it?
> Do we need that rule for regression testing in the upstream?

Based on Jann's suggestion, it seem like it could be interesting to find
these locking pauses, and then collect functions that are used in locks
and in lock pauses.  If a function is mostly used with locks held, then
using it in a lock pause could be a sign of a bug.  I will see if it turns
up anything interesting.

julia