[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH HVM v2 1/1] hvm: refactor set param
On 2/9/21 10:40 AM, Jan Beulich wrote: > CAUTION: This email originated from outside of the organization. Do not click > links or open attachments unless you can confirm the sender and know the > content is safe. > > > > On 08.02.2021 20:47, Norbert Manthey wrote: >> On 2/8/21 3:21 PM, Jan Beulich wrote: >>> On 05.02.2021 21:39, Norbert Manthey wrote: >>>> @@ -4108,6 +4108,13 @@ static int hvm_allow_set_param(struct domain *d, >>>> if ( rc ) >>>> return rc; >>>> >>>> + if ( index >= HVM_NR_PARAMS ) >>>> + return -EINVAL; >>>> + >>>> + /* Make sure we evaluate permissions before loading data of domains. >>>> */ >>>> + block_speculation(); >>>> + >>>> + value = d->arch.hvm.params[index]; >>>> switch ( index ) >>>> { >>>> /* The following parameters should only be changed once. */ >>> I don't see the need for the heavier block_speculation() here; >>> afaict array_access_nospec() should do fine. The switch() in >>> context above as well as the switch() further down in the >>> function don't have any speculation susceptible code. >> The reason to block speculation instead of just using the hardened index >> access is to not allow to speculatively load data from another domain. > Okay, looks like I got mislead by the added bounds check. Why > do you add that, when the sole caller already has one? It'll > suffice since you move the array access past the barrier, > won't it? I can drop that bound check again. This was added to make sure other callers would be save as well. Thinking about this a little more, the check could actually be moved into the hvm_allow_set_param function, above the first index access in that function. Are there good reasons to not move the index check into the allow function? > >>>> @@ -4141,6 +4148,9 @@ static int hvm_set_param(struct domain *d, uint32_t >>>> index, uint64_t value) >>>> if ( rc ) >>>> return rc; >>>> >>>> + /* Make sure we evaluate permissions before loading data of domains. >>>> */ >>>> + block_speculation(); >>>> + >>>> switch ( index ) >>>> { >>>> case HVM_PARAM_CALLBACK_IRQ: >>> Like you do for the "get" path I think this similarly renders >>> pointless the use in hvmop_set_param() (and - see below - the >>> same consideration wrt is_hvm_domain() applies). >> Can you please be more specific why this is pointless? I understand that >> the is_hvm_domain check comes with a barrier that can be used to not add >> another barrier. However, I did not find such a barrier here, which >> comes between the 'if (rc)' just above, and the potential next access >> based on the value of 'index'. At least the access behind the switch >> statement cannot be optimized and replaced with a constant value easily. > I'm suspecting a misunderstanding (the more that further down > you did agree to what I've said for hvmop_get_param()): I'm > not saying your addition is pointless. Instead I'm saying that > your addition should be accompanied by removal of the barrier > from hvmop_set_param(), paralleling what you do to > hvmop_get_param(). And additionally I'm saying that just like > in hvmop_get_param() the barrier there was already previously > redundant with that inside is_hvm_domain(). I now understand, thank you. I agree, the already existing barrier in the hvmop_set_param function can be dropped as well. I will update the diff accordingly, after we concluded where to put the index check. Best, Norbert Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |