[PATCH v3 06/11] softmmu/physmem: Fail creation of new files in file_ram_open() with readonly=true

David Hildenbrand posted 11 patches 1 year, 3 months ago
Maintainers: David Hildenbrand <david@redhat.com>, Igor Mammedov <imammedo@redhat.com>, Elena Ufimtseva <elena.ufimtseva@oracle.com>, Jagannathan Raman <jag.raman@oracle.com>, "Michael S. Tsirkin" <mst@redhat.com>, Ani Sinha <anisinha@redhat.com>, Eduardo Habkost <eduardo@habkost.net>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Yanan Wang <wangyanan55@huawei.com>, Xiao Guangrong <xiaoguangrong.eric@gmail.com>, Daniel Henrique Barboza <danielhb413@gmail.com>, "Cédric Le Goater" <clg@kaod.org>, David Gibson <david@gibson.dropbear.id.au>, Greg Kurz <groug@kaod.org>, Harsh Prateek Bora <harshpb@linux.ibm.com>, Paolo Bonzini <pbonzini@redhat.com>, Peter Xu <peterx@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com>, Eric Blake <eblake@redhat.com>, Markus Armbruster <armbru@redhat.com>
There is a newer version of this series
[PATCH v3 06/11] softmmu/physmem: Fail creation of new files in file_ram_open() with readonly=true
Posted by David Hildenbrand 1 year, 3 months ago
Currently, if a file does not exist yet, file_ram_open() will create new
empty file and open it writable. However, it even does that when
readonly=true was specified.

Specifying O_RDONLY instead to create a new readonly file would
theoretically work, however, ftruncate() will refuse to resize the new
empty file and we'll get a warning:
    ftruncate: Invalid argument
And later eventually more problems when actually mmap'ing that file and
accessing it.

If someone intends to let QEMU open+mmap a file read-only, better
create+resize+fill that file ahead of time outside of QEMU context.

We'll now fail with:
./qemu-system-x86_64 \
    -object memory-backend-file,id=ram0,mem-path=tmp,readonly=true,size=1g
qemu-system-x86_64: can't open backing store tmp for guest RAM: No such file or directory

All use cases of readonly files (R/O NVDIMMs, VM templating) work on
existing files, so silently creating new files might just hide user
errors when accidentally specifying a non-existent file.

Note that the only memory-backend-file will end up calling
memory_region_init_ram_from_file() -> qemu_ram_alloc_from_file() ->
file_ram_open().

Move error reporting to the single caller.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 softmmu/physmem.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index 32d641754b..f8c47ecb85 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -1289,8 +1289,7 @@ static int64_t get_file_align(int fd)
 static int file_ram_open(const char *path,
                          const char *region_name,
                          bool readonly,
-                         bool *created,
-                         Error **errp)
+                         bool *created)
 {
     char *filename;
     char *sanitized_name;
@@ -1305,6 +1304,10 @@ static int file_ram_open(const char *path,
             break;
         }
         if (errno == ENOENT) {
+            if (readonly) {
+                /* Refuse to create new, readonly files. */
+                return -ENOENT;
+            }
             /* @path names a file that doesn't exist, create it */
             fd = open(path, O_RDWR | O_CREAT | O_EXCL, 0644);
             if (fd >= 0) {
@@ -1334,10 +1337,7 @@ static int file_ram_open(const char *path,
             g_free(filename);
         }
         if (errno != EEXIST && errno != EINTR) {
-            error_setg_errno(errp, errno,
-                             "can't open backing store %s for guest RAM",
-                             path);
-            return -1;
+            return -errno;
         }
         /*
          * Try again on EINTR and EEXIST.  The latter happens when
@@ -1947,8 +1947,10 @@ RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr,
     RAMBlock *block;
 
     fd = file_ram_open(mem_path, memory_region_name(mr),
-                       !!(ram_flags & RAM_READONLY_FD), &created, errp);
+                       !!(ram_flags & RAM_READONLY_FD), &created);
     if (fd < 0) {
+        error_setg_errno(errp, -fd, "can't open backing store %s for guest RAM",
+                         mem_path);
         return NULL;
     }
 
-- 
2.41.0
Re: [PATCH v3 06/11] softmmu/physmem: Fail creation of new files in file_ram_open() with readonly=true
Posted by Peter Xu 1 year, 3 months ago
On Wed, Aug 23, 2023 at 05:34:06PM +0200, David Hildenbrand wrote:
> Currently, if a file does not exist yet, file_ram_open() will create new
> empty file and open it writable. However, it even does that when
> readonly=true was specified.
> 
> Specifying O_RDONLY instead to create a new readonly file would
> theoretically work, however, ftruncate() will refuse to resize the new
> empty file and we'll get a warning:
>     ftruncate: Invalid argument
> And later eventually more problems when actually mmap'ing that file and
> accessing it.
> 
> If someone intends to let QEMU open+mmap a file read-only, better
> create+resize+fill that file ahead of time outside of QEMU context.
> 
> We'll now fail with:
> ./qemu-system-x86_64 \
>     -object memory-backend-file,id=ram0,mem-path=tmp,readonly=true,size=1g
> qemu-system-x86_64: can't open backing store tmp for guest RAM: No such file or directory
> 
> All use cases of readonly files (R/O NVDIMMs, VM templating) work on
> existing files, so silently creating new files might just hide user
> errors when accidentally specifying a non-existent file.
> 
> Note that the only memory-backend-file will end up calling
> memory_region_init_ram_from_file() -> qemu_ram_alloc_from_file() ->
> file_ram_open().
> 
> Move error reporting to the single caller.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

Acked-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu