[libvirt] [PATCH] storage: don't read storage volumes in nonblock mode

[Michal Privoznik]

On 25.11.2013 22:49, Eric Blake wrote:
> Commit 348b4e2 introduced a potential problem (thankfully not
> in any release): we are attempting to use virFileReadHeaderFD()
> on a file that was opened with O_NONBLOCK.  While this
> shouldn't be a problem in practice (because O_NONBLOCK
> typically doesn't affect regular or block files, and fifos and
> sockets cannot be storage volumes), it's better to play it safe
> to avoid races from opening an unexpected file type while also
> avoiding problems with having to handle EAGAIN while read()ing.
> 
> Based on a report by Dan Berrange.
> 
> * src/storage/storage_backend.c
> (virStorageBackendVolOpenCheckMode): Fix up fd after avoiding race.
> 
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
>  src/storage/storage_backend.c | 25 +++++++++++++++++++++----
>  1 file changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
> index 57c1728..bde39d6 100644
> --- a/src/storage/storage_backend.c
> +++ b/src/storage/storage_backend.c
> @@ -1179,6 +1179,12 @@ virStorageBackendVolOpenCheckMode(const char *path, struct stat *sb,
>          return -2;
>      }
> 
> +    /* O_NONBLOCK should only matter during open() for fifos and
> +     * sockets, which we already filtered; but using it prevents a
> +     * TOCTTOU race.  However, later on we will want to read() the
> +     * header from this fd, and virFileRead* routines require a
> +     * blocking fd, so fix it up after verifying we avoided a
> +     * race.  */
>      if ((fd = open(path, O_RDONLY|O_NONBLOCK|O_NOCTTY)) < 0) {
>          if ((errno == ENOENT || errno == ELOOP) &&
>              S_ISLNK(sb->st_mode)) {
> @@ -1200,13 +1206,13 @@ virStorageBackendVolOpenCheckMode(const char *path, struct stat *sb,
>          return -1;
>      }
> 
> -    if (S_ISREG(sb->st_mode))
> +    if (S_ISREG(sb->st_mode)) {
>          mode = VIR_STORAGE_VOL_OPEN_REG;
> -    else if (S_ISCHR(sb->st_mode))
> +    } else if (S_ISCHR(sb->st_mode)) {
>          mode = VIR_STORAGE_VOL_OPEN_CHAR;
> -    else if (S_ISBLK(sb->st_mode))
> +    } else if (S_ISBLK(sb->st_mode)) {
>          mode = VIR_STORAGE_VOL_OPEN_BLOCK;
> -    else if (S_ISDIR(sb->st_mode)) {
> +    } else if (S_ISDIR(sb->st_mode)) {
>          mode = VIR_STORAGE_VOL_OPEN_DIR;
> 
>          if (STREQ(base, ".") ||
> @@ -1215,6 +1221,17 @@ virStorageBackendVolOpenCheckMode(const char *path, struct stat *sb,
>              VIR_INFO("Skipping special dir '%s'", base);
>              return -2;
>          }
> +    } else {
> +        VIR_WARN("ignoring unexpected type for file '%s'", path);
> +        VIR_FORCE_CLOSE(fd);
> +        return -2;
> +    }
> +
> +    if (virSetBlocking(fd, true) < 0) {
> +        virReportSystemError(errno, _("unable to set blocking mode for '%s'"),
> +                             path);
> +        VIR_FORCE_CLOSE(fd);
> +        return -2;
>      }
> 
>      if (!(mode & flags)) {
> 

ACK

Michal