| Message ID | 20210512075445.18935-5-joro@8bytes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/sev-es: Fixes for SEV-ES guest support | expand |
On Wed, May 12, 2021, Joerg Roedel wrote: > From: Joerg Roedel <jroedel@suse.de> > > This reverts commit 7024f60d655272bd2ca1d3a4c9e0a63319b1eea1. > > The commit reverted here introduces a short-cut into the #VC handlers > memory access code which only works reliably in task context. But the > kernels #VC handler can be invoked from any context, making the > access_ok() call trigger a warning with CONFIG_DEBUG_ATOMIC_SLEEP > enabled. > > Also the memcpy() used in the reverted patch is wrong, as it has no > page-fault handling. Access to kernel memory can also fault due to > kernel bugs, and those should not be reported as faults from the #VC > handler but as bugs of their real call-site, which is correctly later > done from vc_forward_exception(). The changelog should call out that a previous patch fixed the original bug by switching to unchecked versions of get/put. Without that, this reads like we're reverting to even worse behavior. Alternatively, and probably even better, fold this revert into the switch to the unchecked version (sounds like those will use kernel-specific flavors?). > Fixes: 7024f60d6552 ("x86/sev-es: Handle string port IO to kernel memory properly") > Cc: stable@vger.kernel.org # v5.11 > Signed-off-by: Joerg Roedel <jroedel@suse.de> > --- > arch/x86/kernel/sev.c | 12 ------------ > 1 file changed, 12 deletions(-) > > diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c > index 110b39345b40..f4f319004713 100644 > --- a/arch/x86/kernel/sev.c > +++ b/arch/x86/kernel/sev.c > @@ -333,12 +333,6 @@ static enum es_result vc_write_mem(struct es_em_ctxt *ctxt, > u16 d2; > u8 d1; > > - /* If instruction ran in kernel mode and the I/O buffer is in kernel space */ > - if (!user_mode(ctxt->regs) && !access_ok(target, size)) { > - memcpy(dst, buf, size); > - return ES_OK; > - } > - > switch (size) { > case 1: > memcpy(&d1, buf, 1); > @@ -388,12 +382,6 @@ static enum es_result vc_read_mem(struct es_em_ctxt *ctxt, > u16 d2; > u8 d1; > > - /* If instruction ran in kernel mode and the I/O buffer is in kernel space */ > - if (!user_mode(ctxt->regs) && !access_ok(s, size)) { > - memcpy(buf, src, size); > - return ES_OK; > - } > - > switch (size) { > case 1: > if (__get_user(d1, s)) > -- > 2.31.1 >
On Wed, May 12, 2021 at 05:38:29PM +0000, Sean Christopherson wrote: > Alternatively, and probably even better, fold this revert into the switch to > the unchecked version (sounds like those will use kernel-specific flavors?). I folded this revert into the previous commit. But I kept the __get_user()/__put_user() calls and just added a comment explaining why they are used and why it is safe to use them. After all, even the get_kernel*() functions call __get_user_size() under the hood. Regards, Joerg
diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c index 110b39345b40..f4f319004713 100644 --- a/arch/x86/kernel/sev.c +++ b/arch/x86/kernel/sev.c @@ -333,12 +333,6 @@ static enum es_result vc_write_mem(struct es_em_ctxt *ctxt, u16 d2; u8 d1; - /* If instruction ran in kernel mode and the I/O buffer is in kernel space */ - if (!user_mode(ctxt->regs) && !access_ok(target, size)) { - memcpy(dst, buf, size); - return ES_OK; - } - switch (size) { case 1: memcpy(&d1, buf, 1); @@ -388,12 +382,6 @@ static enum es_result vc_read_mem(struct es_em_ctxt *ctxt, u16 d2; u8 d1; - /* If instruction ran in kernel mode and the I/O buffer is in kernel space */ - if (!user_mode(ctxt->regs) && !access_ok(s, size)) { - memcpy(buf, src, size); - return ES_OK; - } - switch (size) { case 1: if (__get_user(d1, s))