On 27.04.2021 16:21, 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.
>
> While there change the accessibility check of one frame_head beyond to
> be performed as part of the copy, like it's done in the Linux code.
> Note it's unclear why this is needed.
>
> Also drop the explicit truncation of the head pointer in the 32bit
> case as all callers already pass a zero extended value. The first
> value being rsp from the guest registers, and further calls will use
> ebp from frame_head_32bit struct.
>
> Reported-by: Andrew Cooper <andrew.coop...@citrix.com>
> Signed-off-by: Roger Pau Monné <roger....@citrix.com>
> ---
> Changes since v2:
> - Keep accessibility check for one frame_head beyond.
> - Fix coding style.
I'm indeed more comfortable with this variant, so
Acked-by: Jan Beulich <jbeul...@suse.com>
Andrew - can you live with the 2-frame thingy staying around?
> @@ -51,52 +49,35 @@ static inline int is_32bit_vcpu(struct vcpu *vcpu)
> else
> return is_pv_32bit_vcpu(vcpu);
> }
> -#endif
>
> static struct frame_head *
> dump_guest_backtrace(struct vcpu *vcpu, const struct frame_head *head,
> int mode)
> {
> - frame_head_t bufhead;
> + /* Also check accessibility of one struct frame_head beyond. */
> + frame_head_t bufhead[2];
>
> -#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 };
> - frame_head32_t bufhead32;
> -
> - /* Also check accessibility of one struct frame_head beyond */
> - if (!compat_handle_okay(guest_head, 2))
> - return 0;
> - if (__copy_from_compat(&bufhead32, guest_head, 1))
> - 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);
> + frame_head32_t bufhead32[2];
>
> - /* Also check accessibility of one struct frame_head beyond */
> - if (!guest_handle_okay(guest_head, 2))
> - return 0;
> - if (__copy_from_guest(&bufhead, guest_head, 1))
> + if ( raw_copy_from_guest(&bufhead32, head, sizeof(bufhead32)) )
As a minor remark, personally I'd prefer the & to be dropped here
and ...
> return 0;
> + bufhead[0].ebp = (struct frame_head *)(unsigned
> long)bufhead32[0].ebp;
> + bufhead[0].ret = bufhead32[0].ret;
> }
> + else if ( raw_copy_from_guest(&bufhead, head, sizeof(bufhead)) )
... here (and doing so while committing would be easy), but I'm
not going to insist.
Jan
