Re: [PATCH v4 for-4.14] x86/monitor: revert default behavior when monitoring register write events

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Mon, Jun 8, 2020 at 5:14 PM Razvan Cojocaru
<rcojocaru@xxxxxxxxxxxxxxx> wrote:
>
> On 6/9/20 1:50 AM, Tamas K Lengyel wrote:
> > On Mon, Jun 8, 2020 at 3:16 PM Razvan Cojocaru
> > <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> >> 1. A security application that is unable to _prevent_ things being done
> >> to a system is not doing a very good job, since in that case you can
> >> only collect stats and not veto anything. I would argue that the default
> >> for such a monitoring application should be the current one (all events
> >> should be pre-action).
> >
> > Not all security applications require this though. Malware analysis
> > where stealth is required would absolutely not want this side-effect
> > to be visible to the guest where malware could use it to determine
> > that it's being monitored. So I don't buy into this argument.
>
> Fair enough, in that case having both models supported should be fine.
> I'll leave the rest of that conversation to my colleagues.
>
> >> 2. This is further supported by the fact that that's exactly how the EPT
> >> vm_events work: you get a "I want to write to this page" event _before_
> >> the write occurs. If register writes behave differently, you have _two_
> >> different models: one where you get an event post-action, and one where
> >> you get one pre-action.
> >
> > Whether you get an event before or after the effects of the event have
> > been applied to the system state shouldn't matter as long as you can
> > revert that action. I wouldn't care either way to be the default. But
> > having a default that breaks other use-cases is unacceptable.
>
> You keep saying that as if I disagree. :) But we've already established
> that the potential for a race condition has been found and needs to be
> fixed.
>
> My only (minor) objection has been that a patch fixing the current model
> would have been preferable to one that switches the default as a
> workaround. Still, it's understandable that perhaps there's no time or
> motivation for that.

I've already sent two other patches that make it more manageable to
disable the interface when this feature is used. Your colleagues are
welcome to pick those up or send other fixes that they prefer. As I
don't use this feature I won't be spending more time fixing it then
what I've already spent on it. At this point collectively I probably
spent weeks trying to just track the issue down as it was such an
annoying bug to find.

Tamas