Re: [PATCH v2 3/3] livepatch: force transition process to finish

[Josh Poimboeuf]

On Wed, Aug 30, 2017 at 02:48:24PM +0200, Miroslav Benes wrote:
> On Wed, 30 Aug 2017, Pavel Machek wrote:
> 
> > On Thu 2017-08-10 12:48:15, Miroslav Benes wrote:
> > > If a task sleeps in a set of patched functions uninterruptibly, it could
> > > block the whole transition process indefinitely.  Thus it may be useful
> > > to clear its TIF_PATCH_PENDING to allow the process to finish.
> > > 
> > > Admin can do that now by writing to force sysfs attribute in livepatch
> > > sysfs directory. TIF_PATCH_PENDING is then cleared for all tasks and the
> > > transition can finish successfully.
> > > 
> > > Important note! Use wisely. Admin must be sure that it is safe to
> > > execute such action. This means that it must be checked that by doing so
> > > the consistency model guarantees are not violated.
> > 
> > Yes, that's what admins are good for. Magically determining what state
> > their machine is in, and deciding if all the processes are in the sane
> > state and what the consequences are. They have all the tools they need
> > to do that, like JTAG connection to the CPU and about 10 years of
> > time... do they?
> 
> It is not that bad in this case fortunately. The decision depends on
> 
> 1. which processes are not yet migrated and why (which functions they're 
> sleeping on)
> 
> AND
> 
> 2. what functions are patched and what is the nature of a fix.
> 
> Admin knows 1. and he needs input from a livepatch author about 2. There 
> is no magic there, but cooperation between those two is definitely 
> required.

As we said before, we have to trust the admin knows what they're doing.
If the vendor didn't tell them to force the patch, they shouldn't be
forcing it.  Just like they shouldn't be doing "sudo rm -rf /".

And the last time I checked, there's no taint for logging in as root.

> > This should taint the kernel at the very least.
> 
> That's what we discussed in v1. In cases where the patch does not need the 
> consistency model at all, the taint would be too much. But I have no 
> problem to transform pr_warn to WARN_ON_ONCE.

As I mentioned before, false warnings can have unintended negative
consequences as well.

But anyway, if you do decide to make it a warning, I think the ONCE
wouldn't be helpful.  It should just be WARN_ON.  But I still vote
against it.

> > It should also require capabilities beyond "normal root", because it
> > allows malicious admin to do "bad things (tm)" to the kernel.

What exactly do you consider to be beyond "normal root"???

> I think that malicious admin can do pretty bad (and more serious) things 
> even without this. That is not an excuse, but it is general problem not 
> specific to this patch.

Right.  A malicious admin with root access can do a *lot* worse than
force loading a patch.  A hacker who just obtained root access isn't
going to say "Finally! I can force load that stalled CVE fix!"

-- 
Josh