[PATCH 6.1] Drivers: hv: vmbus: Leak pages if set_memory_encrypted() fails

[PATCH 6.1] Drivers: hv: vmbus: Leak pages if set_memory_encrypted() fails

[Xiangyu Chen]

From: Rick Edgecombe <rick.p.edgecombe@intel.com>

In CoCo VMs it is possible for the untrusted host to cause
set_memory_encrypted() or set_memory_decrypted() to fail such that an
error is returned and the resulting memory is shared. Callers need to
take care to handle these errors to avoid returning decrypted (shared)
memory to the page allocator, which could lead to functional or security
issues.

VMBus code could free decrypted pages if set_memory_encrypted()/decrypted()
fails. Leak the pages if this happens.

Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
Signed-off-by: Michael Kelley <mhklinux@outlook.com>
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Link: https://lore.kernel.org/r/20240311161558.1310-2-mhklinux@outlook.com
Signed-off-by: Wei Liu <wei.liu@kernel.org>
Message-ID: <20240311161558.1310-2-mhklinux@outlook.com>
[Xiangyu: Modified to apply on 6.1.y]
Signed-off-by: Xiangyu Chen <xiangyu.chen@windriver.com>
---
 drivers/hv/connection.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
index da51b50787df..c74088b69a5f 100644
--- a/drivers/hv/connection.c
+++ b/drivers/hv/connection.c
@@ -243,8 +243,17 @@ int vmbus_connect(void)
 		ret |= set_memory_decrypted((unsigned long)
 					    vmbus_connection.monitor_pages[1],
 					    1);
-		if (ret)
+		if (ret) {
+			/*
+			 * If set_memory_decrypted() fails, the encryption state
+			 * of the memory is unknown. So leak the memory instead
+			 * of risking returning decrypted memory to the free list.
+			 * For simplicity, always handle both pages the same.
+			 */
+			vmbus_connection.monitor_pages[0] = NULL;
+			vmbus_connection.monitor_pages[1] = NULL;
 			goto cleanup;
+		}
 
 		/*
 		 * Isolation VM with AMD SNP needs to access monitor page via
-- 
2.43.0

[PATCH v2 6.1/6.6] wifi: mac80211: Avoid address calculations via out of bounds array indexing

[Xiangyu Chen]

From: Kenton Groombridge <concord@gentoo.org>

req->n_channels must be set before req->channels[] can be used.

This patch fixes one of the issues encountered in [1].

[   83.964255] UBSAN: array-index-out-of-bounds in net/mac80211/scan.c:364:4
[   83.964258] index 0 is out of range for type 'struct ieee80211_channel *[]'
[...]
[   83.964264] Call Trace:
[   83.964267]  <TASK>
[   83.964269]  dump_stack_lvl+0x3f/0xc0
[   83.964274]  __ubsan_handle_out_of_bounds+0xec/0x110
[   83.964278]  ieee80211_prep_hw_scan+0x2db/0x4b0
[   83.964281]  __ieee80211_start_scan+0x601/0x990
[   83.964291]  nl80211_trigger_scan+0x874/0x980
[   83.964295]  genl_family_rcv_msg_doit+0xe8/0x160
[   83.964298]  genl_rcv_msg+0x240/0x270
[...]

[1] https://bugzilla.kernel.org/show_bug.cgi?id=218810

Co-authored-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Kees Cook <kees@kernel.org>
Signed-off-by: Kenton Groombridge <concord@gentoo.org>
Link: https://msgid.link/20240605152218.236061-1-concord@gentoo.org
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
[Xiangyu: Modified to apply on 6.1.y and 6.6.y]
Signed-off-by: Xiangyu Chen <xiangyu.chen@windriver.com>
---
V1 -> V2:
add v6.6 support
---
 net/mac80211/scan.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
index 62c22ff329ad..d81b49fb6458 100644
--- a/net/mac80211/scan.c
+++ b/net/mac80211/scan.c
@@ -357,7 +357,8 @@ static bool ieee80211_prep_hw_scan(struct ieee80211_sub_if_data *sdata)
 	struct cfg80211_scan_request *req;
 	struct cfg80211_chan_def chandef;
 	u8 bands_used = 0;
-	int i, ielen, n_chans;
+	int i, ielen;
+	u32 *n_chans;
 	u32 flags = 0;
 
 	req = rcu_dereference_protected(local->scan_req,
@@ -367,34 +368,34 @@ static bool ieee80211_prep_hw_scan(struct ieee80211_sub_if_data *sdata)
 		return false;
 
 	if (ieee80211_hw_check(&local->hw, SINGLE_SCAN_ON_ALL_BANDS)) {
+		local->hw_scan_req->req.n_channels = req->n_channels;
+
 		for (i = 0; i < req->n_channels; i++) {
 			local->hw_scan_req->req.channels[i] = req->channels[i];
 			bands_used |= BIT(req->channels[i]->band);
 		}
-
-		n_chans = req->n_channels;
 	} else {
 		do {
 			if (local->hw_scan_band == NUM_NL80211_BANDS)
 				return false;
 
-			n_chans = 0;
+			n_chans = &local->hw_scan_req->req.n_channels;
+			*n_chans = 0;
 
 			for (i = 0; i < req->n_channels; i++) {
 				if (req->channels[i]->band !=
 				    local->hw_scan_band)
 					continue;
-				local->hw_scan_req->req.channels[n_chans] =
+				local->hw_scan_req->req.channels[(*n_chans)++] =
 							req->channels[i];
-				n_chans++;
+
 				bands_used |= BIT(req->channels[i]->band);
 			}
 
 			local->hw_scan_band++;
-		} while (!n_chans);
+		} while (!*n_chans);
 	}
 
-	local->hw_scan_req->req.n_channels = n_chans;
 	ieee80211_prepare_scan_chandef(&chandef, req->scan_width);
 
 	if (req->flags & NL80211_SCAN_FLAG_MIN_PREQ_CONTENT)
-- 
2.43.0

Re: [PATCH 6.1] Drivers: hv: vmbus: Leak pages if set_memory_encrypted() fails

[Greg KH]

On Wed, Oct 09, 2024 at 04:16:26PM +0800, Xiangyu Chen wrote:
> From: Rick Edgecombe <rick.p.edgecombe@intel.com>
> 
> In CoCo VMs it is possible for the untrusted host to cause
> set_memory_encrypted() or set_memory_decrypted() to fail such that an
> error is returned and the resulting memory is shared. Callers need to
> take care to handle these errors to avoid returning decrypted (shared)
> memory to the page allocator, which could lead to functional or security
> issues.
> 
> VMBus code could free decrypted pages if set_memory_encrypted()/decrypted()
> fails. Leak the pages if this happens.
> 
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> Signed-off-by: Michael Kelley <mhklinux@outlook.com>
> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Link: https://lore.kernel.org/r/20240311161558.1310-2-mhklinux@outlook.com
> Signed-off-by: Wei Liu <wei.liu@kernel.org>
> Message-ID: <20240311161558.1310-2-mhklinux@outlook.com>
> [Xiangyu: Modified to apply on 6.1.y]
> Signed-off-by: Xiangyu Chen <xiangyu.chen@windriver.com>
> ---
>  drivers/hv/connection.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)

Are you sure?  This is _VERY_ different from what you suggested for
5.15.y and what is in mainline.  Also, you didn't show the git id for
the upstream commit.

Please work to figure this out and resend working versions for ALL
affected branches as new patches.

thanks,

greg k-h

Re: [PATCH v2 6.1/6.6] wifi: mac80211: Avoid address calculations via out of bounds array indexing

[Greg KH]

On Wed, Oct 09, 2024 at 04:16:27PM +0800, Xiangyu Chen wrote:
> From: Kenton Groombridge <concord@gentoo.org>
> 
> req->n_channels must be set before req->channels[] can be used.
> 
> This patch fixes one of the issues encountered in [1].
> 
> [   83.964255] UBSAN: array-index-out-of-bounds in net/mac80211/scan.c:364:4
> [   83.964258] index 0 is out of range for type 'struct ieee80211_channel *[]'
> [...]
> [   83.964264] Call Trace:
> [   83.964267]  <TASK>
> [   83.964269]  dump_stack_lvl+0x3f/0xc0
> [   83.964274]  __ubsan_handle_out_of_bounds+0xec/0x110
> [   83.964278]  ieee80211_prep_hw_scan+0x2db/0x4b0
> [   83.964281]  __ieee80211_start_scan+0x601/0x990
> [   83.964291]  nl80211_trigger_scan+0x874/0x980
> [   83.964295]  genl_family_rcv_msg_doit+0xe8/0x160
> [   83.964298]  genl_rcv_msg+0x240/0x270
> [...]
> 
> [1] https://bugzilla.kernel.org/show_bug.cgi?id=218810
> 
> Co-authored-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Kees Cook <kees@kernel.org>
> Signed-off-by: Kenton Groombridge <concord@gentoo.org>
> Link: https://msgid.link/20240605152218.236061-1-concord@gentoo.org
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> [Xiangyu: Modified to apply on 6.1.y and 6.6.y]
> Signed-off-by: Xiangyu Chen <xiangyu.chen@windriver.com>
> ---
> V1 -> V2:
> add v6.6 support

No hint as to what the git id of this is in Linus's tree, so now
dropping :(

Re: [PATCH 6.1] Drivers: hv: vmbus: Leak pages if set_memory_encrypted() fails

[Xiangyu Chen]

Hello Greg,


On 10/9/24 21:33, Greg KH wrote:
> CAUTION: This email comes from a non Wind River email account!
> Do not click links or open attachments unless you recognize the sender and know the content is safe.
>
> On Wed, Oct 09, 2024 at 04:16:26PM +0800, Xiangyu Chen wrote:
>> From: Rick Edgecombe <rick.p.edgecombe@intel.com>
>>
>> In CoCo VMs it is possible for the untrusted host to cause
>> set_memory_encrypted() or set_memory_decrypted() to fail such that an
>> error is returned and the resulting memory is shared. Callers need to
>> take care to handle these errors to avoid returning decrypted (shared)
>> memory to the page allocator, which could lead to functional or security
>> issues.
>>
>> VMBus code could free decrypted pages if set_memory_encrypted()/decrypted()
>> fails. Leak the pages if this happens.
>>
>> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
>> Signed-off-by: Michael Kelley <mhklinux@outlook.com>
>> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>> Link: https://lore.kernel.org/r/20240311161558.1310-2-mhklinux@outlook.com
>> Signed-off-by: Wei Liu <wei.liu@kernel.org>
>> Message-ID: <20240311161558.1310-2-mhklinux@outlook.com>
>> [Xiangyu: Modified to apply on 6.1.y]
>> Signed-off-by: Xiangyu Chen <xiangyu.chen@windriver.com>
>> ---
>>   drivers/hv/connection.c | 11 ++++++++++-
>>   1 file changed, 10 insertions(+), 1 deletion(-)
> Are you sure?  This is _VERY_ different from what you suggested for
> 5.15.y and what is in mainline.  Also, you didn't show the git id for
> the upstream commit.


This commit is a fix for CVE-2024-36913,  currently, if we fully apply 
the commit, we have to backport the

following commits from upstream:

a5ddb745 : Drivers: hv: vmbus: Remove second mapping of VMBus monitor pages

d786e00d : drivers: hv, hyperv_fb: Untangle and refactor Hyper-V panic 
notifiers

9c318a1d : Drivers: hv: move panic report code from vmbus to hv early 
init code

a6fe0438 : Drivers: hv: Change hv_free_hyperv_page() to take void * argument

03f5a999 : Drivers: hv: vmbus: Leak pages if set_memory_encrypted() fails

Some of them are features, it might not be merged to current stable 
branch, so another solution

is modify the connect.c by manual.


 From the upstream commit 03f5a999(Drivers: hv: vmbus: Leak pages if 
set_memory_encrypted() fails) we can see that

the commit aim to check set_memory_decrypted() result, if fails, the 
encryption of memory state is unknown, so leak the

memory.

The commit modified 2 functions, vmbus_connect() and vmbus_disconnect().

In vmbus_connect(), when set_memory_decrypted() fails, marking 
vmbus_connection.monitor_pages[0]/[1] to NULL.

In vmbus_disconnect(),  checking the monitor_pages[0]/[1] is valid, and 
checking set_memory_encrypted() status, if fails, free and

leak it.

On current v6.1 branch, vmbus_disconnect() will free those memory 
whatever set_memory_encrypted() is success or fails, so we can just

add a monitor_pages valid checking in it.


Could you please give a suggestion that which solution is following the 
stable-branch rule, I will resend a V2 patch, thanks.



Br,

Xiangyu

>
> Please work to figure this out and resend working versions for ALL
> affected branches as new patches.
>
> thanks,
>
> greg k-h

[PATCH 6.1] Drivers: hv: vmbus: Leak pages if set_memory_encrypted() fails

[Xiangyu Chen]

From: Rick Edgecombe <rick.p.edgecombe@intel.com>

[ Upstream commit 03f5a999adba062456c8c818a683beb1b498983a ]

In CoCo VMs it is possible for the untrusted host to cause
set_memory_encrypted() or set_memory_decrypted() to fail such that an
error is returned and the resulting memory is shared. Callers need to
take care to handle these errors to avoid returning decrypted (shared)
memory to the page allocator, which could lead to functional or security
issues.

VMBus code could free decrypted pages if set_memory_encrypted()/decrypted()
fails. Leak the pages if this happens.

Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
Signed-off-by: Michael Kelley <mhklinux@outlook.com>
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Link: https://lore.kernel.org/r/20240311161558.1310-2-mhklinux@outlook.com
Signed-off-by: Wei Liu <wei.liu@kernel.org>
Message-ID: <20240311161558.1310-2-mhklinux@outlook.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
[Xiangyu: bp to fix CVE-2024-36913, resolved minor conflicts]
Signed-off-by: Xiangyu Chen <xiangyu.chen@windriver.com>
---
 drivers/hv/connection.c | 66 ++++++++++++++++++++++++++---------------
 1 file changed, 42 insertions(+), 24 deletions(-)

diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
index da51b50787df..23fb0df9d350 100644
--- a/drivers/hv/connection.c
+++ b/drivers/hv/connection.c
@@ -243,8 +243,17 @@ int vmbus_connect(void)
 		ret |= set_memory_decrypted((unsigned long)
 					    vmbus_connection.monitor_pages[1],
 					    1);
-		if (ret)
+		if (ret) {
+			/*
+			 * If set_memory_decrypted() fails, the encryption state
+			 * of the memory is unknown. So leak the memory instead
+			 * of risking returning decrypted memory to the free list.
+			 * For simplicity, always handle both pages the same.
+			 */
+			vmbus_connection.monitor_pages[0] = NULL;
+			vmbus_connection.monitor_pages[1] = NULL;
 			goto cleanup;
+		}
 
 		/*
 		 * Isolation VM with AMD SNP needs to access monitor page via
@@ -377,30 +386,39 @@ void vmbus_disconnect(void)
 	}
 
 	if (hv_is_isolation_supported()) {
-		/*
-		 * memunmap() checks input address is ioremap address or not
-		 * inside. It doesn't unmap any thing in the non-SNP CVM and
-		 * so not check CVM type here.
-		 */
-		memunmap(vmbus_connection.monitor_pages[0]);
-		memunmap(vmbus_connection.monitor_pages[1]);
-
-		set_memory_encrypted((unsigned long)
-			vmbus_connection.monitor_pages_original[0],
-			1);
-		set_memory_encrypted((unsigned long)
-			vmbus_connection.monitor_pages_original[1],
-			1);
-	}
+		if(vmbus_connection.monitor_pages[0]) {
+			/*
+			 * memunmap() checks input address is ioremap address or not
+			 * inside. It doesn't unmap any thing in the non-SNP CVM and
+			 * so not check CVM type here.
+			 */
+			memunmap(vmbus_connection.monitor_pages[0]);
+			if (!set_memory_encrypted((unsigned long)
+				vmbus_connection.monitor_pages_original[0], 1))
+				hv_free_hyperv_page((unsigned long)vmbus_connection.monitor_pages[0]);
+			vmbus_connection.monitor_pages_original[0] =
+				vmbus_connection.monitor_pages[0] = NULL;
+		}
+
+		if(vmbus_connection.monitor_pages[1]) {
+			memunmap(vmbus_connection.monitor_pages[1]);
+			if (!set_memory_encrypted((unsigned long)
+				vmbus_connection.monitor_pages_original[1], 1))
+				hv_free_hyperv_page((unsigned long)vmbus_connection.monitor_pages[1]);
+			vmbus_connection.monitor_pages_original[1] =
+				vmbus_connection.monitor_pages[1] = NULL;
+		}
+	} else {
 
-	hv_free_hyperv_page((unsigned long)
-		vmbus_connection.monitor_pages_original[0]);
-	hv_free_hyperv_page((unsigned long)
-		vmbus_connection.monitor_pages_original[1]);
-	vmbus_connection.monitor_pages_original[0] =
-		vmbus_connection.monitor_pages[0] = NULL;
-	vmbus_connection.monitor_pages_original[1] =
-		vmbus_connection.monitor_pages[1] = NULL;
+		hv_free_hyperv_page((unsigned long)
+			vmbus_connection.monitor_pages_original[0]);
+		hv_free_hyperv_page((unsigned long)
+			vmbus_connection.monitor_pages_original[1]);
+		vmbus_connection.monitor_pages_original[0] =
+			vmbus_connection.monitor_pages[0] = NULL;
+		vmbus_connection.monitor_pages_original[1] =
+			vmbus_connection.monitor_pages[1] = NULL;
+	}
 }
 
 /*
-- 
2.43.0