| Message ID | bc145167-0471-4ab3-935c-aa5dc20e342a@moroto.mountain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dma-buf: heaps: Fix off by one in cma_heap_vm_fault() | expand |
On Mon, Oct 2, 2023 at 12:04 AM Dan Carpenter <dan.carpenter@linaro.org> wrote: > > The buffer->pages[] has "buffer->pagecount" elements so this > comparison > has to be changed to >= to avoid reading beyond the end of the array. > The buffer->pages[] array is allocated in cma_heap_allocate(). > > Fixes: a5d2d29e24be ("dma-buf: heaps: Move heap-helper logic into the cma_heap implementation") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > drivers/dma-buf/heaps/cma_heap.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c > index ee899f8e6721..bea7e574f916 100644 > --- a/drivers/dma-buf/heaps/cma_heap.c > +++ b/drivers/dma-buf/heaps/cma_heap.c > @@ -165,7 +165,7 @@ static vm_fault_t cma_heap_vm_fault(struct vm_fault *vmf) > struct vm_area_struct *vma = vmf->vma; > struct cma_heap_buffer *buffer = vma->vm_private_data; > > - if (vmf->pgoff > buffer->pagecount) > + if (vmf->pgoff >= buffer->pagecount) > return VM_FAULT_SIGBUS; > Hi Dan, Your fix looks correct to me, but I'm curious if you observed this problem on a device? The mmap in dma-buf.c looks like it prevents creating a mapping that is too large, and I think an access beyond the VMA should segfault before reaching here. Thanks, T.J.
On Mon, Oct 02, 2023 at 10:16:24AM -0700, T.J. Mercier wrote: > On Mon, Oct 2, 2023 at 12:04 AM Dan Carpenter <dan.carpenter@linaro.org> wrote: > > > > The buffer->pages[] has "buffer->pagecount" elements so this > comparison > > has to be changed to >= to avoid reading beyond the end of the array. > > The buffer->pages[] array is allocated in cma_heap_allocate(). > > > > Fixes: a5d2d29e24be ("dma-buf: heaps: Move heap-helper logic into the cma_heap implementation") > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > > --- > > drivers/dma-buf/heaps/cma_heap.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c > > index ee899f8e6721..bea7e574f916 100644 > > --- a/drivers/dma-buf/heaps/cma_heap.c > > +++ b/drivers/dma-buf/heaps/cma_heap.c > > @@ -165,7 +165,7 @@ static vm_fault_t cma_heap_vm_fault(struct vm_fault *vmf) > > struct vm_area_struct *vma = vmf->vma; > > struct cma_heap_buffer *buffer = vma->vm_private_data; > > > > - if (vmf->pgoff > buffer->pagecount) > > + if (vmf->pgoff >= buffer->pagecount) > > return VM_FAULT_SIGBUS; > > > Hi Dan, > > Your fix looks correct to me, but I'm curious if you observed this > problem on a device? The mmap in dma-buf.c looks like it prevents > creating a mapping that is too large, and I think an access beyond the > VMA should segfault before reaching here. This is from static analysis and not from testing. You could be correct that this bug can't affect real life. regards, dan carpenter
On Tue, Oct 3, 2023 at 1:30 AM Dan Carpenter <dan.carpenter@linaro.org> wrote: > > On Mon, Oct 02, 2023 at 10:16:24AM -0700, T.J. Mercier wrote: > > On Mon, Oct 2, 2023 at 12:04 AM Dan Carpenter <dan.carpenter@linaro.org> wrote: > > > > > > The buffer->pages[] has "buffer->pagecount" elements so this > comparison > > > has to be changed to >= to avoid reading beyond the end of the array. > > > The buffer->pages[] array is allocated in cma_heap_allocate(). > > > > > > Fixes: a5d2d29e24be ("dma-buf: heaps: Move heap-helper logic into the cma_heap implementation") > > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > > > --- > > > drivers/dma-buf/heaps/cma_heap.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c > > > index ee899f8e6721..bea7e574f916 100644 > > > --- a/drivers/dma-buf/heaps/cma_heap.c > > > +++ b/drivers/dma-buf/heaps/cma_heap.c > > > @@ -165,7 +165,7 @@ static vm_fault_t cma_heap_vm_fault(struct vm_fault *vmf) > > > struct vm_area_struct *vma = vmf->vma; > > > struct cma_heap_buffer *buffer = vma->vm_private_data; > > > > > > - if (vmf->pgoff > buffer->pagecount) > > > + if (vmf->pgoff >= buffer->pagecount) > > > return VM_FAULT_SIGBUS; > > > > > Hi Dan, > > > > Your fix looks correct to me, but I'm curious if you observed this > > problem on a device? The mmap in dma-buf.c looks like it prevents > > creating a mapping that is too large, and I think an access beyond the > > VMA should segfault before reaching here. > > This is from static analysis and not from testing. You could be correct > that this bug can't affect real life. > > regards, > dan carpenter Ok, thanks Dan. Reviewed-by: T.J. Mercier <tjmercier@google.com>
diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c index ee899f8e6721..bea7e574f916 100644 --- a/drivers/dma-buf/heaps/cma_heap.c +++ b/drivers/dma-buf/heaps/cma_heap.c @@ -165,7 +165,7 @@ static vm_fault_t cma_heap_vm_fault(struct vm_fault *vmf) struct vm_area_struct *vma = vmf->vma; struct cma_heap_buffer *buffer = vma->vm_private_data; - if (vmf->pgoff > buffer->pagecount) + if (vmf->pgoff >= buffer->pagecount) return VM_FAULT_SIGBUS; vmf->page = buffer->pages[vmf->pgoff];
The buffer->pages[] has "buffer->pagecount" elements so this > comparison has to be changed to >= to avoid reading beyond the end of the array. The buffer->pages[] array is allocated in cma_heap_allocate(). Fixes: a5d2d29e24be ("dma-buf: heaps: Move heap-helper logic into the cma_heap implementation") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- drivers/dma-buf/heaps/cma_heap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)