> On Oct 25, 2018, at 8:44 AM, Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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.
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
