[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 2/2] evtchn/fifo: don't enforce higher than necessary alignment
On 15.07.2020 12:46, Julien Grall wrote:
> On Wed, 15 Jul 2020, 12:17 Jan Beulich, <jbeulich@xxxxxxxx> wrote:
>
>> Neither the code nor the original commit provide any justification for
>> the need to 8-byte align the struct in all cases. Enforce just as much
>> alignment as the structure actually needs - 4 bytes - by using alignof()
>> instead of a literal number.
>>
>> Take the opportunity and also
>> - add so far missing validation that native and compat mode layouts of
>> the structures actually match,
>> - tie sizeof() expressions to the types of the fields we're actually
>> after, rather than specifying the type explicitly (which in the
>> general case risks a disconnect, even if there's close to zero risk in
>> this particular case),
>> - use ENXIO instead of EINVAL for the two cases of the address not
>> satisfying the requirements, which will make an issue here better
>> stand out at the call site.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>> I question the need for the array_index_nospec() here. Or else I'd
>> expect map_vcpu_info() would also need the same.
>>
>> --- a/xen/common/event_fifo.c
>> +++ b/xen/common/event_fifo.c
>> @@ -504,6 +504,16 @@ static void setup_ports(struct domain *d
>> }
>> }
>>
>> +#ifdef CONFIG_COMPAT
>> +
>> +#include <compat/event_channel.h>
>> +
>> +#define xen_evtchn_fifo_control_block evtchn_fifo_control_block
>> +CHECK_evtchn_fifo_control_block;
>> +#undef xen_evtchn_fifo_control_block
>> +
>> +#endif
>> +
>> int evtchn_fifo_init_control(struct evtchn_init_control *init_control)
>> {
>> struct domain *d = current->domain;
>> @@ -523,19 +533,20 @@ int evtchn_fifo_init_control(struct evtc
>> return -ENOENT;
>>
>> /* Must not cross page boundary. */
>> - if ( offset > (PAGE_SIZE - sizeof(evtchn_fifo_control_block_t)) )
>> - return -EINVAL;
>> + if ( offset > (PAGE_SIZE - sizeof(*v->evtchn_fifo->control_block)) )
>> + return -ENXIO;
>>
>> /*
>> * Make sure the guest controlled value offset is bounded even during
>> * speculative execution.
>> */
>> offset = array_index_nospec(offset,
>> - PAGE_SIZE -
>> sizeof(evtchn_fifo_control_block_t) + 1);
>> + PAGE_SIZE -
>> + sizeof(*v->evtchn_fifo->control_block) +
>> 1);
>>
>> - /* Must be 8-bytes aligned. */
>> - if ( offset & (8 - 1) )
>> - return -EINVAL;
>> + /* Must be suitably aligned. */
>> + if ( offset & (alignof(*v->evtchn_fifo->control_block) - 1) )
>> + return -ENXIO;
>>
>
> A guest relying on this new alignment wouldn't work on older version of
> Xen. So I don't think a guest would ever be able to use it.
>
> Therefore is it really worth the change?
That's the question. One of your arguments for using a literal number
also for the vCPU info mapping check was that here a literal number
is used. The goal isn't so much relaxation of the interface, but
making the code consistent as well as eliminating a (how I'd call it)
kludge.
Your commit message lead to think the relaxation is the key motivation to change the code.
Guests not caring to be able to run on older versions could also make
use of the relaxation (which may be more relevant in 10 y ears time
than it is now).
That makes sense. However, I am a bit concerned that an OS developer may not notice the alignment problem with older version.
I would suggest to at least document the alignment expected in the public header.
|
