| Message ID | 1474461664-32624-1-git-send-email-rcojocaru@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
>>> 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
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
>>> 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
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) ) {
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(-)