Re: [bug report] staging: add bcm2708 vchiq driver

[Eric Anholt]

Michael Zoran  writes:

> On Tue, 2016-11-15 at 22:04 -0500, Vince Weaver wrote:
>> On Tue, 15 Nov 2016, Michael Zoran wrote:
>> 
>> > 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.
>> 
>> All Raspberry Pi models have an MMU.  I'm not sure why anyone thinks 
>> otherwise.
>> 
>> Though just to be pedantic, you don't need an MMU to "swap".  In
>> general 
>> you do to "page".
>> 
>> Vince
>
> From the BCM2835 doc floating around the web the RPI should have 2
> MMUs.  One on the VC side and one on the ARM side.  Since we are being
> pedantic here, I'll ask the question this way.
>
> Do all RPIs has an enabled MMU on the ARM side and do all RPIs support
> paging?

Yes.  The MMU between ARM's virtual and ARM's physical memory addresses
is always present and is totally normal.  The second MMU maps between
ARM's physical and bus, and is too coarse to be of much use so it's
almost a 1:1 mapping.

When people talk about RPi lacking an MMU, they're talking about between
devices (such as the 3D engine, video decode/encode, VPU, etc.) and
memory.


signature.asc
Description: PGP signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [bug report] staging: add bcm2708 vchiq driver

[Phil Elwell]

On 16/11/2016 10:21, Dan Carpenter wrote:
> On Tue, Nov 15, 2016 at 10:04:05PM -0500, Vince Weaver wrote:
>> On Tue, 15 Nov 2016, Michael Zoran wrote:
>>
>>> 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.
>> All Raspberry Pi models have an MMU.  I'm not sure why anyone thinks
>> otherwise.
> No idea.  Someone told me that when I complained about a different
> security bug on RPI.
The ARM cores have MMUs between them and the SoC bus, but the VC4 VPU
and its peripherals do not, leading to the need for contiguous
allocations. Some VPU peripherals can only address 256MB of RAM, which
further constrains allocations.

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [bug report] staging: add bcm2708 vchiq driver

[Dan Carpenter]

On Tue, Nov 15, 2016 at 10:04:05PM -0500, Vince Weaver wrote:
> On Tue, 15 Nov 2016, Michael Zoran wrote:
> 
> > 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.
> 
> All Raspberry Pi models have an MMU.  I'm not sure why anyone thinks
> otherwise.

No idea.  Someone told me that when I complained about a different
security bug on RPI.

regards,
dan carpenter

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [bug report] staging: add bcm2708 vchiq driver

[Michael Zoran]

On Tue, 2016-11-15 at 22:04 -0500, Vince Weaver wrote:
> On Tue, 15 Nov 2016, Michael Zoran wrote:
> 
> > 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.
> 
> All Raspberry Pi models have an MMU.  I'm not sure why anyone thinks 
> otherwise.
> 
> Though just to be pedantic, you don't need an MMU to "swap".  In
> general 
> you do to "page".
> 
> Vince

From the BCM2835 doc floating around the web the RPI should have 2
MMUs.  One on the VC side and one on the ARM side.  Since we are being
pedantic here, I'll ask the question this way.

Do all RPIs has an enabled MMU on the ARM side and do all RPIs support
paging?
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [bug report] staging: add bcm2708 vchiq driver

[Vince Weaver]

On Tue, 15 Nov 2016, Michael Zoran wrote:

> 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.

All Raspberry Pi models have an MMU.  I'm not sure why anyone thinks 
otherwise.

Though just to be pedantic, you don't need an MMU to "swap".  In general 
you do to "page".

Vince
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [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  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  intrc;
> >   1541  uint8_t   *end_virt_addr = virt_addr +
> > num_bytes;
> >   1542  intnum_pages;
> >   1543  intoffset;
> >   1544  intend_offset;
> >   1545  intpage_idx;
> >   1546  intprev_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
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [bug report] staging: add bcm2708 vchiq driver

[Stefan Wahren]

Hi Dan,

> Dan Carpenter  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:1597
> 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  intrc;
>   1541  uint8_t   *end_virt_addr = virt_addr + num_bytes;
>   1542  intnum_pages;
>   1543  intoffset;
>   1544  intend_offset;
>   1545  intpage_idx;
>   1546  intprev_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/004746.html
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[bug report] staging: add bcm2708 vchiq driver

[Dan Carpenter]

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:1597 
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  intrc;
  1541  uint8_t   *end_virt_addr = virt_addr + num_bytes;
  1542  intnum_pages;
  1543  intoffset;
  1544  intend_offset;
  1545  intpage_idx;
  1546  intprev_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...

  1563  if (pages == NULL) {
  1564  vchiq_log_error(vchiq_arm_log_level,
  1565  "Unable to allocation memory for %d pages\n",
  1566  num_pages);
  1567  return;
  1568  }
  1569  

regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel