| 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
| ||
|
Message-ID: <47b4dac7-b4b8-2fac-8271-9adcd1b19b70@gmail.com>
Date: Fri, 5 Mar 2021 14:23:31 +0800
From: Tianyu Lan <ltykernel@...il.com>
To: Vitaly Kuznetsov <vkuznets@...hat.com>
Cc: Tianyu Lan <Tianyu.Lan@...rosoft.com>,
linux-hyperv@...r.kernel.org, linux-kernel@...r.kernel.org,
netdev@...r.kernel.org, linux-arch@...r.kernel.org,
thomas.lendacky@....com, brijesh.singh@....com,
sunilmut@...rosoft.com, kys@...rosoft.com, haiyangz@...rosoft.com,
sthemmin@...rosoft.com, wei.liu@...nel.org, tglx@...utronix.de,
mingo@...hat.com, bp@...en8.de, x86@...nel.org, hpa@...or.com,
davem@...emloft.net, kuba@...nel.org, gregkh@...uxfoundation.org,
arnd@...db.de
Subject: Re: [RFC PATCH 2/12] x86/Hyper-V: Add new hvcall guest address host
visibility support
On 3/4/2021 12:58 AM, Vitaly Kuznetsov wrote:
> Tianyu Lan <ltykernel@...il.com> writes:
>
>> From: Tianyu Lan <Tianyu.Lan@...rosoft.com>
>>
>> Add new hvcall guest address host visibility support. Mark vmbus
>> ring buffer visible to host when create gpadl buffer and mark back
>> to not visible when tear down gpadl buffer.
>>
>> Signed-off-by: Sunil Muthuswamy <sunilmut@...rosoft.com>
>> Co-Developed-by: Sunil Muthuswamy <sunilmut@...rosoft.com>
>> Signed-off-by: Tianyu Lan <Tianyu.Lan@...rosoft.com>
>> ---
>> arch/x86/include/asm/hyperv-tlfs.h | 13 ++++++++
>> arch/x86/include/asm/mshyperv.h | 4 +--
>> arch/x86/kernel/cpu/mshyperv.c | 46 ++++++++++++++++++++++++++
>> drivers/hv/channel.c | 53 ++++++++++++++++++++++++++++--
>> drivers/net/hyperv/hyperv_net.h | 1 +
>> drivers/net/hyperv/netvsc.c | 9 +++--
>> drivers/uio/uio_hv_generic.c | 6 ++--
>> include/asm-generic/hyperv-tlfs.h | 1 +
>> include/linux/hyperv.h | 3 +-
>> 9 files changed, 126 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h
>> index fb1893a4c32b..d22b1c3f425a 100644
>> --- a/arch/x86/include/asm/hyperv-tlfs.h
>> +++ b/arch/x86/include/asm/hyperv-tlfs.h
>> @@ -573,4 +573,17 @@ enum hv_interrupt_type {
>>
>> #include <asm-generic/hyperv-tlfs.h>
>>
>> +/* All input parameters should be in single page. */
>> +#define HV_MAX_MODIFY_GPA_REP_COUNT \
>> + ((PAGE_SIZE - 2 * sizeof(u64)) / (sizeof(u64)))
>
> Would it be easier to express this as '((PAGE_SIZE / sizeof(u64)) - 2'
Yes, will update. Thanks.
>> +
>> +/* HvCallModifySparseGpaPageHostVisibility hypercall */
>> +struct hv_input_modify_sparse_gpa_page_host_visibility {
>> + u64 partition_id;
>> + u32 host_visibility:2;
>> + u32 reserved0:30;
>> + u32 reserved1;
>> + u64 gpa_page_list[HV_MAX_MODIFY_GPA_REP_COUNT];
>> +} __packed;
>> +
>> #endif
>> diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
>> index ccf60a809a17..1e8275d35c1f 100644
>> --- a/arch/x86/include/asm/mshyperv.h
>> +++ b/arch/x86/include/asm/mshyperv.h
>> @@ -262,13 +262,13 @@ static inline void hv_set_msi_entry_from_desc(union hv_msi_entry *msi_entry,
>> msi_entry->address.as_uint32 = msi_desc->msg.address_lo;
>> msi_entry->data.as_uint32 = msi_desc->msg.data;
>> }
>> -
>
> stray change
>
>> struct irq_domain *hv_create_pci_msi_domain(void);
>>
>> int hv_map_ioapic_interrupt(int ioapic_id, bool level, int vcpu, int vector,
>> struct hv_interrupt_entry *entry);
>> int hv_unmap_ioapic_interrupt(int ioapic_id, struct hv_interrupt_entry *entry);
>> -
>> +int hv_set_mem_host_visibility(void *kbuffer, u32 size, u32 visibility);
>> +int hv_mark_gpa_visibility(u16 count, const u64 pfn[], u32 visibility);
>> #else /* CONFIG_HYPERV */
>> static inline void hyperv_init(void) {}
>> static inline void hyperv_setup_mmu_ops(void) {}
>> diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
>> index e88bc296afca..347c32eac8fd 100644
>> --- a/arch/x86/kernel/cpu/mshyperv.c
>> +++ b/arch/x86/kernel/cpu/mshyperv.c
>> @@ -37,6 +37,8 @@
>> bool hv_root_partition;
>> EXPORT_SYMBOL_GPL(hv_root_partition);
>>
>> +#define HV_PARTITION_ID_SELF ((u64)-1)
>> +
>
> We seem to have this already:
>
> include/asm-generic/hyperv-tlfs.h:#define HV_PARTITION_ID_SELF ((u64)-1)
>> struct ms_hyperv_info ms_hyperv;
>> EXPORT_SYMBOL_GPL(ms_hyperv);
>>
>> @@ -477,3 +479,47 @@ const __initconst struct hypervisor_x86 x86_hyper_ms_hyperv = {
>> .init.msi_ext_dest_id = ms_hyperv_msi_ext_dest_id,
>> .init.init_platform = ms_hyperv_init_platform,
>> };
>> +
>> +int hv_mark_gpa_visibility(u16 count, const u64 pfn[], u32 visibility)
>> +{
>> + struct hv_input_modify_sparse_gpa_page_host_visibility **input_pcpu;
>> + struct hv_input_modify_sparse_gpa_page_host_visibility *input;
>> + u16 pages_processed;
>> + u64 hv_status;
>> + unsigned long flags;
>> +
>> + /* no-op if partition isolation is not enabled */
>> + if (!hv_is_isolation_supported())
>> + return 0;
>> +
>> + if (count > HV_MAX_MODIFY_GPA_REP_COUNT) {
>> + pr_err("Hyper-V: GPA count:%d exceeds supported:%lu\n", count,
>> + HV_MAX_MODIFY_GPA_REP_COUNT);
>> + return -EINVAL;
>> + }
>> +
>> + local_irq_save(flags);
>> + input_pcpu = (struct hv_input_modify_sparse_gpa_page_host_visibility **)
>> + this_cpu_ptr(hyperv_pcpu_input_arg);
>> + input = *input_pcpu;
>> + if (unlikely(!input)) {
>> + local_irq_restore(flags);
>> + return -1;
>
> -EFAULT/-ENOMEM/... maybe ?
Yes, will update.
>
>> + }
>> +
>> + input->partition_id = HV_PARTITION_ID_SELF;
>> + input->host_visibility = visibility;
>> + input->reserved0 = 0;
>> + input->reserved1 = 0;
>> + memcpy((void *)input->gpa_page_list, pfn, count * sizeof(*pfn));
>> + hv_status = hv_do_rep_hypercall(
>> + HVCALL_MODIFY_SPARSE_GPA_PAGE_HOST_VISIBILITY, count,
>> + 0, input, &pages_processed);
>> + local_irq_restore(flags);
>> +
>> + if (!(hv_status & HV_HYPERCALL_RESULT_MASK))
>> + return 0;
>> +
>> + return -EFAULT;
>
> Could we just propagate "hv_status & HV_HYPERCALL_RESULT_MASK" maybe?
Yes. will update.
>
>> +}
>> +EXPORT_SYMBOL(hv_mark_gpa_visibility);
>> diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
>> index daa21cc72beb..204e6f3598a5 100644
>> --- a/drivers/hv/channel.c
>> +++ b/drivers/hv/channel.c
>> @@ -237,6 +237,38 @@ int vmbus_send_modifychannel(u32 child_relid, u32 target_vp)
>> }
>> EXPORT_SYMBOL_GPL(vmbus_send_modifychannel);
>>
>> +/*
>> + * hv_set_mem_host_visibility - Set host visibility for specified memory.
>> + */
>> +int hv_set_mem_host_visibility(void *kbuffer, u32 size, u32 visibility)
>> +{
>> + int i, pfn;
>> + int pagecount = size >> HV_HYP_PAGE_SHIFT;
>> + u64 *pfn_array;
>> + int ret = 0;
>> +
>> + if (!hv_isolation_type_snp())
>> + return 0;
>> +
>> + pfn_array = vzalloc(HV_HYP_PAGE_SIZE);
>> + if (!pfn_array)
>> + return -ENOMEM;
>> +
>> + for (i = 0, pfn = 0; i < pagecount; i++) {
>> + pfn_array[pfn] = virt_to_hvpfn(kbuffer + i * HV_HYP_PAGE_SIZE);
>> + pfn++;
>> +
>> + if (pfn == HV_MAX_MODIFY_GPA_REP_COUNT || i == pagecount - 1) {
>> + ret |= hv_mark_gpa_visibility(pfn, pfn_array, visibility);
>> + pfn = 0;
>
> hv_mark_gpa_visibility() return different error codes and aggregating
> them with
>
> ret |= ...
>
> will have an unpredictable result. I'd suggest bail immediately instead:
>
> if (ret)
> goto err_free_pfn_array;
Yes, this makes sense. Thanks.
>
>> + }
>> + }
>> +
>
> err_free_pfn_array:
>
>> + vfree(pfn_array);
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(hv_set_mem_host_visibility);
>> +
>> /*
>> * create_gpadl_header - Creates a gpadl for the specified buffer
>> */
>> @@ -410,6 +442,12 @@ static int __vmbus_establish_gpadl(struct vmbus_channel *channel,
>> if (ret)
>> return ret;
>>
>> + ret = hv_set_mem_host_visibility(kbuffer, size, visibility);
>> + if (ret) {
>> + pr_warn("Failed to set host visibility.\n");
>> + return ret;
>> + }
>> +
>> init_completion(&msginfo->waitevent);
>> msginfo->waiting_channel = channel;
>>
>> @@ -693,7 +731,9 @@ static int __vmbus_open(struct vmbus_channel *newchannel,
>> error_free_info:
>> kfree(open_info);
>> error_free_gpadl:
>> - vmbus_teardown_gpadl(newchannel, newchannel->ringbuffer_gpadlhandle);
>> + vmbus_teardown_gpadl(newchannel, newchannel->ringbuffer_gpadlhandle,
>> + page_address(newchannel->ringbuffer_page),
>> + newchannel->ringbuffer_pagecount << PAGE_SHIFT);
>
> Instead of modifying vmbus_teardown_gpadl() interface and all its call
> sites, could we just keep track of all established gpadls and then get
> the required data from there? I.e. make vmbus_establish_gpadl() save
> kbuffer/size to some internal structure associated with 'gpadl_handle'.
>
Yes, that's another approach. Add an array or list in struct vmbus_channel.
>> newchannel->ringbuffer_gpadlhandle = 0;
>> error_clean_ring:
>> hv_ringbuffer_cleanup(&newchannel->outbound);
>> @@ -740,7 +780,8 @@ EXPORT_SYMBOL_GPL(vmbus_open);
>> /*
>> * vmbus_teardown_gpadl -Teardown the specified GPADL handle
>> */
>> -int vmbus_teardown_gpadl(struct vmbus_channel *channel, u32 gpadl_handle)
>> +int vmbus_teardown_gpadl(struct vmbus_channel *channel, u32 gpadl_handle,
>> + void *kbuffer, u32 size)
>
> This probably doesn't matter but why not 'u64 size'?
>
>> {
>> struct vmbus_channel_gpadl_teardown *msg;
>> struct vmbus_channel_msginfo *info;
>> @@ -793,6 +834,10 @@ int vmbus_teardown_gpadl(struct vmbus_channel *channel, u32 gpadl_handle)
>> spin_unlock_irqrestore(&vmbus_connection.channelmsg_lock, flags);
>>
>> kfree(info);
>> +
>> + if (hv_set_mem_host_visibility(kbuffer, size, VMBUS_PAGE_NOT_VISIBLE))
>> + pr_warn("Fail to set mem host visibility.\n");
>
> pr_err() maybe?
>
Yes, will update.
>> +
>> return ret;
>> }
>> EXPORT_SYMBOL_GPL(vmbus_teardown_gpadl);
>> @@ -869,7 +914,9 @@ static int vmbus_close_internal(struct vmbus_channel *channel)
>> /* Tear down the gpadl for the channel's ring buffer */
>> else if (channel->ringbuffer_gpadlhandle) {
>> ret = vmbus_teardown_gpadl(channel,
>> - channel->ringbuffer_gpadlhandle);
>> + channel->ringbuffer_gpadlhandle,
>> + page_address(channel->ringbuffer_page),
>> + channel->ringbuffer_pagecount << PAGE_SHIFT);
>> if (ret) {
>> pr_err("Close failed: teardown gpadl return %d\n", ret);
>> /*
>> diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
>> index 2a87cfa27ac0..b3a43c4ec8ab 100644
>> --- a/drivers/net/hyperv/hyperv_net.h
>> +++ b/drivers/net/hyperv/hyperv_net.h
>> @@ -1034,6 +1034,7 @@ struct netvsc_device {
>>
>> /* Send buffer allocated by us */
>> void *send_buf;
>> + u32 send_buf_size;
>> u32 send_buf_gpadl_handle;
>> u32 send_section_cnt;
>> u32 send_section_size;
>> diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
>> index bb72c7578330..08d73401bb28 100644
>> --- a/drivers/net/hyperv/netvsc.c
>> +++ b/drivers/net/hyperv/netvsc.c
>> @@ -245,7 +245,9 @@ static void netvsc_teardown_recv_gpadl(struct hv_device *device,
>>
>> if (net_device->recv_buf_gpadl_handle) {
>> ret = vmbus_teardown_gpadl(device->channel,
>> - net_device->recv_buf_gpadl_handle);
>> + net_device->recv_buf_gpadl_handle,
>> + net_device->recv_buf,
>> + net_device->recv_buf_size);
>>
>> /* If we failed here, we might as well return and have a leak
>> * rather than continue and a bugchk
>> @@ -267,7 +269,9 @@ static void netvsc_teardown_send_gpadl(struct hv_device *device,
>>
>> if (net_device->send_buf_gpadl_handle) {
>> ret = vmbus_teardown_gpadl(device->channel,
>> - net_device->send_buf_gpadl_handle);
>> + net_device->send_buf_gpadl_handle,
>> + net_device->send_buf,
>> + net_device->send_buf_size);
>>
>> /* If we failed here, we might as well return and have a leak
>> * rather than continue and a bugchk
>> @@ -419,6 +423,7 @@ static int netvsc_init_buf(struct hv_device *device,
>> ret = -ENOMEM;
>> goto cleanup;
>> }
>> + net_device->send_buf_size = buf_size;
>>
>> /* Establish the gpadl handle for this buffer on this
>> * channel. Note: This call uses the vmbus connection rather
>> diff --git a/drivers/uio/uio_hv_generic.c b/drivers/uio/uio_hv_generic.c
>> index 813a7bee5139..c8d4704fc90c 100644
>> --- a/drivers/uio/uio_hv_generic.c
>> +++ b/drivers/uio/uio_hv_generic.c
>> @@ -181,13 +181,15 @@ static void
>> hv_uio_cleanup(struct hv_device *dev, struct hv_uio_private_data *pdata)
>> {
>> if (pdata->send_gpadl) {
>> - vmbus_teardown_gpadl(dev->channel, pdata->send_gpadl);
>> + vmbus_teardown_gpadl(dev->channel, pdata->send_gpadl,
>> + pdata->send_buf, SEND_BUFFER_SIZE);
>> pdata->send_gpadl = 0;
>> vfree(pdata->send_buf);
>> }
>>
>> if (pdata->recv_gpadl) {
>> - vmbus_teardown_gpadl(dev->channel, pdata->recv_gpadl);
>> + vmbus_teardown_gpadl(dev->channel, pdata->recv_gpadl,
>> + pdata->recv_buf, RECV_BUFFER_SIZE);
>> pdata->recv_gpadl = 0;
>> vfree(pdata->recv_buf);
>> }
>> diff --git a/include/asm-generic/hyperv-tlfs.h b/include/asm-generic/hyperv-tlfs.h
>> index 83448e837ded..ad19f4199f90 100644
>> --- a/include/asm-generic/hyperv-tlfs.h
>> +++ b/include/asm-generic/hyperv-tlfs.h
>> @@ -158,6 +158,7 @@ struct ms_hyperv_tsc_page {
>> #define HVCALL_RETARGET_INTERRUPT 0x007e
>> #define HVCALL_FLUSH_GUEST_PHYSICAL_ADDRESS_SPACE 0x00af
>> #define HVCALL_FLUSH_GUEST_PHYSICAL_ADDRESS_LIST 0x00b0
>> +#define HVCALL_MODIFY_SPARSE_GPA_PAGE_HOST_VISIBILITY 0x00db
>>
>> #define HV_FLUSH_ALL_PROCESSORS BIT(0)
>> #define HV_FLUSH_ALL_VIRTUAL_ADDRESS_SPACES BIT(1)
>> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
>> index 016fdca20d6e..41cbaa2db567 100644
>> --- a/include/linux/hyperv.h
>> +++ b/include/linux/hyperv.h
>> @@ -1183,7 +1183,8 @@ extern int vmbus_establish_gpadl(struct vmbus_channel *channel,
>> u32 visibility);
>>
>> extern int vmbus_teardown_gpadl(struct vmbus_channel *channel,
>> - u32 gpadl_handle);
>> + u32 gpadl_handle,
>> + void *kbuffer, u32 size);
>>
>> void vmbus_reset_channel_cb(struct vmbus_channel *channel);
>
Powered by blists - more mailing lists