From 3f0a805e407f91ef2bab4fdfe3ee91cef7785d6d Mon Sep 17 00:00:00 2001 From: ChinYikMing Date: Thu, 13 Mar 2025 03:04:31 +0800 Subject: [PATCH 1/2] Support readonly feature of VirtIO block device Add support for comma-separated options for VirtIO devices. For the readonly option, if the string readonly is specified, subsequent VirtIO block device access will be read-only; otherwise, the default behavior is read and write. To implement this, a device_features field is introduced to store device features during initialization. VIRTIO_BLK_F_RO is the feature bit to enable readonly. This feature enhances the security of the VirtIO block device by preventing unintended data corruption in the guest OS when accessing critical data. Additionally, after implementing the VirtIO block device in #539, some register access errors were shown and they are fixed in this commit: - reading from the VirtIO DeviceFeatures register now correctly checks the device_features_sel instead of driver_features_sel. - writing to the VirtIO DeviceFeaturesSel register now correctly updates device_features_sel instead of driver_features_sel. --- src/devices/virtio-blk.c | 29 +++++++++++++++++++++-------- src/devices/virtio.h | 8 +++++++- src/main.c | 3 ++- src/riscv.c | 33 ++++++++++++++++++++++++++++++--- 4 files changed, 60 insertions(+), 13 deletions(-) diff --git a/src/devices/virtio-blk.c b/src/devices/virtio-blk.c index 45f7d52f9..a93f04275 100644 --- a/src/devices/virtio-blk.c +++ b/src/devices/virtio-blk.c @@ -94,11 +94,13 @@ static void virtio_blk_update_status(virtio_blk_state_t *vblk, uint32_t status) return; /* Reset */ + uint32_t device_features = vblk->device_features; uint32_t *ram = vblk->ram; uint32_t *disk = vblk->disk; void *priv = vblk->priv; uint32_t capacity = VBLK_PRIV(vblk)->capacity; memset(vblk, 0, sizeof(*vblk)); + vblk->device_features = device_features; vblk->ram = ram; vblk->disk = disk; vblk->priv = priv; @@ -187,6 +189,11 @@ static int virtio_blk_desc_handler(virtio_blk_state_t *vblk, virtio_blk_read_handler(vblk, sector, vq_desc[1].addr, vq_desc[1].len); break; case VIRTIO_BLK_T_OUT: + if (vblk->device_features & VIRTIO_BLK_F_RO) { /* readonly */ + rv_log_error("Fail to write on a read only block device"); + *status = VIRTIO_BLK_S_IOERR; + return -1; + } virtio_blk_write_handler(vblk, sector, vq_desc[1].addr, vq_desc[1].len); break; default: @@ -279,9 +286,9 @@ uint32_t virtio_blk_read(virtio_blk_state_t *vblk, uint32_t addr) case _(VendorID): return VIRTIO_VENDOR_ID; case _(DeviceFeatures): - return vblk->driver_features_sel == 0 - ? VBLK_FEATURES_0 - : (vblk->driver_features_sel == 1 ? VBLK_FEATURES_1 : 0); + return vblk->device_features_sel == 0 + ? VBLK_FEATURES_0 | vblk->device_features + : (vblk->device_features_sel == 1 ? VBLK_FEATURES_1 : 0); case _(QueueNumMax): return VBLK_QUEUE_NUM_MAX; case _(QueueReady): @@ -305,7 +312,7 @@ void virtio_blk_write(virtio_blk_state_t *vblk, uint32_t addr, uint32_t value) #define _(reg) VIRTIO_##reg switch (addr) { case _(DeviceFeaturesSel): - vblk->driver_features_sel = value; + vblk->device_features_sel = value; break; case _(DriverFeatures): vblk->driver_features_sel == 0 ? (vblk->driver_features = value) : 0; @@ -371,7 +378,9 @@ void virtio_blk_write(virtio_blk_state_t *vblk, uint32_t addr, uint32_t value) #undef _ } -uint32_t *virtio_blk_init(virtio_blk_state_t *vblk, char *disk_file) +uint32_t *virtio_blk_init(virtio_blk_state_t *vblk, + char *disk_file, + bool readonly) { if (vblk_dev_cnt >= VBLK_DEV_CNT_MAX) { rv_log_error( @@ -391,7 +400,7 @@ uint32_t *virtio_blk_init(virtio_blk_state_t *vblk, char *disk_file) } /* Open disk file */ - int disk_fd = open(disk_file, O_RDWR); + int disk_fd = open(disk_file, readonly ? O_RDONLY : O_RDWR); if (disk_fd < 0) { rv_log_error("Could not open %s", disk_file); exit(EXIT_FAILURE); @@ -405,8 +414,9 @@ uint32_t *virtio_blk_init(virtio_blk_state_t *vblk, char *disk_file) /* Set up the disk memory */ uint32_t *disk_mem; #if HAVE_MMAP - disk_mem = mmap(NULL, VBLK_PRIV(vblk)->disk_size, PROT_READ | PROT_WRITE, - MAP_SHARED, disk_fd, 0); + disk_mem = mmap(NULL, VBLK_PRIV(vblk)->disk_size, + readonly ? PROT_READ : (PROT_READ | PROT_WRITE), MAP_SHARED, + disk_fd, 0); if (disk_mem == MAP_FAILED) goto err; #else @@ -421,6 +431,9 @@ uint32_t *virtio_blk_init(virtio_blk_state_t *vblk, char *disk_file) VBLK_PRIV(vblk)->capacity = (VBLK_PRIV(vblk)->disk_size - 1) / DISK_BLK_SIZE + 1; + if (readonly) + vblk->device_features = VIRTIO_BLK_F_RO; + return disk_mem; err: diff --git a/src/devices/virtio.h b/src/devices/virtio.h index 27d54442c..b0cbadd3a 100644 --- a/src/devices/virtio.h +++ b/src/devices/virtio.h @@ -33,6 +33,9 @@ #define VIRTIO_BLK_S_IOERR 1 #define VIRTIO_BLK_S_UNSUPP 2 +/* TODO: support more features */ +#define VIRTIO_BLK_F_RO (1 << 5) + /* VirtIO MMIO registers */ #define VIRTIO_REG_LIST \ _(MagicValue, 0x000) /* R */ \ @@ -87,6 +90,7 @@ typedef struct { typedef struct { /* feature negotiation */ + uint32_t device_features; uint32_t device_features_sel; uint32_t driver_features; uint32_t driver_features_sel; @@ -107,7 +111,9 @@ uint32_t virtio_blk_read(virtio_blk_state_t *vblk, uint32_t addr); void virtio_blk_write(virtio_blk_state_t *vblk, uint32_t addr, uint32_t value); -uint32_t *virtio_blk_init(virtio_blk_state_t *vblk, char *disk_file); +uint32_t *virtio_blk_init(virtio_blk_state_t *vblk, + char *disk_file, + bool readonly); virtio_blk_state_t *vblk_new(); diff --git a/src/main.c b/src/main.c index dfb67b7b9..bed7bc7a5 100644 --- a/src/main.c +++ b/src/main.c @@ -74,7 +74,8 @@ static void print_usage(const char *filename) #if RV32_HAS(SYSTEM) && !RV32_HAS(ELF_LOADER) " -k : use as kernel image\n" " -i : use as rootfs\n" - " -x vblk: : use as virtio-blk disk image\n" + " -x vblk:[,readonly] : use as virtio-blk disk image " + "(default read and write)\n" " -b : use customized for the kernel\n" #endif " -d [filename]: dump registers as JSON to the " diff --git a/src/riscv.c b/src/riscv.c index 4616f47cd..f1f474751 100644 --- a/src/riscv.c +++ b/src/riscv.c @@ -535,9 +535,36 @@ riscv_t *rv_create(riscv_user_t rv_attr) attr->uart->out_fd = attr->fd_stdout; /* setup virtio-blk */ - attr->vblk = vblk_new(); - attr->vblk->ram = (uint32_t *) attr->mem->mem_base; - attr->disk = virtio_blk_init(attr->vblk, attr->data.system.vblk_device); + if (attr->data.system.vblk_device) { +/* Currently, only used for block image path and permission */ +#define MAX_OPTS 2 + char *vblk_opts[MAX_OPTS] = {NULL}; + int vblk_opt_idx = 0; + char *opt = strtok(attr->data.system.vblk_device, ","); + while (opt) { + if (vblk_opt_idx == MAX_OPTS) { + rv_log_error("Too many arguments for vblk"); + break; + } + vblk_opts[vblk_opt_idx++] = opt; + opt = strtok(NULL, ","); + } + char *vblk_device = vblk_opts[0]; + char *vblk_readonly = vblk_opts[1]; + + bool readonly = false; + if (vblk_readonly) { + if (strcmp(vblk_readonly, "readonly") != 0) { + rv_log_error("Unknown vblk option: %s", vblk_readonly); + exit(EXIT_FAILURE); + } + readonly = true; + } + + attr->vblk = vblk_new(); + attr->vblk->ram = (uint32_t *) attr->mem->mem_base; + attr->disk = virtio_blk_init(attr->vblk, vblk_device, readonly); + } capture_keyboard_input(); #endif /* !RV32_HAS(SYSTEM) || (RV32_HAS(SYSTEM) && RV32_HAS(ELF_LOADER)) */ From c9e2b5fbeb2ef9de4f1e45267c46b48eb756b896 Mon Sep 17 00:00:00 2001 From: ChinYikMing Date: Sun, 16 Mar 2025 17:08:49 +0800 Subject: [PATCH 2/2] Update README for the readonly feature of VBLK --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 7841d9b7d..092a57572 100644 --- a/README.md +++ b/README.md @@ -75,10 +75,10 @@ If `ENABLE_ARCH_TEST=1` was previously set, run `make distclean` before proceedi $ make ENABLE_SYSTEM=1 system ``` -Build using run using specified images: +Build and run using specified images (`readonly` option makes the virtual block device read-only): ```shell $ make ENABLE_SYSTEM=1 -$ build/rv32emu -k -i [-x vblk:] +$ build/rv32emu -k -i [-x vblk:[,readonly]] ``` Build with a larger INITRD_SIZE (e.g., 64 MiB) to run SDL-oriented application because the default 8 MiB is insufficient for SDL-oriented application artifacts: