[PATCH] hypervisor, tools: Added magic bytes for .cell files

[Ralf Ramsauer]

Hi,

I wrote a small patch that inserts two different magics in
jailhouse_cell_desc and jailhouse_system.
Could someone please test this patch on real hardware? I was only able
to check this patch using stubs.

Besides that: Is it correct, that jailhouse does not do any boundary
checks for .cell files at all?
The ioctl calls of tools/jailhouse.c does not propagate the size of the
memory location where the .cell file is stored.
Additionally, the kernel driver just casts the memory to
jailhouse_cell_desc resp. jailhouse_system - also without any boundary
checks.

Ralf
---
Inserted magic byte in struct jailhouse_cell_desc and jailhouse_system.
Jailhouse userland tool will refuse loading a system config as cell
config et vice versa.

Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>
---
configs/apic-demo.c | 1 +
configs/bananapi-gic-demo.c | 1 +
configs/bananapi-uart-demo.c | 1 +
configs/bananapi.c | 1 +
configs/e1000-demo.c | 1 +
configs/f2a88xm-hd3.c | 1 +
configs/h87i.c | 1 +
configs/imb-a180.c | 1 +
configs/ioapic-demo.c | 1 +
configs/ivshmem-demo.c | 1 +
configs/jetson-tk1-demo.c | 1 +
configs/jetson-tk1.c | 1 +
configs/linux-x86-demo.c | 1 +
configs/pci-demo.c | 1 +
configs/qemu-vm.c | 1 +
configs/smp-demo.c | 1 +
configs/tiny-demo.c | 1 +
configs/vexpress-gic-demo.c | 1 +
configs/vexpress-linux-demo.c | 1 +
configs/vexpress-uart-demo.c | 1 +
configs/vexpress.c | 1 +
hypervisor/include/jailhouse/cell-config.h | 5 +++++
tools/Makefile | 5 ++++-
tools/jailhouse.c | 36
++++++++++++++++++++++++++++--
24 files changed, 64 insertions(+), 3 deletions(-)

diff --git a/configs/apic-demo.c b/configs/apic-demo.c
index 046a782..f460754 100644
--- a/configs/apic-demo.c
+++ b/configs/apic-demo.c
@@ -24,6 +24,7 @@ struct {
__u8 pio_bitmap[0x2000];
} __attribute__((packed)) config = {
.cell = {
+ .magic = JAILHOUSE_CELLCONFIG_MAGIC,
.name = "apic-demo",

.cpu_set_size = sizeof(config.cpus),
diff --git a/configs/bananapi-gic-demo.c b/configs/bananapi-gic-demo.c
index f34a2a4..d81cba1 100644
--- a/configs/bananapi-gic-demo.c
+++ b/configs/bananapi-gic-demo.c
@@ -24,6 +24,7 @@ struct {
struct jailhouse_memory mem_regions[3];
} __attribute__((packed)) config = {
.cell = {
+ .magic = JAILHOUSE_CELLCONFIG_MAGIC,
.name = "bananapi-gic-demo",
.flags = JAILHOUSE_CELL_PASSIVE_COMMREG,

diff --git a/configs/bananapi-uart-demo.c b/configs/bananapi-uart-demo.c
index 3934d67..f4e3949 100644
--- a/configs/bananapi-uart-demo.c
+++ b/configs/bananapi-uart-demo.c
@@ -24,6 +24,7 @@ struct {
struct jailhouse_memory mem_regions[3];
} __attribute__((packed)) config = {
.cell = {
+ .magic = JAILHOUSE_CELLCONFIG_MAGIC,
.name = "bananapi-uart-demo",
.flags = JAILHOUSE_CELL_PASSIVE_COMMREG,

diff --git a/configs/bananapi.c b/configs/bananapi.c
index edc6477..24a7106 100644
--- a/configs/bananapi.c
+++ b/configs/bananapi.c
@@ -24,6 +24,7 @@ struct {
struct jailhouse_irqchip irqchips[1];
} __attribute__((packed)) config = {
.header = {
+ .magic = JAILHOUSE_SYSCONFIG_MAGIC,
.hypervisor_memory = {
.phys_start = 0x7c000000,
.size = 0x4000000,
diff --git a/configs/e1000-demo.c b/configs/e1000-demo.c
index 11b87cf..4869342 100644
--- a/configs/e1000-demo.c
+++ b/configs/e1000-demo.c
@@ -27,6 +27,7 @@ struct {
struct jailhouse_pci_capability pci_caps[1];
} __attribute__((packed)) config = {
.cell = {
+ .magic = JAILHOUSE_CELLCONFIG_MAGIC,
.name = "e1000-demo",
.flags = JAILHOUSE_CELL_PASSIVE_COMMREG,

diff --git a/configs/f2a88xm-hd3.c b/configs/f2a88xm-hd3.c
index 2736496..8c8016f 100644
--- a/configs/f2a88xm-hd3.c
+++ b/configs/f2a88xm-hd3.c
@@ -32,6 +32,7 @@ struct {
struct jailhouse_pci_capability pci_caps[27];
} __attribute__((packed)) config = {
.header = {
+ .magic = JAILHOUSE_SYSCONFIG_MAGIC,
.hypervisor_memory = {
.phys_start = 0x3b000000,
.size = 0x4000000,
diff --git a/configs/h87i.c b/configs/h87i.c
index c5473ac..abf4d26 100644
--- a/configs/h87i.c
+++ b/configs/h87i.c
@@ -27,6 +27,7 @@ struct {
struct jailhouse_pci_capability pci_caps[28];
} __attribute__((packed)) config = {
.header = {
+ .magic = JAILHOUSE_SYSCONFIG_MAGIC,
.hypervisor_memory = {
.phys_start = 0x3b000000,
.size = 0x4000000,
diff --git a/configs/imb-a180.c b/configs/imb-a180.c
index e74cbe8..9f3f501 100644
--- a/configs/imb-a180.c
+++ b/configs/imb-a180.c
@@ -31,6 +31,7 @@ struct {
struct jailhouse_pci_capability pci_caps[26];
} __attribute__((packed)) config = {
.header = {
+ .magic = JAILHOUSE_SYSCONFIG_MAGIC,
.hypervisor_memory = {
.phys_start = 0x3b000000,
.size = 0x4000000,
diff --git a/configs/ioapic-demo.c b/configs/ioapic-demo.c
index aec1a21..8ce0a4f 100644
--- a/configs/ioapic-demo.c
+++ b/configs/ioapic-demo.c
@@ -26,6 +26,7 @@ struct {
__u8 pio_bitmap[0x2000];
} __attribute__((packed)) config = {
.cell = {
+ .magic = JAILHOUSE_CELLCONFIG_MAGIC,
.name = "ioapic-demo",
.flags = JAILHOUSE_CELL_PASSIVE_COMMREG,

diff --git a/configs/ivshmem-demo.c b/configs/ivshmem-demo.c
index 591d9ea..2bfb3b9 100644
--- a/configs/ivshmem-demo.c
+++ b/configs/ivshmem-demo.c
@@ -24,6 +24,7 @@ struct {
struct jailhouse_pci_capability pci_caps[0];
} __attribute__((packed)) config = {
.cell = {
+ .magic = JAILHOUSE_CELLCONFIG_MAGIC,
.name = "ivshmem-demo",
.flags = JAILHOUSE_CELL_PASSIVE_COMMREG,

diff --git a/configs/jetson-tk1-demo.c b/configs/jetson-tk1-demo.c
index 02c924d..5f7872a 100644
--- a/configs/jetson-tk1-demo.c
+++ b/configs/jetson-tk1-demo.c
@@ -24,6 +24,7 @@ struct {
struct jailhouse_memory mem_regions[2];
} __attribute__((packed)) config = {
.cell = {
+ .magic = JAILHOUSE_CELLCONFIG_MAGIC,
.name = "jetson-tk1-demo",
.flags = JAILHOUSE_CELL_PASSIVE_COMMREG,

diff --git a/configs/jetson-tk1.c b/configs/jetson-tk1.c
index b3c69ef..a75755b 100644
--- a/configs/jetson-tk1.c
+++ b/configs/jetson-tk1.c
@@ -27,6 +27,7 @@ struct {
struct jailhouse_irqchip irqchips[1];
} __attribute__((packed)) config = {
.header = {
+ .magic = JAILHOUSE_SYSCONFIG_MAGIC,
.hypervisor_memory = {
.phys_start = 0xfc000000,
.size = 0x4000000 - 0x100000, /* -1MB (PSCI) */
diff --git a/configs/linux-x86-demo.c b/configs/linux-x86-demo.c
index ffb7765..7ec7f48 100644
--- a/configs/linux-x86-demo.c
+++ b/configs/linux-x86-demo.c
@@ -25,6 +25,7 @@ struct {
struct jailhouse_pci_device pci_devices[1];
} __attribute__((packed)) config = {
.cell = {
+ .magic = JAILHOUSE_CELLCONFIG_MAGIC,
.name = "linux-x86-demo",
.flags = JAILHOUSE_CELL_PASSIVE_COMMREG,

diff --git a/configs/pci-demo.c b/configs/pci-demo.c
index 9449aac..9bd1eb2 100644
--- a/configs/pci-demo.c
+++ b/configs/pci-demo.c
@@ -27,6 +27,7 @@ struct {
struct jailhouse_pci_capability pci_caps[1];
} __attribute__((packed)) config = {
.cell = {
+ .magic = JAILHOUSE_CELLCONFIG_MAGIC,
.name = "pci-demo",
.flags = JAILHOUSE_CELL_PASSIVE_COMMREG,

diff --git a/configs/qemu-vm.c b/configs/qemu-vm.c
index 637b9c6..89cf6e0 100644
--- a/configs/qemu-vm.c
+++ b/configs/qemu-vm.c
@@ -41,6 +41,7 @@ struct {
struct jailhouse_pci_capability pci_caps[5];
} __attribute__((packed)) config = {
.header = {
+ .magic = JAILHOUSE_SYSCONFIG_MAGIC,
.hypervisor_memory = {
.phys_start = 0x3b000000,
.size = 0x600000,
diff --git a/configs/smp-demo.c b/configs/smp-demo.c
index 8649466..f05ba57 100644
--- a/configs/smp-demo.c
+++ b/configs/smp-demo.c
@@ -24,6 +24,7 @@ struct {
__u8 pio_bitmap[0x2000];
} __attribute__((packed)) config = {
.cell = {
+ .magic = JAILHOUSE_CELLCONFIG_MAGIC,
.name = "smp-demo",
.flags = JAILHOUSE_CELL_PASSIVE_COMMREG,

diff --git a/configs/tiny-demo.c b/configs/tiny-demo.c
index 355a330..552716e 100644
--- a/configs/tiny-demo.c
+++ b/configs/tiny-demo.c
@@ -24,6 +24,7 @@ struct {
__u8 pio_bitmap[0x2000];
} __attribute__((packed)) config = {
.cell = {
+ .magic = JAILHOUSE_CELLCONFIG_MAGIC,
.name = "tiny-demo",
.flags = JAILHOUSE_CELL_PASSIVE_COMMREG,

diff --git a/configs/vexpress-gic-demo.c b/configs/vexpress-gic-demo.c
index 44512d4..a1249fd 100644
--- a/configs/vexpress-gic-demo.c
+++ b/configs/vexpress-gic-demo.c
@@ -21,6 +21,7 @@ struct {
struct jailhouse_memory mem_regions[2];
} __attribute__((packed)) config = {
.cell = {
+ .magic = JAILHOUSE_CELLCONFIG_MAGIC,
.name = "gic-demo",
.flags = JAILHOUSE_CELL_PASSIVE_COMMREG,

diff --git a/configs/vexpress-linux-demo.c b/configs/vexpress-linux-demo.c
index 5a3d7b5..0913c79 100644
--- a/configs/vexpress-linux-demo.c
+++ b/configs/vexpress-linux-demo.c
@@ -22,6 +22,7 @@ struct {
struct jailhouse_irqchip irqchips[1];
} __attribute__((packed)) config = {
.cell = {
+ .magic = JAILHOUSE_CELLCONFIG_MAGIC,
.name = "linux-demo",
.flags = JAILHOUSE_CELL_PASSIVE_COMMREG,

diff --git a/configs/vexpress-uart-demo.c b/configs/vexpress-uart-demo.c
index 524a08e..ee9d4ec 100644
--- a/configs/vexpress-uart-demo.c
+++ b/configs/vexpress-uart-demo.c
@@ -21,6 +21,7 @@ struct {
struct jailhouse_memory mem_regions[2];
} __attribute__((packed)) config = {
.cell = {
+ .magic = JAILHOUSE_CELLCONFIG_MAGIC,
.name = "pl011-demo",
.flags = JAILHOUSE_CELL_PASSIVE_COMMREG,

diff --git a/configs/vexpress.c b/configs/vexpress.c
index eb27345..1bde404 100644
--- a/configs/vexpress.c
+++ b/configs/vexpress.c
@@ -22,6 +22,7 @@ struct {
struct jailhouse_irqchip irqchips[1];
} __attribute__((packed)) config = {
.header = {
+ .magic = JAILHOUSE_SYSCONFIG_MAGIC,
.hypervisor_memory = {
.phys_start = 0xfc000000,
.size = 0x4000000,
diff --git a/hypervisor/include/jailhouse/cell-config.h
b/hypervisor/include/jailhouse/cell-config.h
index 261d9c7..aa55d4a 100644
--- a/hypervisor/include/jailhouse/cell-config.h
+++ b/hypervisor/include/jailhouse/cell-config.h
@@ -39,11 +39,15 @@
#ifndef _JAILHOUSE_CELL_CONFIG_H
#define _JAILHOUSE_CELL_CONFIG_H

+#define JAILHOUSE_SYSCONFIG_MAGIC 0x000000005a5a5a5a
+#define JAILHOUSE_CELLCONFIG_MAGIC 0x00000000b4b4b4b4
+
#define JAILHOUSE_CELL_NAME_MAXLEN 31

#define JAILHOUSE_CELL_PASSIVE_COMMREG 0x00000001

struct jailhouse_cell_desc {
+ __u64 magic;
char name[JAILHOUSE_CELL_NAME_MAXLEN+1];
__u32 flags;

@@ -119,6 +123,7 @@ struct jailhouse_pci_capability {
#define JAILHOUSE_MAX_IOMMU_UNITS 8

struct jailhouse_system {
+ __u64 magic;
struct jailhouse_memory hypervisor_memory;
struct jailhouse_memory debug_uart;
union {
diff --git a/tools/Makefile b/tools/Makefile
index 55d0368..bb967c1 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -12,7 +12,10 @@

CC = $(CROSS_COMPILE)gcc

-CFLAGS = -g -O3 -I../driver -DLIBEXECDIR=\"$(libexecdir)\" \
+CFLAGS = -g -O3 \
+ -I../driver \
+ -I../hypervisor/include \
+ -DLIBEXECDIR=\"$(libexecdir)\" \
-Wall -Wmissing-declarations -Wmissing-prototypes \
-DJAILHOUSE_VERSION=\"$(shell cat ../VERSION)\"

diff --git a/tools/jailhouse.c b/tools/jailhouse.c
index 6a36892..1e5704d 100644
--- a/tools/jailhouse.c
+++ b/tools/jailhouse.c
@@ -24,6 +24,7 @@
#include <sys/stat.h>

#include <jailhouse.h>
+#include <jailhouse/cell-config.h>

#define JAILHOUSE_EXEC_DIR LIBEXECDIR "/jailhouse"

@@ -146,11 +147,27 @@ static int enable(int argc, char *argv[])
{
void *config;
int err, fd;
+ size_t size;
+ const char *filename = argv[2];

if (argc != 3)
help(argv[0], 1);

- config = read_file(argv[2], NULL);
+ config = read_file(filename, &size);
+
+ // Minimum size check
+ if (size < sizeof(struct jailhouse_system)) {
+ fprintf(stderr,
+ "Jailhouse system descriptor size mismatch: %s\n",
+ filename);
+ exit(1);
+ }
+
+ if (((struct jailhouse_system*)config)->magic !=
+ JAILHOUSE_SYSCONFIG_MAGIC) {
+ fprintf(stderr, "Not a system descriptor: %s\n", filename);
+ exit(1);
+ }

fd = open_dev();

@@ -169,13 +186,28 @@ static int cell_create(int argc, char *argv[])
struct jailhouse_cell_create cell_create;
size_t size;
int err, fd;
+ const char *filename = argv[3];

if (argc != 4)
help(argv[0], 1);

- cell_create.config_address = (unsigned long)read_file(argv[3], &size);
+ cell_create.config_address = (unsigned long)read_file(filename, &size);
cell_create.config_size = size;

+ // Minimum size check
+ if (cell_create.config_size < sizeof(struct jailhouse_cell_desc)) {
+ fprintf(stderr,
+ "Jailhouse cell descriptor size mismatch: %s\n",
+ filename);
+ exit(1);
+ }
+
+ if (((struct jailhouse_cell_desc*)cell_create.config_address)->magic !=
+ JAILHOUSE_CELLCONFIG_MAGIC) {
+ fprintf(stderr, "Not a cell descriptor: %s\n", filename);
+ exit(1);
+ }
+
fd = open_dev();

err = ioctl(fd, JAILHOUSE_CELL_CREATE, &cell_create);
--
2.5.0

--
Ralf Ramsauer
GPG: 0x8F10049B

[Jan Kiszka]

On 2015-08-13 13:10, Ralf Ramsauer wrote:
> Hi,
>
> I wrote a small patch that inserts two different magics in
> jailhouse_cell_desc and jailhouse_system.
> Could someone please test this patch on real hardware? I was only able
> to check this patch using stubs.
>
> Besides that: Is it correct, that jailhouse does not do any boundary
> checks for .cell files at all?
> The ioctl calls of tools/jailhouse.c does not propagate the size of the
> memory location where the .cell file is stored.
> Additionally, the kernel driver just casts the memory to
> jailhouse_cell_desc resp. jailhouse_system - also without any boundary
> checks.
>
> Ralf
> ---
> Inserted magic byte in struct jailhouse_cell_desc and jailhouse_system.
> Jailhouse userland tool will refuse loading a system config as cell
> config et vice versa.

Let's do the check in the kernel driver and keep the userland tool
agnostic to the config format.

Please also update tools/root-cell-config.c.tmpl.

And I would suggest to align the magic type and handling with the
jailhouse.bin header (char array, string magic, memcmp), already to have
this consistent.

Thanks,
Jan

--
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

[Veaceslav Falico]

On Thu, Aug 13, 2015 at 01:10:26PM +0200, Ralf Ramsauer wrote:
>Hi,
>
>I wrote a small patch that inserts two different magics in
>jailhouse_cell_desc and jailhouse_system.
>Could someone please test this patch on real hardware? I was only able
>to check this patch using stubs.
>
>Besides that: Is it correct, that jailhouse does not do any boundary
>checks for .cell files at all?
>The ioctl calls of tools/jailhouse.c does not propagate the size of the
>memory location where the .cell file is stored.
>Additionally, the kernel driver just casts the memory to
>jailhouse_cell_desc resp. jailhouse_system - also without any boundary
>checks.
>
> Ralf
>---
>Inserted magic byte in struct jailhouse_cell_desc and jailhouse_system.
>Jailhouse userland tool will refuse loading a system config as cell
>config et vice versa.
>
>Signed-off-by: Ralf Ramsauer <ra...@ramses-pyramidenbau.de>

Also, as you'll anyway have to resubmit it - try running checkpatch on top
of your patch first, it will save quite a bit of headache :).

>--
>You received this message because you are subscribed to the Google Groups "Jailhouse" group.
>To unsubscribe from this group and stop receiving emails from it, send an email to jailhouse-de...@googlegroups.com.
>For more options, visit https://groups.google.com/d/optout.