Skip to contentSkip to conversation

GenWQE Character device and DDCB queue

DISCUSSION Dan Carpenter 2 participants 3 messages Dec 20, 2013 – Dec 21, 2013 Kernel janitors

lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

Hello Frank Haverkamp,

The patch eaf4722d4645: "GenWQE Character device and DDCB queue" from Dec 9, 2013, leads to the following
static checker warning: "drivers/misc/genwqe/card_dev.c:657 do_flash_update()
warn: maybe return -EFAULT instead of the bytes remaining?"

drivers/misc/genwqe/card_dev.c
564 tocopy = min_t(size_t, load->size, FLASH_BLOCK);
565
566 rc = copy_from_user(xbuf, buf, tocopy);
567 if (rc) {
568 dev_err(&pci_dev->dev,
569 "err: could not copy all data rc=%d\n", rc);

There should be an "rc = -EFAULT" here. Also printing lots of user triggerable dev_errs() is a DoS vector. The caller doesn't check the error immediately; is that an information leak bug?

570 goto free_buffer;
571 }
572 crc = genwqe_crc32(xbuf, tocopy, 0xffffffff);
573
574 dev_info(&pci_dev->dev,
575 "[%s] DMA: 0x%llx CRC: %08x SZ: %ld %d\n",
576 __func__, dma_addr, crc, tocopy, blocks_to_flash);
577

regards,
dan carpenter

#1 lore raw

Hi Dan,

Am Samstag, den 21.12.2013, 00:01 +0300 schrieb Dan Carpenter:

Quoted 9 lines

Hello Frank Haverkamp,

The patch eaf4722d4645: "GenWQE Character device and DDCB queue" from Dec 9, 2013, leads to the following
static checker warning: "drivers/misc/genwqe/card_dev.c:657 do_flash_update()
warn: maybe return -EFAULT instead of the bytes remaining?"

drivers/misc/genwqe/card_dev.c

Quoted 8 lines
   564                  tocopy = min_t(size_t, load->size, FLASH_BLOCK);
   565  
   566                  rc = copy_from_user(xbuf, buf, tocopy);
   567                  if (rc) {
   568                          dev_err(&pci_dev->dev,
   569                                  "err: could not copy all data rc=%d\n", rc);

There should be an "rc = -EFAULT" here.

Yes, that is better. I will fix that.

Quoted 2 lines

Also printing lots of user
triggerable dev_errs() is a DoS vector.

Ok. Either I will remove or turn those into dev_dbg. Thanks for pointing this out.

Quoted 2 lines

The caller doesn't check the
error immediately; is that an information leak bug?

I think it is ok the way it is. The hardware returns retc, progress and attn. The user likes to have those in addition to rc to know why the call failed:

	rc = do_flash_update(cfile, &load);

	if (copy_to_user((void __user *)arg, &load, sizeof(load))) {
		dev_err(&pci_dev->dev,
			"err: could not copy params to user\n");
		return -EFAULT;
	}
	dev_info(&pci_dev->dev, "[%s] rc=%d\n", __func__, rc);
	return rc;

I will remove or convert the dev_info() calls too.

Quoted 10 lines
   570                          goto free_buffer;
   571                  }
   572                  crc = genwqe_crc32(xbuf, tocopy, 0xffffffff);
   573  
   574                  dev_info(&pci_dev->dev,
   575                           "[%s] DMA: 0x%llx CRC: %08x SZ: %ld %d\n",
   576                          __func__, dma_addr, crc, tocopy, blocks_to_flash);
   577
Quoted 2 lines

regards,
dan carpenter

Thanks for reviewing and checking our code.

Regards

Frank

author
#2 lore raw
In reply to Dan Carpenter
Quoted 4 lines · Frank Haverkamp
Quoted 2 lines

The caller doesn't check the
error immediately; is that an information leak bug?

I think it is ok the way it is.

Yes. You are right, of course. I didn't look at this carefully.

regards,
dan carpenter