On Mon, May 17, 2021 at 02:53:12PM +0000, Jing Zhang wrote:
> Provides a file descriptor per VM to read VM stats info/data.
> Provides a file descriptor per vCPU to read vCPU stats info/data.
>
> Signed-off-by: Jing Zhang <jingzhangos@xxxxxxxxxx>
> ---
> arch/arm64/kvm/guest.c | 26 +++++
> arch/mips/kvm/mips.c | 52 +++++++++
> arch/powerpc/kvm/book3s.c | 52 +++++++++
> arch/powerpc/kvm/booke.c | 45 ++++++++
> arch/s390/kvm/kvm-s390.c | 117 ++++++++++++++++++++
> arch/x86/kvm/x86.c | 53 +++++++++
> include/linux/kvm_host.h | 127 ++++++++++++++++++++++
> include/uapi/linux/kvm.h | 50 +++++++++
> virt/kvm/kvm_main.c | 223 ++++++++++++++++++++++++++++++++++++++
> 9 files changed, 745 insertions(+)
>
> +static ssize_t kvm_vcpu_stats_read(struct file *file, char __user *user_buffer,
> + size_t size, loff_t *offset)
> +{
> + char id[KVM_STATS_ID_MAXLEN];
> + struct kvm_vcpu *vcpu = file->private_data;
> + ssize_t copylen, len, remain = size;
> + size_t size_header, size_desc, size_stats;
> + loff_t pos = *offset;
> + char __user *dest = user_buffer;
> + void *src;
Nit. Better to do pointer arithmetic on a "char *". Note that gcc and
clang will do the expected thing.
> +
> + snprintf(id, sizeof(id), "kvm-%d/vcpu-%d",
> + task_pid_nr(current), vcpu->vcpu_id);
> + size_header = sizeof(kvm_vcpu_stats_header);
> + size_desc =
> + kvm_vcpu_stats_header.count * sizeof(struct _kvm_stats_desc);
> + size_stats = sizeof(vcpu->stat);
> +
> + len = sizeof(id) + size_header + size_desc + size_stats - pos;
> + len = min(len, remain);
> + if (len <= 0)
> + return 0;
> + remain = len;
If 'desc_offset' is not right after the header, then the 'len'
calculation is missing the gap into account. For example, assuming there
is a gap of 0x1000000 between the header and the descriptors:
desc_offset = sizeof(id) + size_header + 0x1000000
and the user calls the ioctl with enough space for the whole file,
including the gap:
*offset = 0
size = sizeof(id) + size_header + size_desc + size_stats + 0x1000000
then 'remain' gets the wrong size:
remain = sizeof(id) + size_header + size_desc + size_stats
and ... (more below)
> +
> + /* Copy kvm vcpu stats header id string */
> + copylen = sizeof(id) - pos;
> + copylen = min(copylen, remain);
> + if (copylen > 0) {
> + src = (void *)id + pos;
> + if (copy_to_user(dest, src, copylen))
> + return -EFAULT;
> + remain -= copylen;
> + pos += copylen;
> + dest += copylen;
> + }
> + /* Copy kvm vcpu stats header */
> + copylen = sizeof(id) + size_header - pos;
> + copylen = min(copylen, remain);
> + if (copylen > 0) {
> + src = (void *)&kvm_vcpu_stats_header;
> + src += pos - sizeof(id);
> + if (copy_to_user(dest, src, copylen))
> + return -EFAULT;
> + remain -= copylen;
> + pos += copylen;
> + dest += copylen;
> + }
> + /* Copy kvm vcpu stats descriptors */
> + copylen = kvm_vcpu_stats_header.desc_offset + size_desc - pos;
This would be the state at this point:
pos = sizeof(id) + size_header
copylen = sizeof(id) + size_header + 0x1000000 + size_desc - (sizeof(id) + size_header)
= 0x1000000 + size_desc
remain = size_desc + size_stats
> + copylen = min(copylen, remain);
copylen = size_desc + size_stats
which is not enough to copy the descriptors (and the data).
> + if (copylen > 0) {
> + src = (void *)&kvm_vcpu_stats_desc;
> + src += pos - kvm_vcpu_stats_header.desc_offset;
Moreover, src also needs to take the gap into account.
src = &kvm_vcpu_stats_desc + (sizeof(id) + size_header) - (sizeof(id) + size_header + 0x1000000)
= &kvm_vcpu_stats_desc - 0x1000000
Otherwise, src ends up pointing at the wrong place.
> + if (copy_to_user(dest, src, copylen))
> + return -EFAULT;
> + remain -= copylen;
> + pos += copylen;
> + dest += copylen;
> + }
> + /* Copy kvm vcpu stats values */
> + copylen = kvm_vcpu_stats_header.data_offset + size_stats - pos;
The same problem occurs here. There is a potential gap before
data_offset that needs to be taken into account for src and len.
Would it be possible to just ensure that there is no gap? maybe even
remove data_offset and desc_offset and always place them adjacent, and
have the descriptors right after the header.
> + copylen = min(copylen, remain);
> + if (copylen > 0) {
> + src = (void *)&vcpu->stat;
> + src += pos - kvm_vcpu_stats_header.data_offset;
> + if (copy_to_user(dest, src, copylen))
> + return -EFAULT;
> + remain -= copylen;
> + pos += copylen;
> + dest += copylen;
> + }
> +
> + *offset = pos;
> + return len;
> +}
> +
>
> +static ssize_t kvm_vm_stats_read(struct file *file, char __user *user_buffer,
> + size_t size, loff_t *offset)
> +{
Consider moving the common code between kvm_vcpu_stats_read and this one
into some function that takes pointers to header, desc, and data. Unless
there is something vcpu or vm specific besides that.
> + char id[KVM_STATS_ID_MAXLEN];
> + struct kvm *kvm = file->private_data;
> + ssize_t copylen, len, remain = size;
> + size_t size_header, size_desc, size_stats;
> + loff_t pos = *offset;
> + char __user *dest = user_buffer;
> + void *src;
> +
> + snprintf(id, sizeof(id), "kvm-%d", task_pid_nr(current));
> + size_header = sizeof(kvm_vm_stats_header);
> + size_desc = kvm_vm_stats_header.count * sizeof(struct _kvm_stats_desc);
> + size_stats = sizeof(kvm->stat);
> +
> + len = sizeof(id) + size_header + size_desc + size_stats - pos;
> + len = min(len, remain);
> + if (len <= 0)
> + return 0;
> + remain = len;
> +
> + /* Copy kvm vm stats header id string */
> + copylen = sizeof(id) - pos;
> + copylen = min(copylen, remain);
> + if (copylen > 0) {
> + src = (void *)id + pos;
> + if (copy_to_user(dest, src, copylen))
> + return -EFAULT;
> + remain -= copylen;
> + pos += copylen;
> + dest += copylen;
> + }
> + /* Copy kvm vm stats header */
> + copylen = sizeof(id) + size_header - pos;
> + copylen = min(copylen, remain);
> + if (copylen > 0) {
> + src = (void *)&kvm_vm_stats_header;
> + src += pos - sizeof(id);
> + if (copy_to_user(dest, src, copylen))
> + return -EFAULT;
> + remain -= copylen;
> + pos += copylen;
> + dest += copylen;
> + }
> + /* Copy kvm vm stats descriptors */
> + copylen = kvm_vm_stats_header.desc_offset + size_desc - pos;
> + copylen = min(copylen, remain);
> + if (copylen > 0) {
> + src = (void *)&kvm_vm_stats_desc;
> + src += pos - kvm_vm_stats_header.desc_offset;
> + if (copy_to_user(dest, src, copylen))
> + return -EFAULT;
> + remain -= copylen;
> + pos += copylen;
> + dest += copylen;
> + }
> + /* Copy kvm vm stats values */
> + copylen = kvm_vm_stats_header.data_offset + size_stats - pos;
> + copylen = min(copylen, remain);
> + if (copylen > 0) {
> + src = (void *)&kvm->stat;
> + src += pos - kvm_vm_stats_header.data_offset;
> + if (copy_to_user(dest, src, copylen))
> + return -EFAULT;
> + remain -= copylen;
> + pos += copylen;
> + dest += copylen;
> + }
> +
> + *offset = pos;
> + return len;
> +}
> +
> --
> 2.31.1.751.gd2f1c929bd-goog
>
> _______________________________________________
> kvmarm mailing list
> kvmarm@xxxxxxxxxxxxxxxxxxxxx
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
