Re: [PATCH v5] sev: add sev-inject-launch-secret

2020-10-20 Thread Tobin Feldman-Fitzthum 

On 2020-10-20 11:56, Paolo Bonzini wrote:

On 20/10/20 15:54, Eduardo Habkost wrote:

On Tue, Oct 20, 2020 at 11:03:51AM +0200, Paolo Bonzini wrote:

On 15/10/20 16:37, to...@linux.ibm.com wrote:
-static void *gpa2hva(MemoryRegion **p_mr, hwaddr addr, Error 
**errp)
+void *gpa2hva(MemoryRegion **p_mr, hwaddr addr, uint64_t size, 
Error **errp)

 {
 MemoryRegionSection mrs = 
memory_region_find(get_system_memory(),

- addr, 1);
+ addr, size);


You need to check size against mrs.size and fail if mrs.size is 
smaller.

 Otherwise, the ioctl can access memory out of range.


Good catch!  I'm dequeuing it.

Is there a reason memory_region_find() doesn't ensure that by
default?


IIRC memory_region_find() was used to do DMA in the very first versions
of "virtio-blk dataplane" so you would call it multiple times in a 
loop.

 So it's like that because it maps the way address_space_map() works.


The call at virtio_balloon_handle_output() looks suspicious,
though, because it looks for a BALLOON_PAGE_SIZE range, but
there's no check for the returned section size.


I think it's not a bug because ultimately it's checked in
ram_block_discard_range, but it's not pretty.

Paolo


Ok, it seems like the best solution is, as Paolo suggested, to add a
simple check at the end of gpa2hva that makes sure mr.size is equal
to size. gpa2hva is used one other place where the size is hard-coded
as 1, so adding the check isn't going to break anything.

Would you like me to resubmit with this tweak?

-Tobin

Re: [PATCH v5] sev: add sev-inject-launch-secret

2020-10-20 Thread Paolo Bonzini 
On 20/10/20 15:54, Eduardo Habkost wrote:
> On Tue, Oct 20, 2020 at 11:03:51AM +0200, Paolo Bonzini wrote:
>> On 15/10/20 16:37, to...@linux.ibm.com wrote:
>>> -static void *gpa2hva(MemoryRegion **p_mr, hwaddr addr, Error **errp)
>>> +void *gpa2hva(MemoryRegion **p_mr, hwaddr addr, uint64_t size, Error 
>>> **errp)
>>>  {
>>>  MemoryRegionSection mrs = memory_region_find(get_system_memory(),
>>> - addr, 1);
>>> + addr, size);
>>
>> You need to check size against mrs.size and fail if mrs.size is smaller.
>>  Otherwise, the ioctl can access memory out of range.
> 
> Good catch!  I'm dequeuing it.
> 
> Is there a reason memory_region_find() doesn't ensure that by
> default?

IIRC memory_region_find() was used to do DMA in the very first versions
of "virtio-blk dataplane" so you would call it multiple times in a loop.
 So it's like that because it maps the way address_space_map() works.

> The call at virtio_balloon_handle_output() looks suspicious,
> though, because it looks for a BALLOON_PAGE_SIZE range, but
> there's no check for the returned section size.

I think it's not a bug because ultimately it's checked in
ram_block_discard_range, but it's not pretty.

Paolo

Re: [PATCH v5] sev: add sev-inject-launch-secret

2020-10-20 Thread Eduardo Habkost 
On Tue, Oct 20, 2020 at 11:03:51AM +0200, Paolo Bonzini wrote:
> On 15/10/20 16:37, to...@linux.ibm.com wrote:
> > -static void *gpa2hva(MemoryRegion **p_mr, hwaddr addr, Error **errp)
> > +void *gpa2hva(MemoryRegion **p_mr, hwaddr addr, uint64_t size, Error 
> > **errp)
> >  {
> >  MemoryRegionSection mrs = memory_region_find(get_system_memory(),
> > - addr, 1);
> > + addr, size);
> 
> You need to check size against mrs.size and fail if mrs.size is smaller.
>  Otherwise, the ioctl can access memory out of range.

Good catch!  I'm dequeuing it.

Is there a reason memory_region_find() doesn't ensure that by
default?

It looks like there's only one memory_region_find() call in the
code that doesn't expect the returned section to contain the
entire range (at platform_bus_map_mmio()).  All the remaining
memory_region_find() calls either have size==1 (so it doesn't
matter) or have an extra check for MemoryRegionSection.size.

The call at virtio_balloon_handle_output() looks suspicious,
though, because it looks for a BALLOON_PAGE_SIZE range, but
there's no check for the returned section size.

-- 
Eduardo

Re: [PATCH v5] sev: add sev-inject-launch-secret

2020-10-20 Thread Paolo Bonzini 
On 15/10/20 16:37, to...@linux.ibm.com wrote:
> -static void *gpa2hva(MemoryRegion **p_mr, hwaddr addr, Error **errp)
> +void *gpa2hva(MemoryRegion **p_mr, hwaddr addr, uint64_t size, Error **errp)
>  {
>  MemoryRegionSection mrs = memory_region_find(get_system_memory(),
> - addr, 1);
> + addr, size);

You need to check size against mrs.size and fail if mrs.size is smaller.
 Otherwise, the ioctl can access memory out of range.

Sorry Eduardo for the late review.

Paolo

Re: [PATCH v5] sev: add sev-inject-launch-secret

2020-10-19 Thread Tobin Feldman-Fitzthum 

On 2020-10-19 12:47, Eduardo Habkost wrote:

On Mon, Oct 19, 2020 at 12:46:08PM -0400, Eduardo Habkost wrote:

On Thu, Oct 15, 2020 at 10:37:13AM -0400, to...@linux.ibm.com wrote:
[...]
> diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c
> index 88e3f39a1e..2d2ee54cc6 100644
> --- a/target/i386/sev-stub.c
> +++ b/target/i386/sev-stub.c
> @@ -49,3 +49,8 @@ SevCapability *sev_get_capabilities(Error **errp)
>  error_setg(errp, "SEV is not available in this QEMU");
>  return NULL;
>  }
> +int sev_inject_launch_secret(const char *hdr, const char *secret,
> + uint64_t gpa)
> +{
> +return 1;
> +}

This doesn't match the actual function prototype.  I had to apply the 
following

fixup:

---
diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c
index 2d2ee54cc6..62a2587e7b 100644
--- a/target/i386/sev-stub.c
+++ b/target/i386/sev-stub.c
@@ -49,8 +49,10 @@ SevCapability *sev_get_capabilities(Error **errp)
 error_setg(errp, "SEV is not available in this QEMU");
 return NULL;
 }
+
 int sev_inject_launch_secret(const char *hdr, const char *secret,
- uint64_t gpa)
+ uint64_t gpa, Error *errp)


Oops. Fixing up the fixup:


Thanks Eduardo.

-Tobin


---
diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c
index 62a2587e7b..e4e60d9a7d 100644
--- a/target/i386/sev-stub.c
+++ b/target/i386/sev-stub.c
@@ -51,7 +51,7 @@ SevCapability *sev_get_capabilities(Error **errp)
 }

 int sev_inject_launch_secret(const char *hdr, const char *secret,
- uint64_t gpa, Error *errp)
+ uint64_t gpa, Error **errp)
 {
 error_setg(errp, "SEV is not available in this QEMU");
 return 1;

Re: [PATCH v5] sev: add sev-inject-launch-secret

2020-10-19 Thread Eduardo Habkost 
On Thu, Oct 15, 2020 at 10:37:13AM -0400, to...@linux.ibm.com wrote:
[...]
> diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c
> index 88e3f39a1e..2d2ee54cc6 100644
> --- a/target/i386/sev-stub.c
> +++ b/target/i386/sev-stub.c
> @@ -49,3 +49,8 @@ SevCapability *sev_get_capabilities(Error **errp)
>  error_setg(errp, "SEV is not available in this QEMU");
>  return NULL;
>  }
> +int sev_inject_launch_secret(const char *hdr, const char *secret,
> + uint64_t gpa)
> +{
> +return 1;
> +}

This doesn't match the actual function prototype.  I had to apply the following
fixup:

---
diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c
index 2d2ee54cc6..62a2587e7b 100644
--- a/target/i386/sev-stub.c
+++ b/target/i386/sev-stub.c
@@ -49,8 +49,10 @@ SevCapability *sev_get_capabilities(Error **errp)
 error_setg(errp, "SEV is not available in this QEMU");
 return NULL;
 }
+
 int sev_inject_launch_secret(const char *hdr, const char *secret,
- uint64_t gpa)
+ uint64_t gpa, Error *errp)
 {
+error_setg(errp, "SEV is not available in this QEMU");
 return 1;
 }

-- 
Eduardo

Re: [PATCH v5] sev: add sev-inject-launch-secret

2020-10-19 Thread Eduardo Habkost 
On Mon, Oct 19, 2020 at 12:46:08PM -0400, Eduardo Habkost wrote:
> On Thu, Oct 15, 2020 at 10:37:13AM -0400, to...@linux.ibm.com wrote:
> [...]
> > diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c
> > index 88e3f39a1e..2d2ee54cc6 100644
> > --- a/target/i386/sev-stub.c
> > +++ b/target/i386/sev-stub.c
> > @@ -49,3 +49,8 @@ SevCapability *sev_get_capabilities(Error **errp)
> >  error_setg(errp, "SEV is not available in this QEMU");
> >  return NULL;
> >  }
> > +int sev_inject_launch_secret(const char *hdr, const char *secret,
> > + uint64_t gpa)
> > +{
> > +return 1;
> > +}
> 
> This doesn't match the actual function prototype.  I had to apply the 
> following
> fixup:
> 
> ---
> diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c
> index 2d2ee54cc6..62a2587e7b 100644
> --- a/target/i386/sev-stub.c
> +++ b/target/i386/sev-stub.c
> @@ -49,8 +49,10 @@ SevCapability *sev_get_capabilities(Error **errp)
>  error_setg(errp, "SEV is not available in this QEMU");
>  return NULL;
>  }
> +
>  int sev_inject_launch_secret(const char *hdr, const char *secret,
> - uint64_t gpa)
> + uint64_t gpa, Error *errp)

Oops. Fixing up the fixup:

---
diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c
index 62a2587e7b..e4e60d9a7d 100644
--- a/target/i386/sev-stub.c
+++ b/target/i386/sev-stub.c
@@ -51,7 +51,7 @@ SevCapability *sev_get_capabilities(Error **errp)
 }
 
 int sev_inject_launch_secret(const char *hdr, const char *secret,
- uint64_t gpa, Error *errp)
+ uint64_t gpa, Error **errp)
 {
 error_setg(errp, "SEV is not available in this QEMU");
 return 1;

-- 
Eduardo

Re: [PATCH v5] sev: add sev-inject-launch-secret

2020-10-19 Thread Eduardo Habkost 
On Thu, Oct 15, 2020 at 10:37:13AM -0400, to...@linux.ibm.com wrote:
> From: Tobin Feldman-Fitzthum 
> 
> AMD SEV allows a guest owner to inject a secret blob
> into the memory of a virtual machine. The secret is
> encrypted with the SEV Transport Encryption Key and
> integrity is guaranteed with the Transport Integrity
> Key. Although QEMU facilitates the injection of the
> launch secret, it cannot access the secret.
> 
> Signed-off-by: Tobin Feldman-Fitzthum 
> Reviewed-by: Daniel P. Berrangé 

Queued, thanks!

-- 
Eduardo

Re: [PATCH v5] sev: add sev-inject-launch-secret

2020-10-15 Thread Brijesh Singh 


On 10/15/20 9:37 AM, to...@linux.ibm.com wrote:
> From: Tobin Feldman-Fitzthum 
>
> AMD SEV allows a guest owner to inject a secret blob
> into the memory of a virtual machine. The secret is
> encrypted with the SEV Transport Encryption Key and
> integrity is guaranteed with the Transport Integrity
> Key. Although QEMU facilitates the injection of the
> launch secret, it cannot access the secret.
>
> Signed-off-by: Tobin Feldman-Fitzthum 
> Reviewed-by: Daniel P. Berrangé 


Reviewed-by: Brijesh Singh 

thanks


> ---
>  include/monitor/monitor.h |  3 ++
>  include/sysemu/sev.h  |  2 ++
>  monitor/misc.c|  8 ++---
>  qapi/misc-target.json | 18 +++
>  target/i386/monitor.c |  7 +
>  target/i386/sev-stub.c|  5 +++
>  target/i386/sev.c | 65 +++
>  target/i386/trace-events  |  1 +
>  8 files changed, 105 insertions(+), 4 deletions(-)
>
> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> index 348bfad3d5..af3887bb71 100644
> --- a/include/monitor/monitor.h
> +++ b/include/monitor/monitor.h
> @@ -4,6 +4,7 @@
>  #include "block/block.h"
>  #include "qapi/qapi-types-misc.h"
>  #include "qemu/readline.h"
> +#include "include/exec/hwaddr.h"
>  
>  typedef struct MonitorHMP MonitorHMP;
>  typedef struct MonitorOptions MonitorOptions;
> @@ -37,6 +38,8 @@ void monitor_flush(Monitor *mon);
>  int monitor_set_cpu(Monitor *mon, int cpu_index);
>  int monitor_get_cpu_index(Monitor *mon);
>  
> +void *gpa2hva(MemoryRegion **p_mr, hwaddr addr, uint64_t size, Error **errp);
> +
>  void monitor_read_command(MonitorHMP *mon, int show_prompt);
>  int monitor_read_password(MonitorHMP *mon, ReadLineFunc *readline_func,
>void *opaque);
> diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h
> index 98c1ec8d38..7ab6e3e31d 100644
> --- a/include/sysemu/sev.h
> +++ b/include/sysemu/sev.h
> @@ -18,4 +18,6 @@
>  
>  void *sev_guest_init(const char *id);
>  int sev_encrypt_data(void *handle, uint8_t *ptr, uint64_t len);
> +int sev_inject_launch_secret(const char *hdr, const char *secret,
> + uint64_t gpa, Error **errp);
>  #endif
> diff --git a/monitor/misc.c b/monitor/misc.c
> index 4a859fb24a..f1ade245d5 100644
> --- a/monitor/misc.c
> +++ b/monitor/misc.c
> @@ -667,10 +667,10 @@ static void hmp_physical_memory_dump(Monitor *mon, 
> const QDict *qdict)
>  memory_dump(mon, count, format, size, addr, 1);
>  }
>  
> -static void *gpa2hva(MemoryRegion **p_mr, hwaddr addr, Error **errp)
> +void *gpa2hva(MemoryRegion **p_mr, hwaddr addr, uint64_t size, Error **errp)
>  {
>  MemoryRegionSection mrs = memory_region_find(get_system_memory(),
> - addr, 1);
> + addr, size);
>  
>  if (!mrs.mr) {
>  error_setg(errp, "No memory is mapped at address 0x%" HWADDR_PRIx, 
> addr);
> @@ -694,7 +694,7 @@ static void hmp_gpa2hva(Monitor *mon, const QDict *qdict)
>  MemoryRegion *mr = NULL;
>  void *ptr;
>  
> -ptr = gpa2hva(, addr, _err);
> +ptr = gpa2hva(, addr, 1, _err);
>  if (local_err) {
>  error_report_err(local_err);
>  return;
> @@ -770,7 +770,7 @@ static void hmp_gpa2hpa(Monitor *mon, const QDict *qdict)
>  void *ptr;
>  uint64_t physaddr;
>  
> -ptr = gpa2hva(, addr, _err);
> +ptr = gpa2hva(, addr, 1, _err);
>  if (local_err) {
>  error_report_err(local_err);
>  return;
> diff --git a/qapi/misc-target.json b/qapi/misc-target.json
> index 1e561fa97b..4486a543ae 100644
> --- a/qapi/misc-target.json
> +++ b/qapi/misc-target.json
> @@ -201,6 +201,24 @@
>  { 'command': 'query-sev-capabilities', 'returns': 'SevCapability',
>'if': 'defined(TARGET_I386)' }
>  
> +##
> +# @sev-inject-launch-secret:
> +#
> +# This command injects a secret blob into memory of SEV guest.
> +#
> +# @packet-header: the launch secret packet header encoded in base64
> +#
> +# @secret: the launch secret data to be injected encoded in base64
> +#
> +# @gpa: the guest physical address where secret will be injected.
> +#
> +# Since: 5.2
> +#
> +##
> +{ 'command': 'sev-inject-launch-secret',
> +  'data': { 'packet-header': 'str', 'secret': 'str', 'gpa': 'uint64' },
> +  'if': 'defined(TARGET_I386)' }
> +
>  ##
>  # @dump-skeys:
>  #
> diff --git a/target/i386/monitor.c b/target/i386/monitor.c
> index 7abae3c8df..f9d4951465 100644
> --- a/target/i386/monitor.c
> +++ b/target/i386/monitor.c
> @@ -728,3 +728,10 @@ SevCapability *qmp_query_sev_capabilities(Error **errp)
>  {
>  return sev_get_capabilities(errp);
>  }
> +
> +void qmp_sev_inject_launch_secret(const char *packet_hdr,
> +  const char *secret, uint64_t gpa,
> +  Error **errp)
> +{
> +sev_inject_launch_secret(packet_hdr, secret, gpa, errp);
> +}
> diff --git a/target/i386/sev-stub.c

[PATCH v5] sev: add sev-inject-launch-secret

2020-10-15 Thread tobin 
From: Tobin Feldman-Fitzthum 

AMD SEV allows a guest owner to inject a secret blob
into the memory of a virtual machine. The secret is
encrypted with the SEV Transport Encryption Key and
integrity is guaranteed with the Transport Integrity
Key. Although QEMU facilitates the injection of the
launch secret, it cannot access the secret.

Signed-off-by: Tobin Feldman-Fitzthum 
Reviewed-by: Daniel P. Berrangé 
---
 include/monitor/monitor.h |  3 ++
 include/sysemu/sev.h  |  2 ++
 monitor/misc.c|  8 ++---
 qapi/misc-target.json | 18 +++
 target/i386/monitor.c |  7 +
 target/i386/sev-stub.c|  5 +++
 target/i386/sev.c | 65 +++
 target/i386/trace-events  |  1 +
 8 files changed, 105 insertions(+), 4 deletions(-)

diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 348bfad3d5..af3887bb71 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -4,6 +4,7 @@
 #include "block/block.h"
 #include "qapi/qapi-types-misc.h"
 #include "qemu/readline.h"
+#include "include/exec/hwaddr.h"
 
 typedef struct MonitorHMP MonitorHMP;
 typedef struct MonitorOptions MonitorOptions;
@@ -37,6 +38,8 @@ void monitor_flush(Monitor *mon);
 int monitor_set_cpu(Monitor *mon, int cpu_index);
 int monitor_get_cpu_index(Monitor *mon);
 
+void *gpa2hva(MemoryRegion **p_mr, hwaddr addr, uint64_t size, Error **errp);
+
 void monitor_read_command(MonitorHMP *mon, int show_prompt);
 int monitor_read_password(MonitorHMP *mon, ReadLineFunc *readline_func,
   void *opaque);
diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h
index 98c1ec8d38..7ab6e3e31d 100644
--- a/include/sysemu/sev.h
+++ b/include/sysemu/sev.h
@@ -18,4 +18,6 @@
 
 void *sev_guest_init(const char *id);
 int sev_encrypt_data(void *handle, uint8_t *ptr, uint64_t len);
+int sev_inject_launch_secret(const char *hdr, const char *secret,
+ uint64_t gpa, Error **errp);
 #endif
diff --git a/monitor/misc.c b/monitor/misc.c
index 4a859fb24a..f1ade245d5 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -667,10 +667,10 @@ static void hmp_physical_memory_dump(Monitor *mon, const 
QDict *qdict)
 memory_dump(mon, count, format, size, addr, 1);
 }
 
-static void *gpa2hva(MemoryRegion **p_mr, hwaddr addr, Error **errp)
+void *gpa2hva(MemoryRegion **p_mr, hwaddr addr, uint64_t size, Error **errp)
 {
 MemoryRegionSection mrs = memory_region_find(get_system_memory(),
- addr, 1);
+ addr, size);
 
 if (!mrs.mr) {
 error_setg(errp, "No memory is mapped at address 0x%" HWADDR_PRIx, 
addr);
@@ -694,7 +694,7 @@ static void hmp_gpa2hva(Monitor *mon, const QDict *qdict)
 MemoryRegion *mr = NULL;
 void *ptr;
 
-ptr = gpa2hva(, addr, _err);
+ptr = gpa2hva(, addr, 1, _err);
 if (local_err) {
 error_report_err(local_err);
 return;
@@ -770,7 +770,7 @@ static void hmp_gpa2hpa(Monitor *mon, const QDict *qdict)
 void *ptr;
 uint64_t physaddr;
 
-ptr = gpa2hva(, addr, _err);
+ptr = gpa2hva(, addr, 1, _err);
 if (local_err) {
 error_report_err(local_err);
 return;
diff --git a/qapi/misc-target.json b/qapi/misc-target.json
index 1e561fa97b..4486a543ae 100644
--- a/qapi/misc-target.json
+++ b/qapi/misc-target.json
@@ -201,6 +201,24 @@
 { 'command': 'query-sev-capabilities', 'returns': 'SevCapability',
   'if': 'defined(TARGET_I386)' }
 
+##
+# @sev-inject-launch-secret:
+#
+# This command injects a secret blob into memory of SEV guest.
+#
+# @packet-header: the launch secret packet header encoded in base64
+#
+# @secret: the launch secret data to be injected encoded in base64
+#
+# @gpa: the guest physical address where secret will be injected.
+#
+# Since: 5.2
+#
+##
+{ 'command': 'sev-inject-launch-secret',
+  'data': { 'packet-header': 'str', 'secret': 'str', 'gpa': 'uint64' },
+  'if': 'defined(TARGET_I386)' }
+
 ##
 # @dump-skeys:
 #
diff --git a/target/i386/monitor.c b/target/i386/monitor.c
index 7abae3c8df..f9d4951465 100644
--- a/target/i386/monitor.c
+++ b/target/i386/monitor.c
@@ -728,3 +728,10 @@ SevCapability *qmp_query_sev_capabilities(Error **errp)
 {
 return sev_get_capabilities(errp);
 }
+
+void qmp_sev_inject_launch_secret(const char *packet_hdr,
+  const char *secret, uint64_t gpa,
+  Error **errp)
+{
+sev_inject_launch_secret(packet_hdr, secret, gpa, errp);
+}
diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c
index 88e3f39a1e..2d2ee54cc6 100644
--- a/target/i386/sev-stub.c
+++ b/target/i386/sev-stub.c
@@ -49,3 +49,8 @@ SevCapability *sev_get_capabilities(Error **errp)
 error_setg(errp, "SEV is not available in this QEMU");
 return NULL;
 }
+int sev_inject_launch_secret(const char *hdr, const char *secret,
+ uint64_t gpa)
+{