| Message ID | 20210512075445.18935-4-joro@8bytes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/sev-es: Fixes for SEV-ES guest support | expand |
From: Joerg > Sent: 12 May 2021 08:55 > > From: Joerg Roedel <jroedel@suse.de> > > The put_user() and get_user() functions do checks on the address which is > passed to them. They check whether the address is actually a user-space > address and whether its fine to access it. They also call might_fault() > to indicate that they could fault and possibly sleep. > > All of these checks are neither wanted nor required in the #VC exception > handler, which can be invoked from almost any context and also for MMIO > instructions from kernel space on kernel memory. All the #VC handler > wants to know is whether a fault happened when the access was tried. > > This is provided by __put_user()/__get_user(), which just do the access > no matter what. That can't be right at all. __put/get_user() are only valid on user addresses and will try to fault in a missing page - so can sleep. At best this is abused the calls. David > Fixes: f980f9c31a92 ("x86/sev-es: Compile early handler code into kernel image") > Cc: stable@vger.kernel.org # v5.10+ > Signed-off-by: Joerg Roedel <jroedel@suse.de> > --- > arch/x86/kernel/sev.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c > index 6530a844eb61..110b39345b40 100644 > --- a/arch/x86/kernel/sev.c > +++ b/arch/x86/kernel/sev.c > @@ -342,22 +342,22 @@ static enum es_result vc_write_mem(struct es_em_ctxt *ctxt, > switch (size) { > case 1: > memcpy(&d1, buf, 1); > - if (put_user(d1, target)) > + if (__put_user(d1, target)) > goto fault; > break; > case 2: > memcpy(&d2, buf, 2); > - if (put_user(d2, target)) > + if (__put_user(d2, target)) > goto fault; > break; > case 4: > memcpy(&d4, buf, 4); > - if (put_user(d4, target)) > + if (__put_user(d4, target)) > goto fault; > break; > case 8: > memcpy(&d8, buf, 8); > - if (put_user(d8, target)) > + if (__put_user(d8, target)) > goto fault; > break; > default: > @@ -396,22 +396,22 @@ static enum es_result vc_read_mem(struct es_em_ctxt *ctxt, > > switch (size) { > case 1: > - if (get_user(d1, s)) > + if (__get_user(d1, s)) > goto fault; > memcpy(buf, &d1, 1); > break; > case 2: > - if (get_user(d2, s)) > + if (__get_user(d2, s)) > goto fault; > memcpy(buf, &d2, 2); > break; > case 4: > - if (get_user(d4, s)) > + if (__get_user(d4, s)) > goto fault; > memcpy(buf, &d4, 4); > break; > case 8: > - if (get_user(d8, s)) > + if (__get_user(d8, s)) > goto fault; > memcpy(buf, &d8, 8); > break; > -- > 2.31.1 - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 12.05.21 10:04, David Laight wrote: > From: Joerg >> Sent: 12 May 2021 08:55 >> >> From: Joerg Roedel <jroedel@suse.de> >> >> The put_user() and get_user() functions do checks on the address which is >> passed to them. They check whether the address is actually a user-space >> address and whether its fine to access it. They also call might_fault() >> to indicate that they could fault and possibly sleep. >> >> All of these checks are neither wanted nor required in the #VC exception >> handler, which can be invoked from almost any context and also for MMIO >> instructions from kernel space on kernel memory. All the #VC handler >> wants to know is whether a fault happened when the access was tried. >> >> This is provided by __put_user()/__get_user(), which just do the access >> no matter what. > > That can't be right at all. > __put/get_user() are only valid on user addresses and will try to > fault in a missing page - so can sleep. > > At best this is abused the calls. You want something like xen_safe_[read|write]_ulong(). Juergen
On Wed, May 12, 2021 at 08:04:33AM +0000, David Laight wrote: > That can't be right at all. > __put/get_user() are only valid on user addresses and will try to > fault in a missing page - so can sleep. Yes, in general these functions can sleep, but not in this context. They are called in atomic context and the page-fault handler will notice that and goes down the __bad_area_nosemaphore() path and only do the fixup. I also thought about adding page_fault_disable()/page_fault_enable() calls, but being in atomic context is enough according to the faulthandler_disabled() implementation. This is exactly what is needed here. All I want to know is whether a fault happened or not, the page-fault handler must not try to fix the fault in any way. If a fault happens it is later fixed up in vc_forward_exception(). > At best this is abused the calls. Yes, but that is only due to the naming of these functions. In this case they do exactly what is needed. Regards, Joerg
On Wed, May 12, 2021 at 10:16:12AM +0200, Juergen Gross wrote:
> You want something like xen_safe_[read|write]_ulong().
From a first glance I can't see it, what is the difference between the
xen_safe_*_ulong() functions and __get_user()/__put_user()? The only
difference I can see is that __get/__put_user() support different access
sizes, but neither of those disables page-faults by itself, for example.
Couldn't these xen-specific functions not also be replaces by
__get_user()/__put_user()?
Regards,
Joerg
On 12.05.21 10:50, 'Joerg Roedel' wrote: > On Wed, May 12, 2021 at 10:16:12AM +0200, Juergen Gross wrote: >> You want something like xen_safe_[read|write]_ulong(). > > From a first glance I can't see it, what is the difference between the > xen_safe_*_ulong() functions and __get_user()/__put_user()? The only > difference I can see is that __get/__put_user() support different access > sizes, but neither of those disables page-faults by itself, for example. > > Couldn't these xen-specific functions not also be replaces by > __get_user()/__put_user()? No, those were used before, but commit 9da3f2b7405440 broke Xen's use case. That is why I did commit 1457d8cf7664f. Juergen
From: Juergen Gross > Sent: 12 May 2021 09:58 > > On 12.05.21 10:50, 'Joerg Roedel' wrote: > > On Wed, May 12, 2021 at 10:16:12AM +0200, Juergen Gross wrote: > >> You want something like xen_safe_[read|write]_ulong(). > > > > From a first glance I can't see it, what is the difference between the > > xen_safe_*_ulong() functions and __get_user()/__put_user()? The only > > difference I can see is that __get/__put_user() support different access > > sizes, but neither of those disables page-faults by itself, for example. > > > > Couldn't these xen-specific functions not also be replaces by > > __get_user()/__put_user()? > > No, those were used before, but commit 9da3f2b7405440 broke Xen's use > case. That is why I did commit 1457d8cf7664f. I've just looked at 9da3f2b7405440. It doesn't look right to me - wrong return value for a lot of cases. OTOH it isn't in my newest tree at all. If bogus_uaccess() is now elsewhere I can't see how (without changes) it would allow through these faults. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Wed, May 12, 2021 at 10:58:20AM +0200, Juergen Gross wrote: > No, those were used before, but commit 9da3f2b7405440 broke Xen's use > case. That is why I did commit 1457d8cf7664f. I see, thanks for the heads-up. So here this is not a big issue, because when an access to kernel space faults under SEV-ES, its a kernel bug anyway. The issue is that it is not reported correctly. I think I need to re-work the helper and use probe_kernel_read/write() when the address is in kernel space. This distinction is already made when fetching instruction bytes in the #VC handler, but I thought I could get around it for data accesses. Having the distinction between user and kernel memory accesses explicitly in the code seems to be the most robust solution. Regards, Joerg
On 5/12/21 12:54 AM, Joerg Roedel wrote: > The put_user() and get_user() functions do checks on the address which is > passed to them. They check whether the address is actually a user-space > address and whether its fine to access it. They also call might_fault() > to indicate that they could fault and possibly sleep. > > All of these checks are neither wanted nor required in the #VC exception > handler, which can be invoked from almost any context and also for MMIO > instructions from kernel space on kernel memory. All the #VC handler > wants to know is whether a fault happened when the access was tried. > > This is provided by __put_user()/__get_user(), which just do the access > no matter what. The changelog _helps_, but using a "user" function to handle kernel MMIO for its error handling properties seems like it's begging for a comment. __put_user() also seems to have fun stuff like __chk_user_ptr(). It all seems sketchy to me.
On 5/12/21 1:37 AM, 'Joerg Roedel' wrote: > I also thought about adding page_fault_disable()/page_fault_enable() > calls, but being in atomic context is enough according to the > faulthandler_disabled() implementation. That would be nice to add to a comment: page_fault_disable()/page_fault_enable() are not needed because of the context this must be called in.
On Wed, May 12, 2021 at 08:57:53AM -0700, Dave Hansen wrote: > The changelog _helps_, but using a "user" function to handle kernel MMIO > for its error handling properties seems like it's begging for a comment. > > __put_user() also seems to have fun stuff like __chk_user_ptr(). It all > seems sketchy to me. Yeah, as Juergen already pointed out, there are certain problems with that too. But I don't want to write my own accessors, so I will introduce a separate user and kernel path to these functions. Regards, Joerg
On Wed, May 12, 2021 at 11:32:35AM +0200, Joerg Roedel wrote: > On Wed, May 12, 2021 at 10:58:20AM +0200, Juergen Gross wrote: > > No, those were used before, but commit 9da3f2b7405440 broke Xen's use > > case. That is why I did commit 1457d8cf7664f. > > [...] > > Having the distinction between user and kernel memory accesses > explicitly in the code seems to be the most robust solution. On the other hand, as I found out today, 9da3f2b7405440 had a short life-time and got reverted upstream. So using __get_user()/__put_user() should be fine in this code path. It just deserves a comment explaining its use here and why pagefault_enable()/disable() is not needed. Even the get_kernel* helpers use __get_user_size() internally. Regards, Joerg
diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c index 6530a844eb61..110b39345b40 100644 --- a/arch/x86/kernel/sev.c +++ b/arch/x86/kernel/sev.c @@ -342,22 +342,22 @@ static enum es_result vc_write_mem(struct es_em_ctxt *ctxt, switch (size) { case 1: memcpy(&d1, buf, 1); - if (put_user(d1, target)) + if (__put_user(d1, target)) goto fault; break; case 2: memcpy(&d2, buf, 2); - if (put_user(d2, target)) + if (__put_user(d2, target)) goto fault; break; case 4: memcpy(&d4, buf, 4); - if (put_user(d4, target)) + if (__put_user(d4, target)) goto fault; break; case 8: memcpy(&d8, buf, 8); - if (put_user(d8, target)) + if (__put_user(d8, target)) goto fault; break; default: @@ -396,22 +396,22 @@ static enum es_result vc_read_mem(struct es_em_ctxt *ctxt, switch (size) { case 1: - if (get_user(d1, s)) + if (__get_user(d1, s)) goto fault; memcpy(buf, &d1, 1); break; case 2: - if (get_user(d2, s)) + if (__get_user(d2, s)) goto fault; memcpy(buf, &d2, 2); break; case 4: - if (get_user(d4, s)) + if (__get_user(d4, s)) goto fault; memcpy(buf, &d4, 4); break; case 8: - if (get_user(d8, s)) + if (__get_user(d8, s)) goto fault; memcpy(buf, &d8, 8); break;