[bug report] staging: add bcm2708 vchiq driver

[Michael Zoran]

On Tue, 2016-11-15 at 20:42 +0100, Stefan Wahren wrote:
> Hi Dan,
> 
> > Dan Carpenter <dan.carpenter at oracle.com> 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.