linux-kernel - Re: [PATCH v2 2/2] Drivers: hv: Introduce mshv

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <b72e2e4d-b66f-4cba-8e8e-0afc605fa082@linux.microsoft.com>
Date: Wed, 14 May 2025 11:28:38 +0530
From: Naman Jain <namjain@...ux.microsoft.com>
To: ALOK TIWARI <alok.a.tiwari@...cle.com>,
 "K . Y . Srinivasan" <kys@...rosoft.com>,
 Haiyang Zhang <haiyangz@...rosoft.com>, Wei Liu <wei.liu@...nel.org>,
 Dexuan Cui <decui@...rosoft.com>
Cc: Roman Kisel <romank@...ux.microsoft.com>,
 Anirudh Rayabharam <anrayabh@...ux.microsoft.com>,
 Saurabh Sengar <ssengar@...ux.microsoft.com>,
 Stanislav Kinsburskii <skinsburskii@...ux.microsoft.com>,
 Nuno Das Neves <nunodasneves@...ux.microsoft.com>,
 linux-kernel@...r.kernel.org, linux-hyperv@...r.kernel.org
Subject: Re: [PATCH v2 2/2] Drivers: hv: Introduce mshv_vtl driver



On 5/14/2025 2:04 AM, ALOK TIWARI wrote:
> 
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2023, Microsoft Corporation.
>> + *
>> + * Author:
>> + *   Roman Kisel <romank@...ux.microsoft.com>
>> + *   Saurabh Sengar <ssengar@...ux.microsoft.com>
>> + *   Naman Jain <namjain@...ux.microsoft.com>
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/miscdevice.h>
>> +#include <linux/anon_inodes.h>
>> +#include <linux/pfn_t.h>
>> +#include <linux/cpuhotplug.h>
>> +#include <linux/count_zeros.h>
>> +#include <linux/eventfd.h>
>> +#include <linux/poll.h>
>> +#include <linux/file.h>
>> +#include <linux/vmalloc.h>
>> +#include <asm/debugreg.h>
>> +#include <asm/mshyperv.h>
>> +#include <trace/events/ipi.h>
>> +#include <uapi/asm/mtrr.h>
>> +#include <uapi/linux/mshv.h>
>> +#include <hyperv/hvhdk.h>
>> +
>> +#include "../../kernel/fpu/legacy.h"
>> +#include "mshv.h"
>> +#include "mshv_vtl.h"
>> +#include "hyperv_vmbus.h"
>> +
>> +MODULE_AUTHOR("Microsoft");
>> +MODULE_LICENSE("GPL");
>> +MODULE_DESCRIPTION("Microsoft Hyper-V VTL Driver");
>> +
>> +#define MSHV_ENTRY_REASON_LOWER_VTL_CALL     0x1
>> +#define MSHV_ENTRY_REASON_INTERRUPT          0x2
>> +#define MSHV_ENTRY_REASON_INTERCEPT          0x3
>> +
>> +#define MAX_GUEST_MEM_SIZE    BIT_ULL(40)
>> +#define MSHV_PG_OFF_CPU_MASK    0xFFFF
>> +#define MSHV_REAL_OFF_SHIFT    16
>> +#define MSHV_RUN_PAGE_OFFSET    0
>> +#define MSHV_REG_PAGE_OFFSET    1
>> +#define VTL2_VMBUS_SINT_INDEX    7
>> +
>> +static struct device *mem_dev;
>> +
>> +static struct tasklet_struct msg_dpc;
>> +static wait_queue_head_t fd_wait_queue;
>> +static bool has_message;
>> +static struct eventfd_ctx *flag_eventfds[HV_EVENT_FLAGS_COUNT];
>> +static DEFINE_MUTEX(flag_lock);
>> +static bool __read_mostly mshv_has_reg_page;
>> +
>> +struct mshv_vtl_hvcall_fd {
>> +    u64 allow_bitmap[2 * PAGE_SIZE];
>> +    bool allow_map_intialized;
> 
> typo allow_map_intialized -> allow_map_initialized
> 

Noted. Will change it here and at other places. Thanks.

>> +    /*
>> +     * Used to protect hvcall setup in IOCTLs
>> +     */
>> +    struct mutex init_mutex;
>> +    struct miscdevice *dev;
>> +};
>> +
>> +struct mshv_vtl_poll_file {
>> +    struct file *file;
>> +    wait_queue_entry_t wait;
>> +    wait_queue_head_t *wqh;
>> +    poll_table pt;
>> +    int cpu;
>> +};
>> +
> [clip]
>> +
>> +static int mshv_vtl_hvcall_setup(struct mshv_vtl_hvcall_fd *fd,
>> +                 struct mshv_vtl_hvcall_setup __user *hvcall_setup_user)
>> +{
>> +    int ret = 0;
>> +    struct mshv_vtl_hvcall_setup hvcall_setup;
>> +
>> +    mutex_lock(&fd->init_mutex);
>> +
>> +    if (fd->allow_map_intialized) {
>> +        dev_err(fd->dev->this_device,
>> +            "Hypercall allow map has already been set, pid %d\n",
>> +            current->pid);
>> +        ret = -EINVAL;
>> +        goto exit;
>> +    }
>> +
>> +    if (copy_from_user(&hvcall_setup, hvcall_setup_user,
>> +               sizeof(struct mshv_vtl_hvcall_setup))) {
>> +        ret = -EFAULT;
>> +        goto exit;
>> +    }
>> +    if (hvcall_setup.bitmap_size > ARRAY_SIZE(fd->allow_bitmap)) {
>> +        ret = -EINVAL;
>> +        goto exit;
>> +    }
> 
> is this valid case if hvcall_setup.bitmap_size == 0 ?

If bitmap_size is 0, then nothing will be copied in copy_from_user()
to .allow_bitmap and then mshv_vtl_hvcall_is_allowed() will start
returning 0/false for further hvcalls . This sounds reasonable
to me.

> 
>> +    if (copy_from_user(&fd->allow_bitmap,
>> +               (void __user *)hvcall_setup.allow_bitmap_ptr,
>> +               hvcall_setup.bitmap_size)) {
>> +        ret = -EFAULT;
>> +        goto exit;
>> +    }
>> +
> [clip]
> 
> 
> 
> Reviewed-by: Alok Tiwari <alok.a.tiwari@...cle.com>
> 
> Thanks,
> Alok

Thanks again for reviewing.

Regards,
Naman