diff mbox series

[Resend,RFC,V2,08/12] UIO/Hyper-V: Not load UIO HV driver in the isolation VM.

Message ID 20210414144945.3460554-9-ltykernel@gmail.com (mailing list archive)
State New
Headers show
Series x86/Hyper-V: Add Hyper-V Isolation VM support | expand

Commit Message

Tianyu Lan April 14, 2021, 2:49 p.m. UTC
From: Tianyu Lan <Tianyu.Lan@microsoft.com>

UIO HV driver should not load in the isolation VM for security reason.
Return ENOTSUPP in the hv_uio_probe() in the isolation VM.

Signed-off-by: Tianyu Lan <Tianyu.Lan@microsoft.com>
---
 drivers/uio/uio_hv_generic.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Christoph Hellwig April 14, 2021, 3:42 p.m. UTC | #1
On Wed, Apr 14, 2021 at 10:49:41AM -0400, Tianyu Lan wrote:
> From: Tianyu Lan <Tianyu.Lan@microsoft.com>
> 
> UIO HV driver should not load in the isolation VM for security reason.
> Return ENOTSUPP in the hv_uio_probe() in the isolation VM.

What is the "security reason"?  After all a user can simply patch the
code and just load it anyway..
Greg KH April 14, 2021, 3:45 p.m. UTC | #2
On Wed, Apr 14, 2021 at 10:49:41AM -0400, Tianyu Lan wrote:
> From: Tianyu Lan <Tianyu.Lan@microsoft.com>
> 
> UIO HV driver should not load in the isolation VM for security reason.
> Return ENOTSUPP in the hv_uio_probe() in the isolation VM.
> 
> Signed-off-by: Tianyu Lan <Tianyu.Lan@microsoft.com>
> ---
>  drivers/uio/uio_hv_generic.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/uio/uio_hv_generic.c b/drivers/uio/uio_hv_generic.c
> index 0330ba99730e..678b021d66f8 100644
> --- a/drivers/uio/uio_hv_generic.c
> +++ b/drivers/uio/uio_hv_generic.c
> @@ -29,6 +29,7 @@
>  #include <linux/hyperv.h>
>  #include <linux/vmalloc.h>
>  #include <linux/slab.h>
> +#include <asm/mshyperv.h>
>  
>  #include "../hv/hyperv_vmbus.h"
>  
> @@ -241,6 +242,10 @@ hv_uio_probe(struct hv_device *dev,
>  	void *ring_buffer;
>  	int ret;
>  
> +	/* UIO driver should not be loaded in the isolation VM.*/
> +	if (hv_is_isolation_supported())
> +		return -ENOTSUPP;
> +		
>  	/* Communicating with host has to be via shared memory not hypercall */
>  	if (!channel->offermsg.monitor_allocated) {
>  		dev_err(&dev->device, "vmbus channel requires hypercall\n");
> -- 
> 2.25.1
> 

Again you send out known-wrong patches?

:(
Stephen Hemminger April 14, 2021, 4:17 p.m. UTC | #3
On Wed, 14 Apr 2021 17:45:51 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Wed, Apr 14, 2021 at 10:49:41AM -0400, Tianyu Lan wrote:
> > From: Tianyu Lan <Tianyu.Lan@microsoft.com>
> > 
> > UIO HV driver should not load in the isolation VM for security reason.
> > Return ENOTSUPP in the hv_uio_probe() in the isolation VM.
> > 
> > Signed-off-by: Tianyu Lan <Tianyu.Lan@microsoft.com>

This is debatable, in isolation VM's shouldn't userspace take responsibility
to validate host communication. If that is an issue please participate with
the DPDK community (main user of this) to make sure netvsc userspace driver
has the required checks.
Tianyu Lan April 15, 2021, 12:54 p.m. UTC | #4
Hi Stephen:
	Thanks for your review.


On 4/15/2021 12:17 AM, Stephen Hemminger wrote:
> On Wed, 14 Apr 2021 17:45:51 +0200
> Greg KH <gregkh@linuxfoundation.org> wrote:
> 
>> On Wed, Apr 14, 2021 at 10:49:41AM -0400, Tianyu Lan wrote:
>>> From: Tianyu Lan <Tianyu.Lan@microsoft.com>
>>>
>>> UIO HV driver should not load in the isolation VM for security reason.
>>> Return ENOTSUPP in the hv_uio_probe() in the isolation VM.
>>>
>>> Signed-off-by: Tianyu Lan <Tianyu.Lan@microsoft.com>
> 
> This is debatable, in isolation VM's shouldn't userspace take responsibility
> to validate host communication. If that is an issue please participate with
> the DPDK community (main user of this) to make sure netvsc userspace driver
> has the required checks.
> 

Agree. Will report back to secure team and apply request to add change 
in userspace netvsc driver. Thanks for advise.
Tianyu Lan April 15, 2021, 1:09 p.m. UTC | #5
On 4/14/2021 11:45 PM, Greg KH wrote:
> On Wed, Apr 14, 2021 at 10:49:41AM -0400, Tianyu Lan wrote:
>> From: Tianyu Lan <Tianyu.Lan@microsoft.com>
>>
>> UIO HV driver should not load in the isolation VM for security reason.
>> Return ENOTSUPP in the hv_uio_probe() in the isolation VM.
>>
>> Signed-off-by: Tianyu Lan <Tianyu.Lan@microsoft.com>
>> ---
>>   drivers/uio/uio_hv_generic.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/uio/uio_hv_generic.c b/drivers/uio/uio_hv_generic.c
>> index 0330ba99730e..678b021d66f8 100644
>> --- a/drivers/uio/uio_hv_generic.c
>> +++ b/drivers/uio/uio_hv_generic.c
>> @@ -29,6 +29,7 @@
>>   #include <linux/hyperv.h>
>>   #include <linux/vmalloc.h>
>>   #include <linux/slab.h>
>> +#include <asm/mshyperv.h>
>>   
>>   #include "../hv/hyperv_vmbus.h"
>>   
>> @@ -241,6 +242,10 @@ hv_uio_probe(struct hv_device *dev,
>>   	void *ring_buffer;
>>   	int ret;
>>   
>> +	/* UIO driver should not be loaded in the isolation VM.*/
>> +	if (hv_is_isolation_supported())
>> +		return -ENOTSUPP;
>> +		
>>   	/* Communicating with host has to be via shared memory not hypercall */
>>   	if (!channel->offermsg.monitor_allocated) {
>>   		dev_err(&dev->device, "vmbus channel requires hypercall\n");
>> -- 
>> 2.25.1
>>
> 
> Again you send out known-wrong patches?
> 
> :(
> 
Sorry for noise. Will fix this next version and I think we should make 
sure user space driver to check data from host. This patch will be removed.
diff mbox series

Patch

diff --git a/drivers/uio/uio_hv_generic.c b/drivers/uio/uio_hv_generic.c
index 0330ba99730e..678b021d66f8 100644
--- a/drivers/uio/uio_hv_generic.c
+++ b/drivers/uio/uio_hv_generic.c
@@ -29,6 +29,7 @@ 
 #include <linux/hyperv.h>
 #include <linux/vmalloc.h>
 #include <linux/slab.h>
+#include <asm/mshyperv.h>
 
 #include "../hv/hyperv_vmbus.h"
 
@@ -241,6 +242,10 @@  hv_uio_probe(struct hv_device *dev,
 	void *ring_buffer;
 	int ret;
 
+	/* UIO driver should not be loaded in the isolation VM.*/
+	if (hv_is_isolation_supported())
+		return -ENOTSUPP;
+		
 	/* Communicating with host has to be via shared memory not hypercall */
 	if (!channel->offermsg.monitor_allocated) {
 		dev_err(&dev->device, "vmbus channel requires hypercall\n");