On Tue, 2016-11-15 at 20:42 +0100, Stefan Wahren wrote:
> Hi Dan,
>
> > Dan Carpenter <dan.carpenter@xxxxxxxxxx> hat am 15. November 2016
> > um 14:15
> > geschrieben:
> >
> >
> > Hello popcornmix,
> >
> > The patch 71bad7f08641: "staging: add bcm2708 vchiq driver" from
> > Jul
> > 2, 2013, leads to the following static checker warning:
> >
> > drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c:1
> > 597
> > dump_phys_mem()
> > error: using offset into zero size array 'pages[]'
> >
> > drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > 1537 static void
> > 1538 dump_phys_mem(void *virt_addr, uint32_t num_bytes)
> > 1539 {
> > 1540 int rc;
> > 1541 uint8_t *end_virt_addr = virt_addr +
> > num_bytes;
> > 1542 int num_pages;
> > 1543 int offset;
> > 1544 int end_offset;
> > 1545 int page_idx;
> > 1546 int prev_idx;
> > 1547 struct page *page;
> > 1548 struct page **pages;
> > 1549 uint8_t *kmapped_virt_ptr;
> > 1550
> > 1551 /* Align virtAddr and endVirtAddr to 16 byte
> > boundaries. */
> > 1552
> > 1553 virt_addr = (void *)((unsigned long)virt_addr &
> > ~0x0fuL);
> > 1554 end_virt_addr = (void *)(((unsigned
> > long)end_virt_addr + 15uL)
> > &
> > 1555 ~0x0fuL);
> > 1556
> > 1557 offset = (int)(long)virt_addr & (PAGE_SIZE - 1);
> > 1558 end_offset = (int)(long)end_virt_addr & (PAGE_SIZE
> > - 1);
> > 1559
> > 1560 num_pages = (offset + num_bytes + PAGE_SIZE - 1) /
> > PAGE_SIZE;
> > 1561
> > 1562 pages = kmalloc(sizeof(struct page *) * num_pages,
> > GFP_KERNEL);
> >
> > The problem that the static checker is complaining about is that
> > num_pages * sizeof(void *) can overflow to zero leading to an Oops
> > later.
> >
> > But really shouldn't we just get rid of this whole function? Why
> > are
> > we dumping memory?? I understand that the RPI doesn't have an MMU
> > so we
> > perhaps don't care too much about security but still...
>
> we touched this topic here [1]
>
> [1] -
> http://lists.infradead.org/pipermail/linux-rpi-kernel/2016-November/0
> 04746.html
>
Today, I did some searching through the downstream tree and the first
time this appears to have been added was back in early 2012 with the
comment that the latest GPU version is being added to the ARM tree. So
I'm starting to think that maybe nothing actually uses this on the ARM
side. Perhaps this whole ioctl should be removed to do nothing except
return 0.
>From a security point of view, I'm wondering if the entire /dev node
should be under an independent build config option that is defaulted to
N. Possibly even the entire driver should be defaulted to N so that it
doesn't unintentionally end up in some kernel image where it doesn't
make sense. Raspbian has it's own security requirements, so they can
enable in the downstream config what makes sense to them.
I'm still interested to know more about the MMU statement. I would
think at least some of the RPI models have one because swapping appears
to be at least somewhat functional on the later models.
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
