* re: add Packet hub driver for Topcliff Platform controller hub
@ 2013-01-07 9:02 Dan Carpenter
2013-01-08 10:30 ` Tomoya MORINAGA
2013-01-08 10:49 ` Arnd Bergmann
0 siblings, 2 replies; 6+ messages in thread
From: Dan Carpenter @ 2013-01-07 9:02 UTC (permalink / raw)
To: masa-korg
Cc: Arnd Bergmann, Greg Kroah-Hartman, linux-kernel, Tomoya MORINAGA
Hi Masayuki Ohtak,
The patch cf4ece53460c: "add Packet hub driver for Topcliff Platform
controller hub" from Sep 1, 2010, leads to the following warning:
drivers/misc/pch_phub.c:596 pch_phub_bin_write()
error: buffer overflow 'buf' 4096 <= 15359
Sorry my question is about an old patch. Smatch complains because we
only pass a PAGE_SIZE buffer to sysfs files so the test for
"if (count > PCH_PHUB_OROM_SIZE) {" makes it think we are overflowing.
In fact, count is never more than 4096 so there is no overflow, but I
also think that it means only the first 4096 bytes of the firmware gets
updated.
drivers/misc/pch_phub.c
560 static ssize_t pch_phub_bin_write(struct file *filp, struct kobject *kobj,
561 struct bin_attribute *attr,
562 char *buf, loff_t off, size_t count)
563 {
564 int err;
565 unsigned int addr_offset;
566 int ret;
567 ssize_t rom_size;
568 struct pch_phub_reg *chip =
569 dev_get_drvdata(container_of(kobj, struct device, kobj));
570
571 ret = mutex_lock_interruptible(&pch_phub_mutex);
572 if (ret)
573 return -ERESTARTSYS;
574
575 if (off > PCH_PHUB_OROM_SIZE) {
576 addr_offset = 0;
577 goto return_ok;
578 }
579 if (count > PCH_PHUB_OROM_SIZE) {
^^^^^^^^^^^^^^^^^^
This is 15359.
580 addr_offset = 0;
581 goto return_ok;
582 }
583
584 chip->pch_phub_extrom_base_address = pci_map_rom(chip->pdev, &rom_size);
585 if (!chip->pch_phub_extrom_base_address) {
586 err = -ENOMEM;
587 goto exrom_map_err;
588 }
589
590 for (addr_offset = 0; addr_offset < count; addr_offset++) {
591 if (PCH_PHUB_OROM_SIZE < off + addr_offset)
592 goto return_ok;
593
594 ret = pch_phub_write_serial_rom(chip,
595 chip->pch_opt_rom_start_address + addr_offset + off,
596 buf[addr_offset]);
^^^^^^^^^^^^^^^^
Smatch complains because "buf" is only 4096 bytes.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: add Packet hub driver for Topcliff Platform controller hub
2013-01-07 9:02 add Packet hub driver for Topcliff Platform controller hub Dan Carpenter
@ 2013-01-08 10:30 ` Tomoya MORINAGA
2013-01-08 11:38 ` Dan Carpenter
2013-01-08 10:49 ` Arnd Bergmann
1 sibling, 1 reply; 6+ messages in thread
From: Tomoya MORINAGA @ 2013-01-08 10:30 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Arnd Bergmann, Greg Kroah-Hartman, linux-kernel
Hi Dan,
On Mon, Jan 7, 2013 at 6:02 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> The patch cf4ece53460c: "add Packet hub driver for Topcliff Platform
> controller hub" from Sep 1, 2010, leads to the following warning:
> drivers/misc/pch_phub.c:596 pch_phub_bin_write()
> error: buffer overflow 'buf' 4096 <= 15359
>
> Sorry my question is about an old patch. Smatch complains because we
> only pass a PAGE_SIZE buffer to sysfs files so the test for
> "if (count > PCH_PHUB_OROM_SIZE) {" makes it think we are overflowing.
> In fact, count is never more than 4096 so there is no overflow, but I
> also think that it means only the first 4096 bytes of the firmware gets
> updated.
>
> drivers/misc/pch_phub.c
> 560 static ssize_t pch_phub_bin_write(struct file *filp, struct kobject *kobj,
> 561 struct bin_attribute *attr,
> 562 char *buf, loff_t off, size_t count)
> 563 {
> 564 int err;
> 565 unsigned int addr_offset;
> 566 int ret;
> 567 ssize_t rom_size;
> 568 struct pch_phub_reg *chip =
> 569 dev_get_drvdata(container_of(kobj, struct device, kobj));
> 570
> 571 ret = mutex_lock_interruptible(&pch_phub_mutex);
> 572 if (ret)
> 573 return -ERESTARTSYS;
> 574
> 575 if (off > PCH_PHUB_OROM_SIZE) {
> 576 addr_offset = 0;
> 577 goto return_ok;
> 578 }
> 579 if (count > PCH_PHUB_OROM_SIZE) {
> ^^^^^^^^^^^^^^^^^^
> This is 15359.
>
> 580 addr_offset = 0;
> 581 goto return_ok;
> 582 }
> 583
> 584 chip->pch_phub_extrom_base_address = pci_map_rom(chip->pdev, &rom_size);
> 585 if (!chip->pch_phub_extrom_base_address) {
> 586 err = -ENOMEM;
> 587 goto exrom_map_err;
> 588 }
> 589
> 590 for (addr_offset = 0; addr_offset < count; addr_offset++) {
> 591 if (PCH_PHUB_OROM_SIZE < off + addr_offset)
> 592 goto return_ok;
> 593
> 594 ret = pch_phub_write_serial_rom(chip,
> 595 chip->pch_opt_rom_start_address + addr_offset + off,
> 596 buf[addr_offset]);
> ^^^^^^^^^^^^^^^^
> Smatch complains because "buf" is only 4096 bytes.
>
I can understand your saying.
You mean just delete the following condition ?
579 if (count > PCH_PHUB_OROM_SIZE) {
Thanks.
--
ROHM Co., Ltd.
tomoya
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: add Packet hub driver for Topcliff Platform controller hub
2013-01-08 10:30 ` Tomoya MORINAGA
@ 2013-01-08 11:38 ` Dan Carpenter
0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2013-01-08 11:38 UTC (permalink / raw)
To: Tomoya MORINAGA; +Cc: Arnd Bergmann, Greg Kroah-Hartman, linux-kernel
On Tue, Jan 08, 2013 at 07:30:34PM +0900, Tomoya MORINAGA wrote:
> Hi Dan,
>
> On Mon, Jan 7, 2013 at 6:02 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > The patch cf4ece53460c: "add Packet hub driver for Topcliff Platform
> > controller hub" from Sep 1, 2010, leads to the following warning:
> > drivers/misc/pch_phub.c:596 pch_phub_bin_write()
> > error: buffer overflow 'buf' 4096 <= 15359
> >
> > Sorry my question is about an old patch. Smatch complains because we
> > only pass a PAGE_SIZE buffer to sysfs files so the test for
> > "if (count > PCH_PHUB_OROM_SIZE) {" makes it think we are overflowing.
> > In fact, count is never more than 4096 so there is no overflow, but I
> > also think that it means only the first 4096 bytes of the firmware gets
> > updated.
> >
> > drivers/misc/pch_phub.c
> > 560 static ssize_t pch_phub_bin_write(struct file *filp, struct kobject *kobj,
> > 561 struct bin_attribute *attr,
> > 562 char *buf, loff_t off, size_t count)
> > 563 {
> > 564 int err;
> > 565 unsigned int addr_offset;
> > 566 int ret;
> > 567 ssize_t rom_size;
> > 568 struct pch_phub_reg *chip =
> > 569 dev_get_drvdata(container_of(kobj, struct device, kobj));
> > 570
> > 571 ret = mutex_lock_interruptible(&pch_phub_mutex);
> > 572 if (ret)
> > 573 return -ERESTARTSYS;
> > 574
> > 575 if (off > PCH_PHUB_OROM_SIZE) {
> > 576 addr_offset = 0;
> > 577 goto return_ok;
> > 578 }
> > 579 if (count > PCH_PHUB_OROM_SIZE) {
> > ^^^^^^^^^^^^^^^^^^
> > This is 15359.
> >
> > 580 addr_offset = 0;
> > 581 goto return_ok;
> > 582 }
> > 583
> > 584 chip->pch_phub_extrom_base_address = pci_map_rom(chip->pdev, &rom_size);
> > 585 if (!chip->pch_phub_extrom_base_address) {
> > 586 err = -ENOMEM;
> > 587 goto exrom_map_err;
> > 588 }
> > 589
> > 590 for (addr_offset = 0; addr_offset < count; addr_offset++) {
> > 591 if (PCH_PHUB_OROM_SIZE < off + addr_offset)
> > 592 goto return_ok;
> > 593
> > 594 ret = pch_phub_write_serial_rom(chip,
> > 595 chip->pch_opt_rom_start_address + addr_offset + off,
> > 596 buf[addr_offset]);
> > ^^^^^^^^^^^^^^^^
> > Smatch complains because "buf" is only 4096 bytes.
> >
>
> I can understand your saying.
>
> You mean just delete the following condition ?
>
> 579 if (count > PCH_PHUB_OROM_SIZE) {
>
What I'm saying is that sysfs files can only be 4096 bytes (larger
on some arches with a larger PAGE_SIZE) and your firmware is larger
than that. It won't work.
regards,
dan carpenter
> Thanks.
>
> --
> ROHM Co., Ltd.
> tomoya
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: add Packet hub driver for Topcliff Platform controller hub
2013-01-07 9:02 add Packet hub driver for Topcliff Platform controller hub Dan Carpenter
2013-01-08 10:30 ` Tomoya MORINAGA
@ 2013-01-08 10:49 ` Arnd Bergmann
2013-01-08 11:48 ` Dan Carpenter
1 sibling, 1 reply; 6+ messages in thread
From: Arnd Bergmann @ 2013-01-08 10:49 UTC (permalink / raw)
To: Dan Carpenter
Cc: masa-korg, Greg Kroah-Hartman, linux-kernel, Tomoya MORINAGA
On Monday 07 January 2013, Dan Carpenter wrote:
> Sorry my question is about an old patch. Smatch complains because we
> only pass a PAGE_SIZE buffer to sysfs files so the test for
> "if (count > PCH_PHUB_OROM_SIZE) {" makes it think we are overflowing.
> In fact, count is never more than 4096 so there is no overflow, but I
> also think that it means only the first 4096 bytes of the firmware gets
> updated.
I think it works correctly as long as user space keeps writing into the
bin file when getting short write return (4096 bytes).
Arnd
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: add Packet hub driver for Topcliff Platform controller hub
2013-01-08 10:49 ` Arnd Bergmann
@ 2013-01-08 11:48 ` Dan Carpenter
2013-01-08 11:56 ` Arnd Bergmann
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2013-01-08 11:48 UTC (permalink / raw)
To: Arnd Bergmann
Cc: masa-korg, Greg Kroah-Hartman, linux-kernel, Tomoya MORINAGA
On Tue, Jan 08, 2013 at 10:49:14AM +0000, Arnd Bergmann wrote:
> On Monday 07 January 2013, Dan Carpenter wrote:
> > Sorry my question is about an old patch. Smatch complains because we
> > only pass a PAGE_SIZE buffer to sysfs files so the test for
> > "if (count > PCH_PHUB_OROM_SIZE) {" makes it think we are overflowing.
> > In fact, count is never more than 4096 so there is no overflow, but I
> > also think that it means only the first 4096 bytes of the firmware gets
> > updated.
>
> I think it works correctly as long as user space keeps writing into the
> bin file when getting short write return (4096 bytes).
>
Ah. Ok. I misunderstood. I've looked through it again and you're
right.
Sorry for the noise.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: add Packet hub driver for Topcliff Platform controller hub
2013-01-08 11:48 ` Dan Carpenter
@ 2013-01-08 11:56 ` Arnd Bergmann
0 siblings, 0 replies; 6+ messages in thread
From: Arnd Bergmann @ 2013-01-08 11:56 UTC (permalink / raw)
To: Dan Carpenter
Cc: masa-korg, Greg Kroah-Hartman, linux-kernel, Tomoya MORINAGA
On Tuesday 08 January 2013, Dan Carpenter wrote:
> On Tue, Jan 08, 2013 at 10:49:14AM +0000, Arnd Bergmann wrote:
> > On Monday 07 January 2013, Dan Carpenter wrote:
> > > Sorry my question is about an old patch. Smatch complains because we
> > > only pass a PAGE_SIZE buffer to sysfs files so the test for
> > > "if (count > PCH_PHUB_OROM_SIZE) {" makes it think we are overflowing.
> > > In fact, count is never more than 4096 so there is no overflow, but I
> > > also think that it means only the first 4096 bytes of the firmware gets
> > > updated.
> >
> > I think it works correctly as long as user space keeps writing into the
> > bin file when getting short write return (4096 bytes).
> >
>
> Ah. Ok. I misunderstood. I've looked through it again and you're
> right.
>
> Sorry for the noise.
No worries, the report looked totally plausible and I also had to look
up how bin files work.
Arnd
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-01-08 11:57 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-01-07 9:02 add Packet hub driver for Topcliff Platform controller hub Dan Carpenter
2013-01-08 10:30 ` Tomoya MORINAGA
2013-01-08 11:38 ` Dan Carpenter
2013-01-08 10:49 ` Arnd Bergmann
2013-01-08 11:48 ` Dan Carpenter
2013-01-08 11:56 ` Arnd Bergmann
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox