[PATCH v4] xen/privcmd: fix error exit of privcmd_ioctl_dm_op()

[PATCH v4] xen/privcmd: fix error exit of privcmd_ioctl_dm_op()

[Juergen Gross]

The error exit of privcmd_ioctl_dm_op() is calling unlock_pages()
potentially with pages being NULL, leading to a NULL dereference.

Additionally lock_pages() doesn't check for pin_user_pages_fast()
having been completely successful, resulting in potentially not
locking all pages into memory. This could result in sporadic failures
when using the related memory in user mode.

Fix all of that by calling unlock_pages() always with the real number
of pinned pages, which will be zero in case pages being NULL, and by
checking the number of pages pinned by pin_user_pages_fast() matching
the expected number of pages.

Cc: <stable@vger.kernel.org>
Fixes: ab520be8cd5d ("xen/privcmd: Add IOCTL_PRIVCMD_DM_OP")
Reported-by: Rustam Subkhankulov <subkhankulov@ispras.ru>
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- use "pinned" as parameter for unlock_pages() (Jan Beulich)
- drop label "unlock" again (Jan Beulich)
- add check for complete success of pin_user_pages_fast()
V3:
- continue after partial success of pin_user_pages_fast() (Jan Beulich)
V4:
- fix case of multiple partial successes for one buffer (Jan Beulich)
---
 drivers/xen/privcmd.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index 3369734108af..e88e8f6f0a33 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -581,27 +581,30 @@ static int lock_pages(
 	struct privcmd_dm_op_buf kbufs[], unsigned int num,
 	struct page *pages[], unsigned int nr_pages, unsigned int *pinned)
 {
-	unsigned int i;
+	unsigned int i, off = 0;
 
-	for (i = 0; i < num; i++) {
+	for (i = 0; i < num; ) {
 		unsigned int requested;
 		int page_count;
 
 		requested = DIV_ROUND_UP(
 			offset_in_page(kbufs[i].uptr) + kbufs[i].size,
-			PAGE_SIZE);
+			PAGE_SIZE) - off;
 		if (requested > nr_pages)
 			return -ENOSPC;
 
 		page_count = pin_user_pages_fast(
-			(unsigned long) kbufs[i].uptr,
+			(unsigned long)kbufs[i].uptr + off * PAGE_SIZE,
 			requested, FOLL_WRITE, pages);
-		if (page_count < 0)
-			return page_count;
+		if (page_count <= 0)
+			return page_count ? : -EFAULT;
 
 		*pinned += page_count;
 		nr_pages -= page_count;
 		pages += page_count;
+
+		off = (requested == page_count) ? 0 : off + page_count;
+		i += !off;
 	}
 
 	return 0;
@@ -677,10 +680,8 @@ static long privcmd_ioctl_dm_op(struct file *file, void __user *udata)
 	}
 
 	rc = lock_pages(kbufs, kdata.num, pages, nr_pages, &pinned);
-	if (rc < 0) {
-		nr_pages = pinned;
+	if (rc < 0)
 		goto out;
-	}
 
 	for (i = 0; i < kdata.num; i++) {
 		set_xen_guest_handle(xbufs[i].h, kbufs[i].uptr);
@@ -692,7 +693,7 @@ static long privcmd_ioctl_dm_op(struct file *file, void __user *udata)
 	xen_preemptible_hcall_end();
 
 out:
-	unlock_pages(pages, nr_pages);
+	unlock_pages(pages, pinned);
 	kfree(xbufs);
 	kfree(pages);
 	kfree(kbufs);
-- 
2.35.3

Re: [PATCH v4] xen/privcmd: fix error exit of privcmd_ioctl_dm_op()

[Jan Beulich]

On 25.08.2022 16:19, Juergen Gross wrote:
> The error exit of privcmd_ioctl_dm_op() is calling unlock_pages()
> potentially with pages being NULL, leading to a NULL dereference.
> 
> Additionally lock_pages() doesn't check for pin_user_pages_fast()
> having been completely successful, resulting in potentially not
> locking all pages into memory. This could result in sporadic failures
> when using the related memory in user mode.
> 
> Fix all of that by calling unlock_pages() always with the real number
> of pinned pages, which will be zero in case pages being NULL, and by
> checking the number of pages pinned by pin_user_pages_fast() matching
> the expected number of pages.
> 
> Cc: <stable@vger.kernel.org>
> Fixes: ab520be8cd5d ("xen/privcmd: Add IOCTL_PRIVCMD_DM_OP")
> Reported-by: Rustam Subkhankulov <subkhankulov@ispras.ru>
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH v4] xen/privcmd: fix error exit of privcmd_ioctl_dm_op()

[Oleksandr Tyshchenko]

On 25.08.22 17:19, Juergen Gross wrote:

Hello Juergen

> The error exit of privcmd_ioctl_dm_op() is calling unlock_pages()
> potentially with pages being NULL, leading to a NULL dereference.
>
> Additionally lock_pages() doesn't check for pin_user_pages_fast()
> having been completely successful, resulting in potentially not
> locking all pages into memory. This could result in sporadic failures
> when using the related memory in user mode.
>
> Fix all of that by calling unlock_pages() always with the real number
> of pinned pages, which will be zero in case pages being NULL, and by
> checking the number of pages pinned by pin_user_pages_fast() matching
> the expected number of pages.
>
> Cc: <stable@vger.kernel.org>
> Fixes: ab520be8cd5d ("xen/privcmd: Add IOCTL_PRIVCMD_DM_OP")
> Reported-by: Rustam Subkhankulov <subkhankulov@ispras.ru>
> Signed-off-by: Juergen Gross <jgross@suse.com>


I haven't spotted any issues:

Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>


> ---
> V2:
> - use "pinned" as parameter for unlock_pages() (Jan Beulich)
> - drop label "unlock" again (Jan Beulich)
> - add check for complete success of pin_user_pages_fast()
> V3:
> - continue after partial success of pin_user_pages_fast() (Jan Beulich)
> V4:
> - fix case of multiple partial successes for one buffer (Jan Beulich)
> ---
>   drivers/xen/privcmd.c | 21 +++++++++++----------
>   1 file changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> index 3369734108af..e88e8f6f0a33 100644
> --- a/drivers/xen/privcmd.c
> +++ b/drivers/xen/privcmd.c
> @@ -581,27 +581,30 @@ static int lock_pages(
>   	struct privcmd_dm_op_buf kbufs[], unsigned int num,
>   	struct page *pages[], unsigned int nr_pages, unsigned int *pinned)
>   {
> -	unsigned int i;
> +	unsigned int i, off = 0;
>   
> -	for (i = 0; i < num; i++) {
> +	for (i = 0; i < num; ) {
>   		unsigned int requested;
>   		int page_count;
>   
>   		requested = DIV_ROUND_UP(
>   			offset_in_page(kbufs[i].uptr) + kbufs[i].size,
> -			PAGE_SIZE);
> +			PAGE_SIZE) - off;
>   		if (requested > nr_pages)
>   			return -ENOSPC;
>   
>   		page_count = pin_user_pages_fast(
> -			(unsigned long) kbufs[i].uptr,
> +			(unsigned long)kbufs[i].uptr + off * PAGE_SIZE,
>   			requested, FOLL_WRITE, pages);
> -		if (page_count < 0)
> -			return page_count;
> +		if (page_count <= 0)
> +			return page_count ? : -EFAULT;


[not related to the current patch]

I just wonder, whether drivers/xen/gntdev.c:gntdev_get_page() really 
wants to gain the same check?

index 59ffea800079..45e16031204d 100644
--- a/drivers/xen/gntdev.c
+++ b/drivers/xen/gntdev.c
@@ -740,8 +740,8 @@ static int gntdev_get_page(struct gntdev_copy_batch 
*batch, void __user *virt,
         int ret;

         ret = pin_user_pages_fast(addr, 1, batch->writeable ? 
FOLL_WRITE : 0, &page);
-       if (ret < 0)
-               return ret;
+       if (ret <= 0)
+               return ret ? : -EFAULT;

         batch->pages[batch->nr_pages++] = page;



>   
>   		*pinned += page_count;
>   		nr_pages -= page_count;
>   		pages += page_count;
> +
> +		off = (requested == page_count) ? 0 : off + page_count;
> +		i += !off;
>   	}
>   
>   	return 0;
> @@ -677,10 +680,8 @@ static long privcmd_ioctl_dm_op(struct file *file, void __user *udata)
>   	}
>   
>   	rc = lock_pages(kbufs, kdata.num, pages, nr_pages, &pinned);
> -	if (rc < 0) {
> -		nr_pages = pinned;
> +	if (rc < 0)
>   		goto out;
> -	}
>   
>   	for (i = 0; i < kdata.num; i++) {
>   		set_xen_guest_handle(xbufs[i].h, kbufs[i].uptr);
> @@ -692,7 +693,7 @@ static long privcmd_ioctl_dm_op(struct file *file, void __user *udata)
>   	xen_preemptible_hcall_end();
>   
>   out:
> -	unlock_pages(pages, nr_pages);
> +	unlock_pages(pages, pinned);
>   	kfree(xbufs);
>   	kfree(pages);
>   	kfree(kbufs);

-- 
Regards,

Oleksandr Tyshchenko

Re: [PATCH v4] xen/privcmd: fix error exit of privcmd_ioctl_dm_op()

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 3514 bytes --]

On 26.08.22 11:08, Oleksandr Tyshchenko wrote:
> 
> On 25.08.22 17:19, Juergen Gross wrote:
> 
> Hello Juergen
> 
>> The error exit of privcmd_ioctl_dm_op() is calling unlock_pages()
>> potentially with pages being NULL, leading to a NULL dereference.
>>
>> Additionally lock_pages() doesn't check for pin_user_pages_fast()
>> having been completely successful, resulting in potentially not
>> locking all pages into memory. This could result in sporadic failures
>> when using the related memory in user mode.
>>
>> Fix all of that by calling unlock_pages() always with the real number
>> of pinned pages, which will be zero in case pages being NULL, and by
>> checking the number of pages pinned by pin_user_pages_fast() matching
>> the expected number of pages.
>>
>> Cc: <stable@vger.kernel.org>
>> Fixes: ab520be8cd5d ("xen/privcmd: Add IOCTL_PRIVCMD_DM_OP")
>> Reported-by: Rustam Subkhankulov <subkhankulov@ispras.ru>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> 
> I haven't spotted any issues:
> 
> Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> 
> 
>> ---
>> V2:
>> - use "pinned" as parameter for unlock_pages() (Jan Beulich)
>> - drop label "unlock" again (Jan Beulich)
>> - add check for complete success of pin_user_pages_fast()
>> V3:
>> - continue after partial success of pin_user_pages_fast() (Jan Beulich)
>> V4:
>> - fix case of multiple partial successes for one buffer (Jan Beulich)
>> ---
>>    drivers/xen/privcmd.c | 21 +++++++++++----------
>>    1 file changed, 11 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
>> index 3369734108af..e88e8f6f0a33 100644
>> --- a/drivers/xen/privcmd.c
>> +++ b/drivers/xen/privcmd.c
>> @@ -581,27 +581,30 @@ static int lock_pages(
>>    	struct privcmd_dm_op_buf kbufs[], unsigned int num,
>>    	struct page *pages[], unsigned int nr_pages, unsigned int *pinned)
>>    {
>> -	unsigned int i;
>> +	unsigned int i, off = 0;
>>    
>> -	for (i = 0; i < num; i++) {
>> +	for (i = 0; i < num; ) {
>>    		unsigned int requested;
>>    		int page_count;
>>    
>>    		requested = DIV_ROUND_UP(
>>    			offset_in_page(kbufs[i].uptr) + kbufs[i].size,
>> -			PAGE_SIZE);
>> +			PAGE_SIZE) - off;
>>    		if (requested > nr_pages)
>>    			return -ENOSPC;
>>    
>>    		page_count = pin_user_pages_fast(
>> -			(unsigned long) kbufs[i].uptr,
>> +			(unsigned long)kbufs[i].uptr + off * PAGE_SIZE,
>>    			requested, FOLL_WRITE, pages);
>> -		if (page_count < 0)
>> -			return page_count;
>> +		if (page_count <= 0)
>> +			return page_count ? : -EFAULT;
> 
> 
> [not related to the current patch]
> 
> I just wonder, whether drivers/xen/gntdev.c:gntdev_get_page() really
> wants to gain the same check?
> 
> index 59ffea800079..45e16031204d 100644
> --- a/drivers/xen/gntdev.c
> +++ b/drivers/xen/gntdev.c
> @@ -740,8 +740,8 @@ static int gntdev_get_page(struct gntdev_copy_batch
> *batch, void __user *virt,
>           int ret;
> 
>           ret = pin_user_pages_fast(addr, 1, batch->writeable ?
> FOLL_WRITE : 0, &page);
> -       if (ret < 0)
> -               return ret;
> +       if (ret <= 0)
> +               return ret ? : -EFAULT;
> 
>           batch->pages[batch->nr_pages++] = page;

I don't think this is needed here, as pin_user_pages_fast() can't
return 0 when called for a single page.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]