[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