Re: [Xen-devel] [PATCH v5] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.10.17 at 15:00, <ppircalabu@xxxxxxxxxxxxxxx> wrote:
> On Vi, 2017-10-13 at 03:51 -0600, Jan Beulich wrote:
>> > 
>> > 
>> > +            BUILD_BUG_ON(sizeof(struct
>> > xen_hvm_altp2m_set_mem_access_multi) <
>> > +                         sizeof(struct
>> > compat_hvm_altp2m_set_mem_access_multi));
>> What good does this do?
>> Sorry, I don't understand how these size checks should look (are we
>> testing that the left hand side is at least as wide as the right hand
>> side?). Could you please give an example? Thanks!
> 
>> > +     */
>> > +    nat.altp2m_op->version  = a.version;
>> > +    nat.altp2m_op->cmd      = a.cmd;
>> > +    nat.altp2m_op->domain   = a.domain;
>> > +    nat.altp2m_op->pad1     = a.pad1;
>> > +    nat.altp2m_op->pad2     = a.pad2;
>> There are _still_ no size checks here.
> I'm not sure I understand what kind of size checks should be used here.
> Are you expecting something similar with the CHECK_FIELD_ macro?

Yes, as I've been stating repeatedly. You want to demonstrate
that field sizes match, or that at least no truncation can occur.

>> > 
>> > +            if ( nat.altp2m_op->u.set_mem_access_multi.opaque > 0
>> > )
>> Please also check rc here to avoid needlessly copying back. In
>> fact _only_ checking rc ought to be fine.
> Actually, in this case rc is overwritten in do_altp2m_op (-EFAULT if
> the copy didn't work or the result of hypercall_create_continuation
> otherwise).

And it's the result of hypercall_create_continuation() which you
want to check against (provided there's no other way for rc to
obtain a positive value).

Jan


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