Re: [Xen-devel] [PATCH v7 1/8] public / x86: Introduce __HYPERCALL_dm_op...

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 24 January 2017 10:55
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Jennifer Herbert
> <jennifer.herbert@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: RE: [PATCH v7 1/8] public / x86: Introduce __HYPERCALL_dm_op...
> 
> >>> On 24.01.17 at 11:19, <Paul.Durrant@xxxxxxxxxx> wrote:
> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> Sent: 24 January 2017 10:00
> >> >>> On 23.01.17 at 14:59, <paul.durrant@xxxxxxxxxx> wrote:
> >> > +static bool copy_buf_from_guest(xen_dm_op_buf_t bufs[],
> >> > +                                unsigned int nr_bufs, void *dst,
> >> > +                                unsigned int idx, size_t dst_size)
> >> > +{
> >> > +    if ( dst_size != bufs[idx].size )
> >> > +        return false;
> >> > +
> >> > +    return !copy_from_guest(dst, bufs[idx].h, dst_size);
> >> > +}
> >> > +
> >> > +static bool copy_buf_to_guest(xen_dm_op_buf_t bufs[],
> >> > +                              unsigned int nr_bufs, unsigned int idx,
> >> > +                              void *src, size_t src_size)
> >> > +{
> >> > +    if ( bufs[idx].size != src_size )
> >> > +        return false;
> >> > +
> >> > +    return !copy_to_guest(bufs[idx].h, src, bufs[idx].size);
> >> > +}
> >>
> >> What are the "nr_bufs" parameters good for in both of these?
> >
> > Good point. I'm not sure what happened to the range check. I think it
> would
> > still be good to have one.
> 
> Which range check? You now validated nr_bufs quite a bit earlier, iirc.
> 

I do, but I thought it might still be worth putting one in these helper 
functions too.

> >> > +static int dm_op(domid_t domid,
> >> > +                 unsigned int nr_bufs,
> >> > +                 xen_dm_op_buf_t bufs[])
> >> > +{
> >> > +    struct domain *d;
> >> > +    struct xen_dm_op op;
> >> > +    bool const_op = true;
> >> > +    long rc;
> >> > +
> >> > +    rc = rcu_lock_remote_domain_by_id(domid, &d);
> >> > +    if ( rc )
> >> > +        return rc;
> >> > +
> >> > +    if ( !has_hvm_container_domain(d) )
> >> > +        goto out;
> >> > +
> >> > +    rc = xsm_dm_op(XSM_DM_PRIV, d);
> >> > +    if ( rc )
> >> > +        goto out;
> >> > +
> >> > +    if ( !copy_buf_from_guest(bufs, nr_bufs, &op, 0, sizeof(op)) )
> >>
> >> I'm afraid my request to have an exact size check in the copy
> >> functions was going a little too far for this particular instance: Just
> >> consider what would happen for a tool stack built with just this one
> >> patch in place, but run against a hypervisor with at least one more
> >> applied.
> >
> > I was wondering about that. I assumed that you were expecting the
> hypervisor
> > and libxc to be kept in lock-step.
> >
> >> We can of course keep things the way they are here, but
> >> then we'll need a placeholder added to the structure right away
> >> (like is e.g. the case for domctl/sysctl). Every sub-structure should
> >> then be checked to not violate that constraint by a BUILD_BUG_ON()
> >> in its respective function (I'd prefer that over a single verification
> >> of the entire structure/union, as that would more clearly pinpoint
> >> a possible offender).
> >>
> >
> > Can I not revert things to the min size check that we had before. Surely
> > that is preferable over the above complexity?
> 
> I'm not sure I see much complexity there. Reverting to the min()
> has the down side that you need to fill the not copied part (an
> extra memset) to avoid acting on uninitialized data (which may
> result in an indirect information leak). Iirc you didn't have any
> precaution like this in that earlier variant. But if that aspect is
> taken care of, I'm not overly opposed to the alternative.
> 

Yes, the memset was there IIRC. I'll go back to that mechanism then.

> >> > --- a/xen/include/xlat.lst
> >> > +++ b/xen/include/xlat.lst
> >> > @@ -56,6 +56,7 @@
> >> >  ?       grant_entry_header              grant_table.h
> >> >  ?       grant_entry_v2                  grant_table.h
> >> >  ?       gnttab_swap_grant_ref           grant_table.h
> >> > +!       dm_op_buf                       hvm/dm_op.h
> >> >  ?       vcpu_hvm_context                hvm/hvm_vcpu.h
> >> >  ?       vcpu_hvm_x86_32                 hvm/hvm_vcpu.h
> >> >  ?       vcpu_hvm_x86_64                 hvm/hvm_vcpu.h
> >>
> >> While for this patch the addition here is sufficient, subsequent
> >> patches should add their sub-structures here with a leading ?,
> >> and you'd then need to invoke the resulting CHECK_* macros
> >> somewhere. I don't think we should leave those structures
> >> unchecked.
> >
> > OK, I can add this if you think it is necessary.
> 
> Yes please - we should have such stuff in place to aid in avoiding
> of mistakes; some of the earlier omitted padding would likely have
> been caught by such checks.

That's true.

  Paul

> 
> Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel