[bug report] staging: vc04_services: add vchiq_pagelist_info structure

Dan Carpenter dan.carpenter at oracle.com
Thu Oct 25 13:44:34 UTC 2018


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


More information about the devel mailing list