Re: [PATCH for-4.20] hvmloader: Rework hypercall infrastructure

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Jul 25, 2024 at 08:18:27AM +0200, Jan Beulich wrote:
> On 24.07.2024 19:24, Andrew Cooper wrote:
> > On 17/07/2024 1:37 pm, Jan Beulich wrote:
> >> On 17.07.2024 13:12, Andrew Cooper wrote:
> >>>  static inline int
> >>>  hypercall_sched_op(
> >>>      int cmd, void *arg)
> >>>  {
> >>> -    return _hypercall2(int, sched_op, cmd, arg);
> >>> +    return _hypercall2(int, __HYPERVISOR_sched_op, cmd, arg);
> >>>  }
> >> I know you don't really like token concatenation in cases like these ones,
> >> but these adjustments all don't look as if they were necessary right here.
> >> The new macros use the macro parameter only in ways where token pasting
> >> would continue to work, afaics. And in the new assembly file you use very
> >> similar token pasting anyway.
> > 
> > That's because my judgement is not about simply tokenisation (or not). 
> > It's about not using ## when it is likely to interfere with
> > grep/cscope/tags/etc.
> > 
> > The assembly file both isn't really interesting to find this way, and
> > needs ## in order to work the way it does (differing prefixes in the
> > hypercall names).
> 
> The "not interesting" part is very subjective. If one really wanted to find
> everything, this still would get in the way. And there being differing
> prefixes in the other case would then simply require passing two arguments
> to the macro instead of just one (which imo is undesirable, but that is
> along with considering the change above undesirable as well).
> 
> I'm not bothered enough to request to undo this altogether, or to split
> this into a separate change, but I wonder whether in the opposite case you
> wouldn't demand either or even both.
> 
> >>> +        .align 4096
> >> PAGE_SIZE? And then again ...
> >>
> >>> +        .globl hypercall_page
> >>> +hypercall_page:
> >>> +         /* Poisoned with `ret` for safety before hypercalls are set up. 
> >>> */
> >>> +        .fill 4096, 1, 0xc3
> >>> +        .type hypercall_page, STT_OBJECT
> >>> +        .size hypercall_page, 4096
> >> ... here?
> > 
> > HVMLoader doesn't have a suitable constant, and doesn't have _AC().
> > 
> > Although we probably can just get away with (1 << PAGE_SHIFT) and drop
> > the ul.
> 
> Oh, right, we still mean to be compatible with gas not ignoring the ul
> suffix on numbers.
> 
> >> As to the "poisoning" - how does RET provide any safety? If a call happened
> >> early, the uncertainty of what %eax is set to would make it unpredictable
> >> how such a caller would further behave. Imo better to crash / hang in such
> >> a case, perhaps by using INT3 instead.
> >>
> >> I notice that matches earlier behavior, but there the comment at least
> >> validly said "rendering them no-ops" (yet still without considering
> >> possible problems resulting from doing so).
> > 
> > That's a complicated one.  I can't remember why I chose that exact
> > phraseology, but it's not really about accidentally-too-early case, it's
> > about error handling.
> > 
> > HVMLoader doesn't have an IDT, so any exception is fatal.  But that's
> > also something that ideally wants fixing (if it weren't for the fact
> > that it's more likely that I'll complete plans to remove hvmloader
> > completely before having time to do an IDT).
> > 
> > But for code which does have a panic() function, there's console_io
> > (logging) and sched_op (SHUTDOWN_crash) which you want to use just in
> > case they do work, before moving onto other methods of terminating.
> 
> All fine, but that still leaves this RET filling yielding random behavior
> at possible early call sites. At the very least the pre-fill should result
> in errors to be observed by too-early callers. For this to at least vaguely
> resemble something valid to call "poisoned", that is.

FWIW, I've not long ago [0] switched FreeBSD hypercall page to be
poisoned with int3, as it was previously filled with nops which
resulted in very funny traces when the hypercall page was used prior
to being initialized.

I also considered filling with ret, but I think that's likely to
mask or make uninitialized usages harder to spot.  It's IMO better to
get a triple-fault than silence attempts to use the hypercall page too
early.

Thanks, Roger.

[0] 
https://cgit.freebsd.org/src/commit/?id=e283c994ab270706142ef5dde9092950000af901