Re: [PATCH v4 16/18] mshv: Validate scheduler message bounds from hypervisor

[Anirudh Rayabharam <anirudh@anirudhrb.com>]

On Wed, May 13, 2026 at 10:39:31AM -0700, Stanislav Kinsburskii wrote:
> On Wed, May 13, 2026 at 11:12:05AM +0000, Anirudh Rayabharam wrote:
> > On Thu, May 07, 2026 at 03:44:26PM +0000, Stanislav Kinsburskii wrote:
> > > handle_pair_message() iterates up to msg->vp_count without verifying it
> > > against HV_MESSAGE_MAX_PARTITION_VP_PAIR_COUNT. Since vp_count is read
> > > from untrusted hypervisor data, a malformed message with a large value
> > > would cause out-of-bounds reads from the partition_ids and vp_indexes
> > > arrays.
> > > 
> > > handle_bitset_message() iterates over set bits in valid_bank_mask (up to
> > > 64) and advances bank_contents for each one. However, the payload buffer
> > > only has space for 16 bank entries. A valid_bank_mask with more than 16
> > > bits set causes bank_contents to read beyond the message buffer.
> > > 
> > > Fix both by adding bounds validation:
> > > - Clamp vp_count to HV_MESSAGE_MAX_PARTITION_VP_PAIR_COUNT
> > > - Track banks consumed and stop before exceeding buffer capacity
> > > 
> > > Fixes: 621191d709b1 ("Drivers: hv: Introduce mshv_root module to expose /dev/mshv to VMMs")
> > > Signed-off-by: Stanislav Kinsburskii <skinsburskii@linux.microsoft.com>
> > > ---
> > >  drivers/hv/mshv_synic.c |   20 ++++++++++++++++++--
> > >  1 file changed, 18 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/hv/mshv_synic.c b/drivers/hv/mshv_synic.c
> > > index 89207aad7cf1f..5d509299f14d7 100644
> > > --- a/drivers/hv/mshv_synic.c
> > > +++ b/drivers/hv/mshv_synic.c
> > > @@ -190,7 +190,9 @@ static void kick_vp(struct mshv_vp *vp)
> > >  static void
> > >  handle_bitset_message(const struct hv_vp_signal_bitset_scheduler_message *msg)
> > >  {
> > > -	int bank_idx, vps_signaled = 0, bank_mask_size;
> > > +	int bank_idx, vps_signaled = 0, bank_mask_size, banks_used = 0;
> > > +	const int max_banks = sizeof(msg->vp_bitset.bitset_buffer) /
> > > +			      sizeof(u64) - 2; /* subtract format + mask */
> > 
> > Could this be a constant in the header?
> > 
> 
> Yes, it could. But it the only place it's used and it's pretty
> self-explanatory, so I don't think it needs to be.

The "subtract format+mask" part is a bit concerning. We might forget to update
this code if the struct layout ever changes. Whereas if the constant is
right next to the definition in the header, it is unlikely to be missed.

Thanks,
Anirudh.