[Qemu-devel] [PATCH v2] file-posix: specify expected filetypes

[Qemu-devel] [PATCH v2] file-posix: specify expected filetypes

[John Snow]

I don't think there's a legitimate reason to open directories as if
they were files. This prevents QEMU from opening and attempting to probe
a directory inode, which can break in exciting ways. One of those ways
is lseek on ext4/xfs, which will return 0x7fffffffffffffff as the file
size instead of EISDIR. This can coax QEMU into responding with a
confusing "file too big" instead of "Hey, that's not a file".

See: https://bugs.launchpad.net/qemu/+bug/1739304/
Signed-off-by: John Snow <jsnow@redhat.com>
---

v2: Is something like this what you had in mind, Kevin?
    I couldn't make the hdev/cdrom checks more specific
    as I'm not sure which environments expect which, but
    if you know I can tighten it.

 block/file-posix.c | 38 +++++++++++++++++++++++++++++---------
 1 file changed, 29 insertions(+), 9 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 36ee89e940..fe06cdb8f8 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -417,7 +417,8 @@ static QemuOptsList raw_runtime_opts = {
 };
 
 static int raw_open_common(BlockDriverState *bs, QDict *options,
-                           int bdrv_flags, int open_flags, Error **errp)
+                           int bdrv_flags, int open_flags,
+                           bool device, Error **errp)
 {
     BDRVRawState *s = bs->opaque;
     QemuOpts *opts;
@@ -556,10 +557,30 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
         error_setg_errno(errp, errno, "Could not stat file");
         goto fail;
     }
-    if (S_ISREG(st.st_mode)) {
-        s->discard_zeroes = true;
-        s->has_fallocate = true;
+
+    if (!device) {
+        if (S_ISBLK(st.st_mode)) {
+            warn_report("Opening a block device as file using 'file'"
+                        "driver is deprecated");
+        } else if (S_ISCHR(st.st_mode)) {
+            warn_report("Opening a character device as file using the 'file'"
+                        "driver is deprecated");
+        } else if (!S_ISREG(st.st_mode)) {
+            error_setg(errp, "A regular file was expected by the 'file' driver,"
+                       "but something else was given");
+            goto fail;
+        } else {
+            s->discard_zeroes = true;
+            s->has_fallocate = true;
+        }
+    } else {
+        if (!(S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode))) {
+            error_setg(errp, "host_device/host_cdrom driver expects either"
+                       "a character or block device");
+            goto fail;
+        }
     }
+
     if (S_ISBLK(st.st_mode)) {
 #ifdef BLKDISCARDZEROES
         unsigned int arg;
@@ -589,7 +610,6 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
         s->needs_alignment = true;
     }
 #endif
-
 #ifdef CONFIG_XFS
     if (platform_test_xfs_fd(s->fd)) {
         s->is_xfs = true;
@@ -611,7 +631,7 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
     BDRVRawState *s = bs->opaque;
 
     s->type = FTYPE_FILE;
-    return raw_open_common(bs, options, flags, 0, errp);
+    return raw_open_common(bs, options, flags, 0, false, errp);
 }
 
 typedef enum {
@@ -2575,7 +2595,7 @@ hdev_open_Mac_error:
 
     s->type = FTYPE_FILE;
 
-    ret = raw_open_common(bs, options, flags, 0, &local_err);
+    ret = raw_open_common(bs, options, flags, 0, true, &local_err);
     if (ret < 0) {
         error_propagate(errp, local_err);
 #if defined(__APPLE__) && defined(__MACH__)
@@ -2802,7 +2822,7 @@ static int cdrom_open(BlockDriverState *bs, QDict *options, int flags,
     s->type = FTYPE_CD;
 
     /* open will not fail even if no CD is inserted, so add O_NONBLOCK */
-    return raw_open_common(bs, options, flags, O_NONBLOCK, errp);
+    return raw_open_common(bs, options, flags, O_NONBLOCK, true, errp);
 }
 
 static int cdrom_probe_device(const char *filename)
@@ -2915,7 +2935,7 @@ static int cdrom_open(BlockDriverState *bs, QDict *options, int flags,
 
     s->type = FTYPE_CD;
 
-    ret = raw_open_common(bs, options, flags, 0, &local_err);
+    ret = raw_open_common(bs, options, flags, 0, true, &local_err);
     if (ret) {
         error_propagate(errp, local_err);
         return ret;
-- 
2.14.3

Re: [Qemu-devel] [PATCH v2] file-posix: specify expected filetypes

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2020 bytes --]

On 01/15/2018 04:50 PM, John Snow wrote:
> I don't think there's a legitimate reason to open directories as if
> they were files. This prevents QEMU from opening and attempting to probe
> a directory inode, which can break in exciting ways. One of those ways
> is lseek on ext4/xfs, which will return 0x7fffffffffffffff as the file
> size instead of EISDIR. This can coax QEMU into responding with a
> confusing "file too big" instead of "Hey, that's not a file".
> 
> See: https://bugs.launchpad.net/qemu/+bug/1739304/
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> 

> @@ -556,10 +557,30 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
>          error_setg_errno(errp, errno, "Could not stat file");
>          goto fail;
>      }
> -    if (S_ISREG(st.st_mode)) {
> -        s->discard_zeroes = true;
> -        s->has_fallocate = true;
> +
> +    if (!device) {
> +        if (S_ISBLK(st.st_mode)) {
> +            warn_report("Opening a block device as file using 'file'"
> +                        "driver is deprecated");

Do we have a proper deprecation documentation in place, and a time frame
for when we'd start rejecting this rather than permitting it with the
warning?

> +        } else if (S_ISCHR(st.st_mode)) {
> +            warn_report("Opening a character device as file using the 'file'"
> +                        "driver is deprecated");
> +        } else if (!S_ISREG(st.st_mode)) {
> +            error_setg(errp, "A regular file was expected by the 'file' driver,"
> +                       "but something else was given");

This makes sense.


> @@ -589,7 +610,6 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
>          s->needs_alignment = true;
>      }
>  #endif
> -
>  #ifdef CONFIG_XFS
>      if (platform_test_xfs_fd(s->fd)) {

Spurious whitespace change.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v2] file-posix: specify expected filetypes

[Kevin Wolf]

Am 15.01.2018 um 23:50 hat John Snow geschrieben:
> I don't think there's a legitimate reason to open directories as if
> they were files. This prevents QEMU from opening and attempting to probe
> a directory inode, which can break in exciting ways. One of those ways
> is lseek on ext4/xfs, which will return 0x7fffffffffffffff as the file
> size instead of EISDIR. This can coax QEMU into responding with a
> confusing "file too big" instead of "Hey, that's not a file".
> 
> See: https://bugs.launchpad.net/qemu/+bug/1739304/
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> 
> v2: Is something like this what you had in mind, Kevin?
>     I couldn't make the hdev/cdrom checks more specific
>     as I'm not sure which environments expect which, but
>     if you know I can tighten it.

This should do. We could in theory be more specific about which OS
expects block devices and which one uses character devices, but there's
probably no point in doing that. And without further research I couldn't
say much more than that Linux uses block devices and FreeBSD character
devices.

>  block/file-posix.c | 38 +++++++++++++++++++++++++++++---------
>  1 file changed, 29 insertions(+), 9 deletions(-)

As Eric said, it would be good to add a deprecation note to
qemu-doc.texi (and probably also the changelog in the wiki).

> diff --git a/block/file-posix.c b/block/file-posix.c
> index 36ee89e940..fe06cdb8f8 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -417,7 +417,8 @@ static QemuOptsList raw_runtime_opts = {
>  };
>  
>  static int raw_open_common(BlockDriverState *bs, QDict *options,
> -                           int bdrv_flags, int open_flags, Error **errp)
> +                           int bdrv_flags, int open_flags,
> +                           bool device, Error **errp)
>  {
>      BDRVRawState *s = bs->opaque;
>      QemuOpts *opts;
> @@ -556,10 +557,30 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
>          error_setg_errno(errp, errno, "Could not stat file");
>          goto fail;
>      }
> -    if (S_ISREG(st.st_mode)) {
> -        s->discard_zeroes = true;
> -        s->has_fallocate = true;
> +
> +    if (!device) {
> +        if (S_ISBLK(st.st_mode)) {
> +            warn_report("Opening a block device as file using 'file'"
> +                        "driver is deprecated");

All your line-wrapped strings are missing a space at the end of the
first line.

Kevin