| Message ID | 20200416112423.25755-1-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [[PATCH,v3] ] xen/guest_access: Harden *copy_to_guest_offset() to prevent const dest operand | expand |
On 16.04.2020 13:24, Julien Grall wrote: > From: Julien Grall <jgrall@amazon.com> > > At the moment, *copy_to_guest_offset() will allow the hypervisor to copy > data to guest handle marked const. > > Thankfully, no users of the helper will do that. Rather than hoping this > can be caught during review, harden copy_to_guest_offset() so the build > will fail if such users are introduced. > > There is no easy way to check whether a const is NULL in C99. The > approach used is to introduce an unused variable that is non-const and > assign the handle. If the handle were const, this would fail at build > because without an explicit cast, it is not possible to assign a const > variable to a non-const variable. > > Suggested-by: Jan Beulich <jbeulich@suse.com> > Signed-off-by: Julien Grall <jgrall@amazon.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> with one further remark: > --- a/xen/include/asm-x86/guest_access.h > +++ b/xen/include/asm-x86/guest_access.h > @@ -87,6 +87,8 @@ > #define copy_to_guest_offset(hnd, off, ptr, nr) ({ \ > const typeof(*(ptr)) *_s = (ptr); \ > char (*_d)[sizeof(*_s)] = (void *)(hnd).p; \ > + /* Check if the handle is not const */ \ > + void *__maybe_unused _t = (hnd).p; \ Not being a native speaker, to me "if" doesn't look appropriate here. I'd use "that" instead, but you may want to confirm this. Overall then maybe "Check that the handle is not for a const type"? Jan
Hi Jan, On 16/04/2020 13:19, Jan Beulich wrote: > On 16.04.2020 13:24, Julien Grall wrote: >> From: Julien Grall <jgrall@amazon.com> >> >> At the moment, *copy_to_guest_offset() will allow the hypervisor to copy >> data to guest handle marked const. >> >> Thankfully, no users of the helper will do that. Rather than hoping this >> can be caught during review, harden copy_to_guest_offset() so the build >> will fail if such users are introduced. >> >> There is no easy way to check whether a const is NULL in C99. The >> approach used is to introduce an unused variable that is non-const and >> assign the handle. If the handle were const, this would fail at build >> because without an explicit cast, it is not possible to assign a const >> variable to a non-const variable. >> >> Suggested-by: Jan Beulich <jbeulich@suse.com> >> Signed-off-by: Julien Grall <jgrall@amazon.com> > > Reviewed-by: Jan Beulich <jbeulich@suse.com> > with one further remark: > >> --- a/xen/include/asm-x86/guest_access.h >> +++ b/xen/include/asm-x86/guest_access.h >> @@ -87,6 +87,8 @@ >> #define copy_to_guest_offset(hnd, off, ptr, nr) ({ \ >> const typeof(*(ptr)) *_s = (ptr); \ >> char (*_d)[sizeof(*_s)] = (void *)(hnd).p; \ >> + /* Check if the handle is not const */ \ >> + void *__maybe_unused _t = (hnd).p; \ > > Not being a native speaker, to me "if" doesn't look appropriate > here. I'd use "that" instead, but you may want to confirm this. > Overall then maybe "Check that the handle is not for a const type"? I am happy with the new suggestion. I will fixup while comitting it. I would also need a review from Stefano here also. Cheers,
On 17.04.2020 19:16, Julien Grall wrote: > On 16/04/2020 13:19, Jan Beulich wrote: >> On 16.04.2020 13:24, Julien Grall wrote: >>> From: Julien Grall <jgrall@amazon.com> >>> >>> At the moment, *copy_to_guest_offset() will allow the hypervisor to copy >>> data to guest handle marked const. >>> >>> Thankfully, no users of the helper will do that. Rather than hoping this >>> can be caught during review, harden copy_to_guest_offset() so the build >>> will fail if such users are introduced. >>> >>> There is no easy way to check whether a const is NULL in C99. The >>> approach used is to introduce an unused variable that is non-const and >>> assign the handle. If the handle were const, this would fail at build >>> because without an explicit cast, it is not possible to assign a const >>> variable to a non-const variable. >>> >>> Suggested-by: Jan Beulich <jbeulich@suse.com> >>> Signed-off-by: Julien Grall <jgrall@amazon.com> >> >> Reviewed-by: Jan Beulich <jbeulich@suse.com> >> with one further remark: >> >>> --- a/xen/include/asm-x86/guest_access.h >>> +++ b/xen/include/asm-x86/guest_access.h >>> @@ -87,6 +87,8 @@ >>> #define copy_to_guest_offset(hnd, off, ptr, nr) ({ \ >>> const typeof(*(ptr)) *_s = (ptr); \ >>> char (*_d)[sizeof(*_s)] = (void *)(hnd).p; \ >>> + /* Check if the handle is not const */ \ >>> + void *__maybe_unused _t = (hnd).p; \ >> >> Not being a native speaker, to me "if" doesn't look appropriate >> here. I'd use "that" instead, but you may want to confirm this. >> Overall then maybe "Check that the handle is not for a const type"? > > I am happy with the new suggestion. I will fixup while comitting it. > > > I would also need a review from Stefano here also. Would you, even under the new rules? In which case it might be a good idea to Cc him (now done here), also to given him a more direct means to object. Same would go for the x86 reviewers ... All of them were Cc-ed on v2. In light of this I can't sensibly ask that you please commit this patch soon, so that I can put mine on top, I guess (this was the original intention when starting to write this reply). Jan
Hi, On 23/04/2020 08:38, Jan Beulich wrote: > On 17.04.2020 19:16, Julien Grall wrote: >> On 16/04/2020 13:19, Jan Beulich wrote: >>> On 16.04.2020 13:24, Julien Grall wrote: >>>> From: Julien Grall <jgrall@amazon.com> >>>> >>>> At the moment, *copy_to_guest_offset() will allow the hypervisor to copy >>>> data to guest handle marked const. >>>> >>>> Thankfully, no users of the helper will do that. Rather than hoping this >>>> can be caught during review, harden copy_to_guest_offset() so the build >>>> will fail if such users are introduced. >>>> >>>> There is no easy way to check whether a const is NULL in C99. The >>>> approach used is to introduce an unused variable that is non-const and >>>> assign the handle. If the handle were const, this would fail at build >>>> because without an explicit cast, it is not possible to assign a const >>>> variable to a non-const variable. >>>> >>>> Suggested-by: Jan Beulich <jbeulich@suse.com> >>>> Signed-off-by: Julien Grall <jgrall@amazon.com> >>> >>> Reviewed-by: Jan Beulich <jbeulich@suse.com> >>> with one further remark: >>> >>>> --- a/xen/include/asm-x86/guest_access.h >>>> +++ b/xen/include/asm-x86/guest_access.h >>>> @@ -87,6 +87,8 @@ >>>> #define copy_to_guest_offset(hnd, off, ptr, nr) ({ \ >>>> const typeof(*(ptr)) *_s = (ptr); \ >>>> char (*_d)[sizeof(*_s)] = (void *)(hnd).p; \ >>>> + /* Check if the handle is not const */ \ >>>> + void *__maybe_unused _t = (hnd).p; \ >>> >>> Not being a native speaker, to me "if" doesn't look appropriate >>> here. I'd use "that" instead, but you may want to confirm this. >>> Overall then maybe "Check that the handle is not for a const type"? >> >> I am happy with the new suggestion. I will fixup while comitting it. >> >> >> I would also need a review from Stefano here also. > > Would you, even under the new rules? "2. In unusual circumstances, a more general maintainer's Ack can stand in for or even overrule a specific maintainer's Ack. Unusual circumstances might include: - The more specific maintainer has not responded either to the original patch, nor to "pings", within a reasonable amount of time. " So it depends on your definition of "reasonable amount of time". A week or two seems reasonable to me for non-pressing issues. > In which case it might be > a good idea to Cc him (now done here), also to given him a more > direct means to object. Same would go for the x86 reviewers ... > All of them were Cc-ed on v2. In light of this I can't sensibly > ask that you please commit this patch soon, so that I can put > mine on top, I guess (this was the original intention when > starting to write this reply). Ah that's why he didn't responded... I thought I CCed him but it seems I didn't call scripts/add_maintainers.pl before sending the patch. Thank you for adding the correct CC! Cheers,
On 23.04.2020 10:10, Julien Grall wrote: > Hi, > > On 23/04/2020 08:38, Jan Beulich wrote: >> On 17.04.2020 19:16, Julien Grall wrote: >>> On 16/04/2020 13:19, Jan Beulich wrote: >>>> On 16.04.2020 13:24, Julien Grall wrote: >>>>> From: Julien Grall <jgrall@amazon.com> >>>>> >>>>> At the moment, *copy_to_guest_offset() will allow the hypervisor to copy >>>>> data to guest handle marked const. >>>>> >>>>> Thankfully, no users of the helper will do that. Rather than hoping this >>>>> can be caught during review, harden copy_to_guest_offset() so the build >>>>> will fail if such users are introduced. >>>>> >>>>> There is no easy way to check whether a const is NULL in C99. The >>>>> approach used is to introduce an unused variable that is non-const and >>>>> assign the handle. If the handle were const, this would fail at build >>>>> because without an explicit cast, it is not possible to assign a const >>>>> variable to a non-const variable. >>>>> >>>>> Suggested-by: Jan Beulich <jbeulich@suse.com> >>>>> Signed-off-by: Julien Grall <jgrall@amazon.com> >>>> >>>> Reviewed-by: Jan Beulich <jbeulich@suse.com> >>>> with one further remark: >>>> >>>>> --- a/xen/include/asm-x86/guest_access.h >>>>> +++ b/xen/include/asm-x86/guest_access.h >>>>> @@ -87,6 +87,8 @@ >>>>> #define copy_to_guest_offset(hnd, off, ptr, nr) ({ \ >>>>> const typeof(*(ptr)) *_s = (ptr); \ >>>>> char (*_d)[sizeof(*_s)] = (void *)(hnd).p; \ >>>>> + /* Check if the handle is not const */ \ >>>>> + void *__maybe_unused _t = (hnd).p; \ >>>> >>>> Not being a native speaker, to me "if" doesn't look appropriate >>>> here. I'd use "that" instead, but you may want to confirm this. >>>> Overall then maybe "Check that the handle is not for a const type"? >>> >>> I am happy with the new suggestion. I will fixup while comitting it. >>> >>> >>> I would also need a review from Stefano here also. >> >> Would you, even under the new rules? > > "2. In unusual circumstances, a more general maintainer's Ack can stand > in for or even overrule a specific maintainer's Ack. Unusual > circumstances might include: > > - The more specific maintainer has not responded either to the > original patch, nor to "pings", within a reasonable amount of time. > " > > So it depends on your definition of "reasonable amount of time". > A week or two seems reasonable to me for non-pressing issues. No, this isn't the part I was referring to - there are no unusual circumstances here. Quite a bit further up you'll in particular find "In a case where a maintainer themselves submits a patch, the Signed-off-by meets the approval requirement (#1); so a Review from anyone in the community suffices for requirement #2." Jan
On 23/04/2020 09:16, Jan Beulich wrote: > On 23.04.2020 10:10, Julien Grall wrote: >> Hi, >> >> On 23/04/2020 08:38, Jan Beulich wrote: >>> On 17.04.2020 19:16, Julien Grall wrote: >>>> On 16/04/2020 13:19, Jan Beulich wrote: >>>>> On 16.04.2020 13:24, Julien Grall wrote: >>>>>> From: Julien Grall <jgrall@amazon.com> >>>>>> >>>>>> At the moment, *copy_to_guest_offset() will allow the hypervisor to copy >>>>>> data to guest handle marked const. >>>>>> >>>>>> Thankfully, no users of the helper will do that. Rather than hoping this >>>>>> can be caught during review, harden copy_to_guest_offset() so the build >>>>>> will fail if such users are introduced. >>>>>> >>>>>> There is no easy way to check whether a const is NULL in C99. The >>>>>> approach used is to introduce an unused variable that is non-const and >>>>>> assign the handle. If the handle were const, this would fail at build >>>>>> because without an explicit cast, it is not possible to assign a const >>>>>> variable to a non-const variable. >>>>>> >>>>>> Suggested-by: Jan Beulich <jbeulich@suse.com> >>>>>> Signed-off-by: Julien Grall <jgrall@amazon.com> >>>>> >>>>> Reviewed-by: Jan Beulich <jbeulich@suse.com> >>>>> with one further remark: >>>>> >>>>>> --- a/xen/include/asm-x86/guest_access.h >>>>>> +++ b/xen/include/asm-x86/guest_access.h >>>>>> @@ -87,6 +87,8 @@ >>>>>> #define copy_to_guest_offset(hnd, off, ptr, nr) ({ \ >>>>>> const typeof(*(ptr)) *_s = (ptr); \ >>>>>> char (*_d)[sizeof(*_s)] = (void *)(hnd).p; \ >>>>>> + /* Check if the handle is not const */ \ >>>>>> + void *__maybe_unused _t = (hnd).p; \ >>>>> >>>>> Not being a native speaker, to me "if" doesn't look appropriate >>>>> here. I'd use "that" instead, but you may want to confirm this. >>>>> Overall then maybe "Check that the handle is not for a const type"? >>>> >>>> I am happy with the new suggestion. I will fixup while comitting it. >>>> >>>> >>>> I would also need a review from Stefano here also. >>> >>> Would you, even under the new rules? >> >> "2. In unusual circumstances, a more general maintainer's Ack can stand >> in for or even overrule a specific maintainer's Ack. Unusual >> circumstances might include: >> >> - The more specific maintainer has not responded either to the >> original patch, nor to "pings", within a reasonable amount of time. >> " >> >> So it depends on your definition of "reasonable amount of time". >> A week or two seems reasonable to me for non-pressing issues. > > No, this isn't the part I was referring to - there are no unusual > circumstances here. Quite a bit further up you'll in particular > find > > "In a case where a maintainer themselves submits a patch, the > Signed-off-by meets the approval requirement (#1); so a Review > from anyone in the community suffices for requirement #2."T This is the general rule that we haven't followed on Arm so far. In any case this is followed by: "Before a maintainer checks in their own patch with another community member's R-b but no co-maintainer Ack, it is especially important to give their co-maintainer opportunity to give feedback, perhaps declaring their intention to check it in without their co-maintainers ack a day before doing so." So let's give a couple of days for Stefano to object. Cheers,
On Fri, 17 Apr 2020, Julien Grall wrote: > On 16/04/2020 13:19, Jan Beulich wrote: > > On 16.04.2020 13:24, Julien Grall wrote: > > > From: Julien Grall <jgrall@amazon.com> > > > > > > At the moment, *copy_to_guest_offset() will allow the hypervisor to copy > > > data to guest handle marked const. > > > > > > Thankfully, no users of the helper will do that. Rather than hoping this > > > can be caught during review, harden copy_to_guest_offset() so the build > > > will fail if such users are introduced. > > > > > > There is no easy way to check whether a const is NULL in C99. The > > > approach used is to introduce an unused variable that is non-const and > > > assign the handle. If the handle were const, this would fail at build > > > because without an explicit cast, it is not possible to assign a const > > > variable to a non-const variable. > > > > > > Suggested-by: Jan Beulich <jbeulich@suse.com> > > > Signed-off-by: Julien Grall <jgrall@amazon.com> > > > > Reviewed-by: Jan Beulich <jbeulich@suse.com> > > with one further remark: > > > > > --- a/xen/include/asm-x86/guest_access.h > > > +++ b/xen/include/asm-x86/guest_access.h > > > @@ -87,6 +87,8 @@ > > > #define copy_to_guest_offset(hnd, off, ptr, nr) ({ \ > > > const typeof(*(ptr)) *_s = (ptr); \ > > > char (*_d)[sizeof(*_s)] = (void *)(hnd).p; \ > > > + /* Check if the handle is not const */ \ > > > + void *__maybe_unused _t = (hnd).p; \ > > > > Not being a native speaker, to me "if" doesn't look appropriate > > here. I'd use "that" instead, but you may want to confirm this. > > Overall then maybe "Check that the handle is not for a const type"? > > I am happy with the new suggestion. I will fixup while comitting it. > > I would also need a review from Stefano here also. Acked-by: Stefano Stabellini <sstabellini@kernel.org>
diff --git a/xen/include/asm-arm/guest_access.h b/xen/include/asm-arm/guest_access.h index 8997a1cbfe..97cf3384f1 100644 --- a/xen/include/asm-arm/guest_access.h +++ b/xen/include/asm-arm/guest_access.h @@ -78,6 +78,8 @@ int access_guest_memory_by_ipa(struct domain *d, paddr_t ipa, void *buf, #define copy_to_guest_offset(hnd, off, ptr, nr) ({ \ const typeof(*(ptr)) *_s = (ptr); \ char (*_d)[sizeof(*_s)] = (void *)(hnd).p; \ + /* Check if the handle is not const */ \ + void *__maybe_unused _t = (hnd).p; \ ((void)((hnd).p == (ptr))); \ raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr)); \ }) @@ -127,6 +129,8 @@ int access_guest_memory_by_ipa(struct domain *d, paddr_t ipa, void *buf, #define __copy_to_guest_offset(hnd, off, ptr, nr) ({ \ const typeof(*(ptr)) *_s = (ptr); \ char (*_d)[sizeof(*_s)] = (void *)(hnd).p; \ + /* Check if the handle is not const */ \ + void *__maybe_unused _t = (hnd).p; \ ((void)((hnd).p == (ptr))); \ __raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));\ }) diff --git a/xen/include/asm-x86/guest_access.h b/xen/include/asm-x86/guest_access.h index ca700c959a..6f5107951c 100644 --- a/xen/include/asm-x86/guest_access.h +++ b/xen/include/asm-x86/guest_access.h @@ -87,6 +87,8 @@ #define copy_to_guest_offset(hnd, off, ptr, nr) ({ \ const typeof(*(ptr)) *_s = (ptr); \ char (*_d)[sizeof(*_s)] = (void *)(hnd).p; \ + /* Check if the handle is not const */ \ + void *__maybe_unused _t = (hnd).p; \ ((void)((hnd).p == (ptr))); \ raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr)); \ }) @@ -137,6 +139,8 @@ #define __copy_to_guest_offset(hnd, off, ptr, nr) ({ \ const typeof(*(ptr)) *_s = (ptr); \ char (*_d)[sizeof(*_s)] = (void *)(hnd).p; \ + /* Check if the handle is not const */ \ + void *__maybe_unused _t = (hnd).p; \ ((void)((hnd).p == (ptr))); \ __raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));\ })