Re: CVE-2014-8159 kernel: infiniband: uverbs: unprotected physical memory access

[Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>]

Hi,

Le jeudi 02 avril 2015 à 16:34 +0000, Shachar Raindel a écrit :
> > -----Original Message-----
> > From: Yann Droneaud [mailto:ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org]
> > Sent: Thursday, April 02, 2015 6:16 PM
> > Le jeudi 02 avril 2015 à 10:52 +0000, Shachar Raindel a écrit :
> > > > -----Original Message-----
> > > > From: Yann Droneaud [mailto:ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org]
> > > > Sent: Thursday, April 02, 2015 1:05 PM
> > > > Le mercredi 18 mars 2015 à 17:39 +0000, Shachar Raindel a écrit :
> > ...
> > > > > +	/*
> > > > > +	 * If the combination of the addr and size requested for this
> > > > memory
> > > > > +	 * region causes an integer overflow, return error.
> > > > > +	 */
> > > > > +	if ((PAGE_ALIGN(addr + size) <= size) ||
> > > > > +	    (PAGE_ALIGN(addr + size) <= addr))
> > > > > +		return ERR_PTR(-EINVAL);
> > > > > +
> > > >
> > > > Can access_ok() be used here ?
> > > >
> > > >          if (!access_ok(writable ? VERIFY_WRITE : VERIFY_READ,
> > > >                         addr, size))
> > > >                   return ERR_PTR(-EINVAL);
> > > >
> > >
> > > No, this will break the current ODP semantics.
> > >
> > > ODP allows the user to register memory that is not accessible yet.
> > > This is a critical design feature, as it allows avoiding holding
> > > a registration cache. Adding this check will break the behavior,
> > > forcing memory to be all accessible when registering an ODP MR.
> > >
> > 
> > Failed to notice previously, but since this would break ODP, and ODP is
> > only available starting v3.19-rc1, my proposed fix might be applicable
> > for older kernel (if not better).
> > 
> 
> Can you explain how this proposed fix is better than the existing patch?
> Why do we want to push to the stable tree a patch that is not in the
> upstream? There is an existing, tested, patch that is going to the tip
> of the development. It even applies cleanly on every kernel version around.
> 

access_ok() check for overflow *and* that the region is the memory range
for the current process. The later check is not done in your proposed 
fix (but it should not be needed as get_user_pages() will be called 
to validate the whole region for non-ODP memory registration).

Anyway, AFAIK access_ok() won't check for address being not NULL and
size not being 0, and I've noticed your proposed fix also ensure address
is not equal to NULL and, more important, that size is not equal to 0:
before v3.15-rc1 and commit eeb8461e36c9 ("IB: Refactor umem to use
linear SG table"), calling ib_umem_get() with size equal to 0 would 
succeed with any arbitrary address ... who knows what might happen in 
the lowlevel drivers (aka. providers) if they got an umem for a 0-sized
memory region.
This part of the changes was not detailled in your commit message: it's
an issue not related to overflow which is addressed by your patch.

So I agree my proposed patch is no better than yours: I've missed the
0-sized memory region issue and didn't take care of NULL address.

Regards.

-- 
Yann Droneaud
OPTEYA


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html