On 23/04/2021 13:53, Jan Beulich wrote: > On 23.04.2021 14:35, Roger Pau Monne wrote: >> Remove the unneeded usage of the compat layer to copy frame pointers >> from guest address space. Instead just use raw_copy_from_guest. >> >> Reported-by: Andrew Cooper <[email protected]> >> Signed-off-by: Roger Pau Monné <[email protected]> >> --- >> Just build tested. Note sure I'm missing something, since using the >> compat layer here was IMO much more complicated than just using the >> raw accessors. > The main reason, I suppose, was that raw_copy_*() aren't supposed to > be used directly. > >> @@ -59,34 +56,17 @@ dump_guest_backtrace(struct vcpu *vcpu, const struct >> frame_head *head, >> { >> frame_head_t bufhead; >> >> -#ifdef CONFIG_COMPAT >> if ( is_32bit_vcpu(vcpu) ) >> { >> - DEFINE_COMPAT_HANDLE(frame_head32_t); >> - __compat_handle_const_frame_head32_t guest_head = >> - { .c = (unsigned long)head }; > You're losing the truncation to 32 bits here. > >> frame_head32_t bufhead32; >> >> - /* Also check accessibility of one struct frame_head beyond */ >> - if (!compat_handle_okay(guest_head, 2)) >> - return 0; > If you intentionally remove this and ... > >> - if (__copy_from_compat(&bufhead32, guest_head, 1)) >> + if (raw_copy_from_guest(&bufhead32, head, sizeof(bufhead32))) >> return 0; >> bufhead.ebp = (struct frame_head *)(unsigned long)bufhead32.ebp; >> bufhead.ret = bufhead32.ret; >> } >> - else >> -#endif >> - { >> - XEN_GUEST_HANDLE_PARAM(const_frame_head_t) guest_head = >> - const_guest_handle_from_ptr(head, frame_head_t); >> - >> - /* Also check accessibility of one struct frame_head beyond */ >> - if (!guest_handle_okay(guest_head, 2)) >> - return 0; > ... this, then you should justify why these aren't needed anymore > (or maybe were never really needed). They've been put there for a > purpose, I'm sure, even if I'm unclear about what one it was/is.
I don't see what purpose they serve at all. The OK checks only look for encroachment into the Xen virtual range. The two cases where this does anything, is userspace with a stack immediately adjacent to the lower canonical boundary (which doesn't happen in practice because of the astounding number of errata there), or for PV32 kernels with a stack at the legacy Xen boundary. ~Andrew
