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@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
