[PATCH 1/2] drm/vmwgfx: avoid underflow when stripping short GuestInfo replies

[PATCH 1/2] drm/vmwgfx: avoid underflow when stripping short GuestInfo replies

[Zack Rusin]

vmw_host_get_guestinfo subtracts the two-byte reply prefix using
max(reply_len - 2, (size_t)0). Since reply_len is unsigned, a one-byte
reply wraps to SIZE_MAX instead of being clamped to zero. The subsequent
copy can read past the host reply allocation.

Theoretically malformed host reply can reach this path when the driver
queries the GuestInfo parameter used to enable VKMS.

Check the reply length before subtracting the prefix so short replies
produce an empty payload and cannot cause an out-of-bounds read.

Sashiko reported this while reviewing the i386 hypercall changes.

Fixes: 89da76fde68d ("drm/vmwgfx: Add VMWare host messaging capability")
Cc: stable@vger.kernel.org
Closes: https://sashiko.dev/#/patchset/4ab10678a8f50053934b62b56af81f881c1d3798.1788414669.git.zack.rusin%40broadcom.com
Signed-off-by: Zack Rusin <zack.rusin@broadcom.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_msg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c b/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
index fd77ab6568cb..49e04a50c6ed 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
@@ -431,7 +431,7 @@ int vmw_host_get_guestinfo(const char *guest_info_param,
 		/* Remove reply code, which are the first 2 characters of
 		 * the reply
 		 */
-		reply_len = max(reply_len - 2, (size_t) 0);
+		reply_len = reply_len > 2 ? reply_len - 2 : 0;
 		reply_len = min(reply_len, *length);
 
 		if (reply_len > 0)
-- 
2.53.0

[PATCH 2/2] drm/vmwgfx: bound MKSSTAT lengths before rounding to pages

[Zack Rusin]

vmw_mksstat_add_ioctl rounds the user-supplied 64bit lengths to page
counts before comparing them with the descriptors page arrays. PFN_UP
can wrap for lengths near U64_MAX. On 32bit kernels, storing a large page
count in size_t can also truncate it to a value that passes the checks.

A render-node client can therefore register a descriptor with lengths
that exceed its pinned page arrays, including nonzero lengths with no
backing pages.

Compare each byte length with its page array's capacity before rounding
or narrowing it. Compute those capacities in 64 bits and calculate the
page counts only after all three lengths have passed validation.

Sashiko reported this while reviewing the i386 hypercall changes.

Fixes: 7a7a933edd6c ("drm/vmwgfx: Introduce VMware mks-guest-stats")
Cc: stable@vger.kernel.org
Closes: https://sashiko.dev/#/patchset/4ab10678a8f50053934b62b56af81f881c1d3798.1788414669.git.zack.rusin%40broadcom.com
Signed-off-by: Zack Rusin <zack.rusin@broadcom.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_msg.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c b/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
index 49e04a50c6ed..58da332486bd 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
@@ -939,9 +939,12 @@ int vmw_mksstat_add_ioctl(struct drm_device *dev, void *data,
 
 	struct vmw_private *const dev_priv = vmw_priv(dev);
 
-	const size_t num_pages_stat = PFN_UP(arg->stat_len);
-	const size_t num_pages_info = PFN_UP(arg->info_len);
-	const size_t num_pages_strs = PFN_UP(arg->strs_len);
+	const u64 max_stat_len = (u64)MKS_GUEST_STAT_INSTANCE_MAX_STAT_PPNS * PAGE_SIZE;
+	const u64 max_info_len = (u64)MKS_GUEST_STAT_INSTANCE_MAX_INFO_PPNS * PAGE_SIZE;
+	const u64 max_strs_len = (u64)MKS_GUEST_STAT_INSTANCE_MAX_STRS_PPNS * PAGE_SIZE;
+	size_t num_pages_stat;
+	size_t num_pages_info;
+	size_t num_pages_strs;
 	long desc_len;
 	long nr_pinned_stat;
 	long nr_pinned_info;
@@ -965,11 +968,15 @@ int vmw_mksstat_add_ioctl(struct drm_device *dev, void *data,
 	if (!arg->description)
 		return -EINVAL;
 
-	if (num_pages_stat > ARRAY_SIZE(pdesc->statPPNs) ||
-		num_pages_info > ARRAY_SIZE(pdesc->infoPPNs) ||
-		num_pages_strs > ARRAY_SIZE(pdesc->strsPPNs))
+	if (arg->stat_len > max_stat_len ||
+	    arg->info_len > max_info_len ||
+	    arg->strs_len > max_strs_len)
 		return -EINVAL;
 
+	num_pages_stat = PFN_UP(arg->stat_len);
+	num_pages_info = PFN_UP(arg->info_len);
+	num_pages_strs = PFN_UP(arg->strs_len);
+
 	/* Find an available slot in the mksGuestStats user array and reserve it */
 	for (slot = 0; slot < ARRAY_SIZE(dev_priv->mksstat_user_pids); ++slot)
 		if (!atomic_cmpxchg(&dev_priv->mksstat_user_pids[slot], 0, MKSSTAT_PID_RESERVED))
-- 
2.53.0

Re: [PATCH 2/2] drm/vmwgfx: bound MKSSTAT lengths before rounding to pages

[Maaz Mombasawala <maaz.mombasawala@broadcom.com>]

LGTM!

Reviewed-by: Maaz Mombasawala <maaz.mombasawala@broadcom.com>

-- 
Maaz Mombasawala <maaz.mombasawala@broadcom.com> 

Re: [PATCH 1/2] drm/vmwgfx: avoid underflow when stripping short GuestInfo replies

[Maaz Mombasawala <maaz.mombasawala@broadcom.com>]

LGTM!

Reviewed-by: Maaz Mombasawala <maaz.mombasawala@broadcom.com>

-- 
Maaz Mombasawala <maaz.mombasawala@broadcom.com>