| Message ID | 1427987752.22575.65.camel@opteya.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
Hi, > -----Original Message----- > From: Yann Droneaud [mailto:ydroneaud@opteya.com] > Sent: Thursday, April 02, 2015 6:16 PM > To: Shachar Raindel > Cc: oss-security@lists.openwall.com; <linux-rdma@vger.kernel.org> > (linux-rdma@vger.kernel.org); linux-kernel@vger.kernel.org; > stable@vger.kernel.org > Subject: Re: CVE-2014-8159 kernel: infiniband: uverbs: unprotected > physical memory access > > Hi, > Le jeudi 02 avril 2015 à 10:52 +0000, Shachar Raindel a écrit : > > > -----Original Message----- > > > From: Yann Droneaud [mailto:ydroneaud@opteya.com] > > > 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. Thanks, --Shachar
Hi, Le jeudi 02 avril 2015 à 16:34 +0000, Shachar Raindel a écrit : > > -----Original Message----- > > From: Yann Droneaud [mailto:ydroneaud@opteya.com] > > 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@opteya.com] > > > > 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.
Hi, Le mercredi 08 avril 2015 à 14:19 +0200, Yann Droneaud a écrit : > Le jeudi 02 avril 2015 à 16:34 +0000, Shachar Raindel a écrit : > > > -----Original Message----- > > > From: Yann Droneaud [mailto:ydroneaud@opteya.com] > > > 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@opteya.com] > > > > > 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 It only check address not being 0 if size is already PAGE_SIZE aligned, and it only check size not being 0 if address is already PAGE_SIZE aligned. > 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.
diff --git a/drivers/infiniband/core/umem.c b/drivers/infiniband/core/umem.c index df0c4f605a21..6758b4ac56eb 100644 --- a/drivers/infiniband/core/umem.c +++ b/drivers/infiniband/core/umem.c @@ -90,6 +90,7 @@ struct ib_umem *ib_umem_get(struct ib_ucontext *context, unsigned long addr, DEFINE_DMA_ATTRS(attrs); struct scatterlist *sg, *sg_list_start; int need_release = 0; + bool writable; if (dmasync) dma_set_attr(DMA_ATTR_WRITE_BARRIER, &attrs); @@ -97,6 +98,19 @@ struct ib_umem *ib_umem_get(struct ib_ucontext *context, unsigned long addr, if (!can_do_mlock()) return ERR_PTR(-EPERM); + /* + * We ask for writable memory if any access flags other than + * "remote read" are set. "Local write" and "remote write" + * obviously require write access. "Remote atomic" can do + * things like fetch and add, which will modify memory, and + * "MW bind" can change permissions by binding a window. + */ + writable = !!(access & ~IB_ACCESS_REMOTE_READ); + + if (!access_ok(writable ? VERIFY_WRITE : VERIFY_READ, + (void __user *)addr, size)) + return ERR_PTR(-EFAULT); + umem = kzalloc(sizeof *umem, GFP_KERNEL); if (!umem) return ERR_PTR(-ENOMEM); @@ -106,14 +120,7 @@ struct ib_umem *ib_umem_get(struct ib_ucontext *context, unsigned long addr, umem->offset = addr & ~PAGE_MASK; umem->page_size = PAGE_SIZE; umem->pid = get_task_pid(current, PIDTYPE_PID); - /* - * We ask for writable memory if any access flags other than - * "remote read" are set. "Local write" and "remote write" - * obviously require write access. "Remote atomic" can do - * things like fetch and add, which will modify memory, and - * "MW bind" can change permissions by binding a window. - */ - umem->writable = !!(access & ~IB_ACCESS_REMOTE_READ); + umem->writable = writable; /* We assume the memory is from hugetlb until proved otherwise */ umem->hugetlb = 1;