[PATCH 0/3] Use new wrappers to copy userspace arrays

[PATCH 0/3] Use new wrappers to copy userspace arrays

[Philipp Stanner]

Hi,

Linus recently merged [1] the wrapper functions memdup_array_user() and
vmemdup_array_user() in include/linux/string.h for Kernel v6.7

I am currently adding them to all places where (v)memdup_user() had been
used to copy arrays.

The wrapper is different to the wrapped functions only in that it might
return -EOVERFLOW. So this new error code might get pushed up to
userspace. I hope this is fine.

I felt that it might be a good idea to land those three patches here
with a single series, since they all touch something KVM-related.

Kind regards,
P.

[1] https://lore.kernel.org/all/169886743808.2396.17544791408117731525.pr-tracker-bot@kernel.org/

Philipp Stanner (3):
  arch/x86/kvm: copy user-array with overflow-check
  arch/s390/kvm: copy userspace-array safely
  virt/kvm: copy userspace-array safely

 arch/s390/kvm/guestdbg.c | 4 ++--
 arch/x86/kvm/cpuid.c     | 4 ++--
 virt/kvm/kvm_main.c      | 5 ++---
 3 files changed, 6 insertions(+), 7 deletions(-)

-- 
2.41.0

[PATCH 1/3] arch/x86/kvm: copy user-array with overflow-check

[Philipp Stanner]

cpuid.c utilizes vmemdup_user() and array_size() to copy two userspace
arrays. This, currently, does not check for an overflow.

Use the new wrapper vmemdup_array_user() to copy the arrays more safely.

Suggested-by: Dave Airlie <airlied@redhat.com>
Signed-off-by: Philipp Stanner <pstanner@redhat.com>
---
 arch/x86/kvm/cpuid.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 773132c3bf5a..4a15b2a20f84 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -467,7 +467,7 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
 		return -E2BIG;
 
 	if (cpuid->nent) {
-		e = vmemdup_user(entries, array_size(sizeof(*e), cpuid->nent));
+		e = vmemdup_array_user(entries, cpuid->nent, sizeof(*e));
 		if (IS_ERR(e))
 			return PTR_ERR(e);
 
@@ -511,7 +511,7 @@ int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu,
 		return -E2BIG;
 
 	if (cpuid->nent) {
-		e2 = vmemdup_user(entries, array_size(sizeof(*e2), cpuid->nent));
+		e2 = vmemdup_array_user(entries, cpuid->nent, sizeof(*e2));
 		if (IS_ERR(e2))
 			return PTR_ERR(e2);
 	}
-- 
2.41.0

[PATCH 2/3] arch/s390/kvm: copy userspace-array safely

[Philipp Stanner]

guestdbg.c utilizes memdup_user() to copy a userspace array. This,
currently, does not check for an overflow.

Use the new wrapper memdup_array_user() to copy the array more safely.

Suggested-by: Dave Airlie <airlied@redhat.com>
Signed-off-by: Philipp Stanner <pstanner@redhat.com>
---
 arch/s390/kvm/guestdbg.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/s390/kvm/guestdbg.c b/arch/s390/kvm/guestdbg.c
index 3765c4223bf9..80879fc73c90 100644
--- a/arch/s390/kvm/guestdbg.c
+++ b/arch/s390/kvm/guestdbg.c
@@ -213,8 +213,8 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
 	else if (dbg->arch.nr_hw_bp > MAX_BP_COUNT)
 		return -EINVAL;
 
-	bp_data = memdup_user(dbg->arch.hw_bp,
-			      sizeof(*bp_data) * dbg->arch.nr_hw_bp);
+	bp_data = memdup_array_user(dbg->arch.hw_bp, dbg->arch.nr_hw_bp,
+				    sizeof(*bp_data));
 	if (IS_ERR(bp_data))
 		return PTR_ERR(bp_data);
 
-- 
2.41.0

[PATCH 3/3] virt/kvm: copy userspace-array safely

[Philipp Stanner]

kvm_main.c utilizes vmemdup_user() and array_size() to copy a userspace
array. Currently, this does not check for an overflow.

Use the new wrapper vmemdup_array_user() to copy the array more safely.

Suggested-by: Dave Airlie <airlied@redhat.com>
Signed-off-by: Philipp Stanner <pstanner@redhat.com>
---
 virt/kvm/kvm_main.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 486800a7024b..2a2f409c2a7d 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -4932,9 +4932,8 @@ static long kvm_vm_ioctl(struct file *filp,
 			goto out;
 		if (routing.nr) {
 			urouting = argp;
-			entries = vmemdup_user(urouting->entries,
-					       array_size(sizeof(*entries),
-							  routing.nr));
+			entries = vmemdup_array_user(urouting->entries,
+						     routing.nr, sizeof(*entries));
 			if (IS_ERR(entries)) {
 				r = PTR_ERR(entries);
 				goto out;
-- 
2.41.0

Re: [PATCH 2/3] arch/s390/kvm: copy userspace-array safely

[Claudio Imbrenda]

On Thu,  2 Nov 2023 19:15:25 +0100
Philipp Stanner <pstanner@redhat.com> wrote:

> guestdbg.c utilizes memdup_user() to copy a userspace array. This,
> currently, does not check for an overflow.
> 
> Use the new wrapper memdup_array_user() to copy the array more safely.
> 
> Suggested-by: Dave Airlie <airlied@redhat.com>
> Signed-off-by: Philipp Stanner <pstanner@redhat.com>

Acked-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

> ---
>  arch/s390/kvm/guestdbg.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/s390/kvm/guestdbg.c b/arch/s390/kvm/guestdbg.c
> index 3765c4223bf9..80879fc73c90 100644
> --- a/arch/s390/kvm/guestdbg.c
> +++ b/arch/s390/kvm/guestdbg.c
> @@ -213,8 +213,8 @@ int kvm_s390_import_bp_data(struct kvm_vcpu *vcpu,
>  	else if (dbg->arch.nr_hw_bp > MAX_BP_COUNT)
>  		return -EINVAL;
>  
> -	bp_data = memdup_user(dbg->arch.hw_bp,
> -			      sizeof(*bp_data) * dbg->arch.nr_hw_bp);
> +	bp_data = memdup_array_user(dbg->arch.hw_bp, dbg->arch.nr_hw_bp,
> +				    sizeof(*bp_data));
>  	if (IS_ERR(bp_data))
>  		return PTR_ERR(bp_data);
>  

Re: [PATCH 0/3] Use new wrappers to copy userspace arrays

[Sean Christopherson]

On Thu, 02 Nov 2023 19:15:23 +0100, Philipp Stanner wrote:
> Linus recently merged [1] the wrapper functions memdup_array_user() and
> vmemdup_array_user() in include/linux/string.h for Kernel v6.7
> 
> I am currently adding them to all places where (v)memdup_user() had been
> used to copy arrays.
> 
> The wrapper is different to the wrapped functions only in that it might
> return -EOVERFLOW. So this new error code might get pushed up to
> userspace. I hope this is fine.
> 
> [...]

Applied to kvm-x86 generic.  Claudio (or anyone else from s390), holler if
you want to take the s390 patch through the s390 tree.

I massaged the shortlogs to use KVM's standard scopes, and to make it clear
that these are hardening patches, i.e. that there is no unsafe/buggy behavior
that is being fixed.  I also added a note at the end of each changelog to call
out that KVM pre-checks the sizes before copying, again to make it clear that
using the safer helper isn't expected to actually change KVM's behavior.

[1/3] KVM: x86: Harden copying of userspace-array against overflow
      https://github.com/kvm-x86/linux/commit/573cc0e5cf14
[2/3] KVM: s390: Harden copying of userspace-array against overflow
      https://github.com/kvm-x86/linux/commit/8b81a8d7c6b7
[3/3] KVM: Harden copying of userspace-array against overflow
      https://github.com/kvm-x86/linux/commit/bc2cad56094c

--
https://github.com/kvm-x86/linux/tree/next

Re: [PATCH 0/3] Use new wrappers to copy userspace arrays

[Christian Borntraeger]

Am 01.12.23 um 02:52 schrieb Sean Christopherson:
> On Thu, 02 Nov 2023 19:15:23 +0100, Philipp Stanner wrote:
>> Linus recently merged [1] the wrapper functions memdup_array_user() and
>> vmemdup_array_user() in include/linux/string.h for Kernel v6.7
>>
>> I am currently adding them to all places where (v)memdup_user() had been
>> used to copy arrays.
>>
>> The wrapper is different to the wrapped functions only in that it might
>> return -EOVERFLOW. So this new error code might get pushed up to
>> userspace. I hope this is fine.
>>
>> [...]
> 
> Applied to kvm-x86 generic.  Claudio (or anyone else from s390), holler if
> you want to take the s390 patch through the s390 tree.

I think this is fine via your tree.

Feel free to add
Acked-by: Christian Borntraeger <borntraeger@linux.ibm.com>
to patch 2 if the commit id is not yet final.

Re: [PATCH 0/3] Use new wrappers to copy userspace arrays

[Sean Christopherson]

On Fri, Dec 01, 2023, Christian Borntraeger wrote:
> 
> 
> Am 01.12.23 um 02:52 schrieb Sean Christopherson:
> > On Thu, 02 Nov 2023 19:15:23 +0100, Philipp Stanner wrote:
> > > Linus recently merged [1] the wrapper functions memdup_array_user() and
> > > vmemdup_array_user() in include/linux/string.h for Kernel v6.7
> > > 
> > > I am currently adding them to all places where (v)memdup_user() had been
> > > used to copy arrays.
> > > 
> > > The wrapper is different to the wrapped functions only in that it might
> > > return -EOVERFLOW. So this new error code might get pushed up to
> > > userspace. I hope this is fine.
> > > 
> > > [...]
> > 
> > Applied to kvm-x86 generic.  Claudio (or anyone else from s390), holler if
> > you want to take the s390 patch through the s390 tree.
> 
> I think this is fine via your tree.
> 
> Feel free to add
> Acked-by: Christian Borntraeger <borntraeger@linux.ibm.com>
> to patch 2 if the commit id is not yet final.

Done, thanks much!  New hashes if anyone cares:

[1/3] KVM: x86: Harden copying of userspace-array against overflow
      https://github.com/kvm-x86/linux/commit/573cc0e5cf14
[2/3] KVM: s390: Harden copying of userspace-array against overflow
      https://github.com/kvm-x86/linux/commit/8c4976772d9b
[3/3] KVM: Harden copying of userspace-array against overflow
      https://github.com/kvm-x86/linux/commit/1f829359c8c3