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

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

[John Snow]

Adjust each caller of raw_open_common to specify if they are expecting
host and character devices or not. Tighten expectations of file types upon
open in the common code and refuse types that are not expected.

This has two effects:

(1) Character and block devices are now considered deprecated for the
    'file' driver, which expects only S_IFREG, and
(2) no file-posix driver (file, host_cdrom, or host_device) can open
    directories now.

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>
---

v5: rebase for 2.12.0-rc0

 block/file-posix.c | 37 +++++++++++++++++++++++++++++--------
 qemu-doc.texi      |  6 ++++++
 2 files changed, 35 insertions(+), 8 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index d7fb772c14..31d9afe026 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -420,7 +420,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;
@@ -558,10 +559,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;
@@ -614,7 +635,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 {
@@ -2611,7 +2632,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__)
@@ -2838,7 +2859,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)
@@ -2951,7 +2972,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;
diff --git a/qemu-doc.texi b/qemu-doc.texi
index 89fa80518a..ff4a098fd7 100644
--- a/qemu-doc.texi
+++ b/qemu-doc.texi
@@ -2699,6 +2699,12 @@ that can be specified with the ``-device'' parameter.
 The drive addr argument is replaced by the the addr argument
 that can be specified with the ``-device'' parameter.
 
+@subsection -drive file=json:@{...@{'driver':'file'@}@} (since 2.12.0)
+
+The 'file' driver for drives is no longer appropriate for character or host
+devices and will only accept regular files (S_IFREG). The correct driver
+for these file types is 'host_cdrom' or 'host_device' as appropriate.
+
 @subsection -usbdevice (since 2.10.0)
 
 The ``-usbdevice DEV'' argument is now a synonym for setting
-- 
2.14.3

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

[Kevin Wolf]

Am 21.03.2018 um 21:01 hat John Snow geschrieben:
> Adjust each caller of raw_open_common to specify if they are expecting
> host and character devices or not. Tighten expectations of file types upon
> open in the common code and refuse types that are not expected.
> 
> This has two effects:
> 
> (1) Character and block devices are now considered deprecated for the
>     'file' driver, which expects only S_IFREG, and
> (2) no file-posix driver (file, host_cdrom, or host_device) can open
>     directories now.
> 
> 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>
> ---
> 
> v5: rebase for 2.12.0-rc0
> 
>  block/file-posix.c | 37 +++++++++++++++++++++++++++++--------
>  qemu-doc.texi      |  6 ++++++
>  2 files changed, 35 insertions(+), 8 deletions(-)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index d7fb772c14..31d9afe026 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -420,7 +420,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;
> @@ -558,10 +559,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;

ret needs to be set here, otherwise we return success. In my test, I
still got the wrong message: "Could not refresh total sector count:
Invalid argument"

> +        } 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;

Same here.

> +        }
>      }

Do we want a qemu-iotests case for this?

Kevin

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

[John Snow]

On 03/21/2018 04:25 PM, Kevin Wolf wrote:
> Am 21.03.2018 um 21:01 hat John Snow geschrieben:
>> Adjust each caller of raw_open_common to specify if they are expecting
>> host and character devices or not. Tighten expectations of file types upon
>> open in the common code and refuse types that are not expected.
>>
>> This has two effects:
>>
>> (1) Character and block devices are now considered deprecated for the
>>     'file' driver, which expects only S_IFREG, and
>> (2) no file-posix driver (file, host_cdrom, or host_device) can open
>>     directories now.
>>
>> 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>
>> ---
>>
>> v5: rebase for 2.12.0-rc0
>>
>>  block/file-posix.c | 37 +++++++++++++++++++++++++++++--------
>>  qemu-doc.texi      |  6 ++++++
>>  2 files changed, 35 insertions(+), 8 deletions(-)
>>
>> diff --git a/block/file-posix.c b/block/file-posix.c
>> index d7fb772c14..31d9afe026 100644
>> --- a/block/file-posix.c
>> +++ b/block/file-posix.c
>> @@ -420,7 +420,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;
>> @@ -558,10 +559,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;
> 
> ret needs to be set here, otherwise we return success. In my test, I
> still got the wrong message: "Could not refresh total sector count:
> Invalid argument"
> 
>> +        } 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;
> 
> Same here.
> 
>> +        }
>>      }
> 
> Do we want a qemu-iotests case for this?
> 
> Kevin
> 

I'll take the hint :)

--js

Re: [Qemu-devel] [PATCH for-2.12 v5] file-posix: specify expected filetypes

[Eric Blake]

On 03/21/2018 03:01 PM, John Snow wrote:
> Adjust each caller of raw_open_common to specify if they are expecting
> host and character devices or not. Tighten expectations of file types upon
> open in the common code and refuse types that are not expected.
> 
> This has two effects:
> 
> (1) Character and block devices are now considered deprecated for the
>      'file' driver, which expects only S_IFREG, and

Which may have interesting effects on v2v's attempt to use qemu-img on 
/dev/null as a quick feature probe of qemu-img...
https://lists.gnu.org/archive/html/qemu-devel/2018-03/msg05753.html

but we may want to special-case that one in a separate patch.

> (2) no file-posix driver (file, host_cdrom, or host_device) can open
>      directories now.
> 
> 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>
> ---
> 
> v5: rebase for 2.12.0-rc0

Still makes sense as a 2.12 bugfix.

Reviewed-by: Eric Blake <eblake@redhat.com>

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

Re: [Qemu-devel] [PATCH for-2.12 v5] file-posix: specify expected filetypes

[Kevin Wolf]

Am 21.03.2018 um 21:26 hat Eric Blake geschrieben:
> On 03/21/2018 03:01 PM, John Snow wrote:
> > Adjust each caller of raw_open_common to specify if they are expecting
> > host and character devices or not. Tighten expectations of file types upon
> > open in the common code and refuse types that are not expected.
> > 
> > This has two effects:
> > 
> > (1) Character and block devices are now considered deprecated for the
> >      'file' driver, which expects only S_IFREG, and
> 
> Which may have interesting effects on v2v's attempt to use qemu-img on
> /dev/null as a quick feature probe of qemu-img...
> https://lists.gnu.org/archive/html/qemu-devel/2018-03/msg05753.html
> 
> but we may want to special-case that one in a separate patch.

Do we know the exact check is performs? I doubt it explicitly specifies
the file driver, and the automatic detection already returned the right
protocol driver.

Kevin