[PATCH V2] x86/mm: Fix Coverity issues 1373105 and 1373106

[PATCH V2] x86/mm: Fix Coverity issues 1373105 and 1373106

[Razvan Cojocaru]

Added missing error checks in p2m_set_mem_access_multi().

Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
---
Changes since V1:
 - Returning -EFAULT instead of -EINVAL.
 - Replaced stray TAB with spaces.
---
 xen/arch/x86/mm/p2m.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index b16e563..9526fff 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1936,8 +1936,12 @@ long p2m_set_mem_access_multi(struct domain *d,
         uint8_t access;
         uint64_t gfn_l;
 
-        copy_from_guest_offset(&gfn_l, pfn_list, start, 1);
-        copy_from_guest_offset(&access, access_list, start, 1);
+        if ( copy_from_guest_offset(&gfn_l, pfn_list, start, 1) ||
+             copy_from_guest_offset(&access, access_list, start, 1) )
+        {
+            rc = -EFAULT;
+            break;
+        }
 
         if ( !xenmem_access_to_p2m_access(p2m, access, &a) )
         {
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH V2] x86/mm: Fix Coverity issues 1373105 and 1373106

[George Dunlap]

On 21/09/16 13:41, Razvan Cojocaru wrote:
> Added missing error checks in p2m_set_mem_access_multi().
> 
> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
> ---
> Changes since V1:
>  - Returning -EFAULT instead of -EINVAL.
>  - Replaced stray TAB with spaces.
> ---
>  xen/arch/x86/mm/p2m.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index b16e563..9526fff 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1936,8 +1936,12 @@ long p2m_set_mem_access_multi(struct domain *d,
>          uint8_t access;
>          uint64_t gfn_l;
>  
> -        copy_from_guest_offset(&gfn_l, pfn_list, start, 1);
> -        copy_from_guest_offset(&access, access_list, start, 1);
> +        if ( copy_from_guest_offset(&gfn_l, pfn_list, start, 1) ||
> +             copy_from_guest_offset(&access, access_list, start, 1) )
> +        {
> +            rc = -EFAULT;
> +            break;
> +        }

This will return EFAULT even if it has managed to successfully handle
some of the pfn/access pairs.  It looks like this is sort of typical
(the handful of places I could find that had copy_from_guest* inside a
loop followed a similar form).

So:

Reviewed-by: George Dunlap <george.dunlap@citrix.com>

I'll check this in.

 -George


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH V2] x86/mm: Fix Coverity issues 1373105 and 1373106

[Jan Beulich]

>>> On 21.09.16 at 14:41, <rcojocaru@bitdefender.com> wrote:
> Added missing error checks in p2m_set_mem_access_multi().

I think the patch subject should say what is being changed/fixed,
and the two Coverity IDs should be listed here instead. Otherwise
someone looking over just the patch titles will have no idea what
this is actually about. If I end up committing this, I'll take the liberty
to do adjustments.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH V2] x86/mm: Fix Coverity issues 1373105 and 1373106

[George Dunlap]

On 21/09/16 13:52, Jan Beulich wrote:
>>>> On 21.09.16 at 14:41, <rcojocaru@bitdefender.com> wrote:
>> Added missing error checks in p2m_set_mem_access_multi().
> 
> I think the patch subject should say what is being changed/fixed,
> and the two Coverity IDs should be listed here instead. Otherwise
> someone looking over just the patch titles will have no idea what
> this is actually about. If I end up committing this, I'll take the liberty
> to do adjustments.

I said I'd commit it, so how about:

x86/mm: Add missing copy_from_user error checks in p2m_set_access_multi

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH V2] x86/mm: Fix Coverity issues 1373105 and 1373106

[Jan Beulich]

>>> On 21.09.16 at 14:55, <george.dunlap@citrix.com> wrote:
> On 21/09/16 13:52, Jan Beulich wrote:
>>>>> On 21.09.16 at 14:41, <rcojocaru@bitdefender.com> wrote:
>>> Added missing error checks in p2m_set_mem_access_multi().
>> 
>> I think the patch subject should say what is being changed/fixed,
>> and the two Coverity IDs should be listed here instead. Otherwise
>> someone looking over just the patch titles will have no idea what
>> this is actually about. If I end up committing this, I'll take the liberty
>> to do adjustments.
> 
> I said I'd commit it, so how about:
> 
> x86/mm: Add missing copy_from_user error checks in p2m_set_access_multi

Sound good.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel