[bug report] staging: vc04_services: add vchiq_pagelist_info structure

Michael Zoran mzoran at crowfest.net
Thu Oct 25 14:02:38 UTC 2018



> On Oct 25, 2018, at 8:44 AM, Dan Carpenter <dan.carpenter at oracle.com> wrote:
> 
> Hello Michael Zoran,
> 
> The patch 4807f2c0e684: "staging: vc04_services: add
> vchiq_pagelist_info structure" from Nov 7, 2016, leads to the
> following static checker warning:
> 
> 	drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c:440 create_pagelist()
> 	warn: overflowed symbol reused:  'count'
> 
> drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
>   398  static struct vchiq_pagelist_info *
>   399  create_pagelist(char __user *buf, size_t count, unsigned short type)
>                                          ^^^^^^^^^^^^
> This comes from the user from the ioctl.  It starts as an int but gets
> cast to unsigned long.
> 
>   400  {
>   401          PAGELIST_T *pagelist;
>   402          struct vchiq_pagelist_info *pagelistinfo;
>   403          struct page **pages;
>   404          u32 *addrs;
>   405          unsigned int num_pages, offset, i, k;
>   406          int actual_pages;
>   407          size_t pagelist_size;
>   408          struct scatterlist *scatterlist, *sg;
>   409          int dma_buffers;
>   410          dma_addr_t dma_addr;
>   411  
>   412          offset = ((unsigned int)(unsigned long)buf & (PAGE_SIZE - 1));
>   413          num_pages = DIV_ROUND_UP(count + offset, PAGE_SIZE);
>                                         ^^^^^^^^^^^^^^^^^^^^^^^^^
> Which means that this can definitely overflow.  I can happen in two
> ways.  First the "count + offset" can overflow.  Then the DIV_ROUND_UP
> starts by adding PAGE_SIZE to the result and that addition can overflow.
> 
> The problem with handling integer overflow issues from a static analysis
> perspective is that many integer overflows are not harmful.  Often we
> maybe we get a smaller num_pages from what we had intended but it doesn't
> cause a problem because we never reference "count" again.  This warning
> message is supposed to solve that problem by warning by only complaining
> when we re-use count.
> 
>   414  
>   415          pagelist_size = sizeof(PAGELIST_T) +
>   416                          (num_pages * sizeof(u32)) +
>   417                          (num_pages * sizeof(pages[0]) +
>   418                          (num_pages * sizeof(struct scatterlist))) +
>   419                          sizeof(struct vchiq_pagelist_info);
>   420  
>   421          /* Allocate enough storage to hold the page pointers and the page
>   422           * list
>   423           */
>   424          pagelist = dma_zalloc_coherent(g_dev,
>   425                                         pagelist_size,
>   426                                         &dma_addr,
>   427                                         GFP_KERNEL);
>   428  
>   429          vchiq_log_trace(vchiq_arm_log_level, "%s - %pK", __func__, pagelist);
>   430  
>   431          if (!pagelist)
>   432                  return NULL;
>   433  
>   434          addrs           = pagelist->addrs;
>   435          pages           = (struct page **)(addrs + num_pages);
>   436          scatterlist     = (struct scatterlist *)(pages + num_pages);
>   437          pagelistinfo    = (struct vchiq_pagelist_info *)
>   438                            (scatterlist + num_pages);
>   439  
>   440          pagelist->length = count;
>                ^^^^^^^^^^^^^^^^^^^^^^^^^
> So it complains here.  But then when I look at it more, it's not clear
> to me if it matters that "pagelist->length" is out of sync with
> "num_pages"...
> 
> Anyway, that's why I'm forwarding this static checker output to you
> instead of trying to fix it myself.
> 
>   441          pagelist->type = type;
>   442          pagelist->offset = offset;
> 
> regards,
> dan carpenter


Hi Dan,

I no longer have any PI/RPI boards anymore.   I gave my boards away to someone who was in desperate need of really a cheap computer. So It’s not clear to me how I can help much or be able to locally test changes anymore.  I’ve also been spending my time on other things these days.

I was mostly interested in getting arm64 to work on the PI 3, which I essentially accomplished.   Although I isn’t clear 64 bit has much use on the PI 3, it was a great learning experience anyway. I learned a lot about linux kernel development and the processes involved in submitting patches.

As a final note, this driver has many of these issues.  I know that fixing static errors like this is important, but due to the nature of the driver itself I’m am not sure changes like this have much gain.






More information about the devel mailing list