Re: [PATCH] Drivers: hv: Allow single instance of hv_util devices

[Sonia Sharma <sosha@linux.microsoft.com>]

On Tue, Jan 7, 2025 at 02:00:48PM +0000, Wei Liu wrote:
>   
> On Sun, Dec 29, 2024 at 06:02:34PM +0000, Michael Kelley wrote:
>> From: Sonia Sharma <sosha@linux.microsoft.com> Sent: Friday, December 20, 2024 3:56 PM
>> > 
>> 
>> Please include the "linux-hyperv@vger.kernel.org" mailing list
>> when submitting patches related to Hyper-V.
>> 
>> > Harden hv_util type device drivers to allow single
>> > instance of the device be configured at given time.
>> >
> 
> Why is this needed? What's the problem that this patch is trying to
> solve?
> 
>> 
>> I think the reason for this patch needs more explanation. For several
>> VMBus devices, a well-behaved Hyper-V is expected to offer only one
>> instance of the device in a given VM. Linux guests originally assumed
>> that the Hyper-V host is well-behaved, so the device drivers for many
>> of these devices were written assuming only a single instance. But
>> with the introduction of Confidential Computing (CoCo) VMs, Hyper-V
>> is no longer assumed to be well-behaved. If a compromised & malicious
>> Hyper-V were to offer multiple instances of such a device, the device
>> driver assumption about a single instance would be false, and
>> memory corruption could occur, which has the potential to lead to
>> compromise of the CoCo VM. The intent is to prevent such a scenario.
>> 
>> Note that this problem extends beyond just "util" devices. Hyper-V
>> is expected to offer only a single instance of keyboard, mouse, frame
>> buffer, and balloon devices as well. So this patch should be extended
>> to include them as well (and your new function names containing
>> "hv_util" should be adjusted). Interestingly, the Hyper-V keyboard driver
>> does not assume a single instance, so it should be safe regardless. But
>> the mouse, frame buffer, and balloon drivers are not safe.
>> 
>> With this understanding, there are two ways to approach the problem:
>> 
>> 1) Enforce the expectation that a well-behaved Hyper-V only offers a
>> single instance of these VMBus devices. That's the approach that this
>> patch takes.
>> 
>> 2) Update the device drivers to remove the assumption of a single
>> instance. With this approach, if a compromised & malicious Hyper-V
>> were to offer multiple instances, the extra devices might be bogus, 
>> but memory corruption would not occur and the integrity of the
>> CoCo VM should not be compromised. As mentioned above, such
>> is already the case with the keyboard driver.
>> 
>> I've thought about the tradeoffs for the two approaches, and don't
>> really have a strong opinion either way. In some sense, #2 is the
>> more correct approach as ideally device drivers shouldn't make
>> single instance assumptions. But #1 is an easier fix, and perhaps
>> more robust. Other reviewers might have other reasons to prefer
>> one over the other, and have a stronger viewpoint on the tradeoffs.
>> I would be interested in any such comments. But I'm OK with
>> approach #1 unless someone points out a good reason to
>> prefer #2.
> 
> #2 is preferred. It is frowned upon to make assumptions that only one
> instance of a device will be present.
> 
> It perhaps takes more work to check and enforce the invariant (as this
> patch demonstrates) than to just let the device framework handle
> multiple instances.
> 
> Thanks,
> Wei.

Thanks Michael and Wei for the review.

The intent of the patch is correctly described by Michael. With that, it seems the consensus is to go with approach #2, so I would then work on a new patch series fixing the assumption of singleton driver wherever needed.

Thank you,
Sonia