Re: [PATCH v2] virtio_blk: add VIRTIO_BLK_F_LIFETIME feature support

[Alvaro Karsz <alvaro.karsz@solid-run.com>]

Thanks Stefan

> How about naming it VBLK_GET_LIFETIME? It's clearer what the ioctl does
> and it follows the name of virtio-blk request type.

You're right, I'll rename it.

> ENOTTY already has meaning for ioctl(2):
>
>
>   ENOTTY fd is not associated with a character special device.
>
>
>   ENOTTY The specified request does not apply to the kind of object that the file descriptor fd references.
>
>
> Use ENOTSUP instead?

 ENOTSUP seems like a better fit, I'll change the error code.

> In terms of security, any process with access to the block device node
> is allowed to get the lifetime?
>
>
> Usually only privileged processes have access to block device nodes, but
> I wanted to check whether anyone can think of a scenario where this is
> not okay.
>
>
> For example, a virtio-blk device may have a partition that an untrusted
> process like a database or key-value store accesses. Can the untrusted
> process read the lifetime information of the entire device?


I agree that only a privileged process should be able to read the lifetime.
I could add something like:
if (!capable(CAP_SYS_ADMIN))
        return -EPERM;

> It's unusual for an ioctl to produce a struct that's not in CPU
> endianness. I think the kernel should deal with endianness here.

The endianness was discussed in the first version:

> > After more thought, I think that the driver should handle the
> > virtio_blk_lifetime struct endianness.
> > Something like:
> > ...
> > lifetime.pre_eol_info = __le16_to_cpu(lifetime.pre_eol_info);
> > lifetime. device_lifetime_est_typ_a  = __le16_to_cpu(lifetime.
> > device_lifetime_est_typ_a);
> > lifetime. device_lifetime_est_typ_b  = __le16_to_cpu(lifetime.
> > device_lifetime_est_typ_b);
> >
> > if (copy_to_user((void __user *)arg, (void *)&lifetime,
> > ...
> >
> > What do you think?
>
>
> I think if you are going to pass struct virtio_blk_lifetime to
> userspace, better pass it as defined in the spec, in LE format.


I tend to agree that endianness should be taken care of in the kernel.

> We need to check that vblk->vdev is non-NULL before accessing it in
> virtblk_ioctl_lifetime():
>
>
>   if (!vblk->vdev) {
>       mutex_unlock(&vblk->dev_mutex);
>       return -ENXIO;
>   }
>
>
> Without the check I expect virtblk_ioctl_lifetime() to dereference a
> NULL pointer.

Right

Alvaro
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization