* Re: FW: puzzle for puzzle lovers
2011-10-04 7:26 puzzle for puzzle lovers Dan Carpenter
@ 2011-10-04 16:16 ` Grant Edwards
2011-10-04 16:55 ` Dan Carpenter
2011-10-04 17:14 ` Grant Edwards
2 siblings, 0 replies; 4+ messages in thread
From: Grant Edwards @ 2011-10-04 16:16 UTC (permalink / raw)
To: kernel-janitors
> Subject: puzzle for puzzle lovers
> Date: Tuesday 04 October 2011
> From: Dan Carpenter <dan.carpenter@oracle.com>
> To: kernel-janitors@vger.kernel.org
>
> Here is something that might amuse someone.
>
> Smatch reports a read past the end of the array in rocket.c
> drivers/tty/rocket.c +2168 init_ISA(77)
> error: buffer overflow calling init_r_port. param 0. 7 >= 4
>
> drivers/tty/rocket.c
> 657 init_completion(&info->close_wait);
> 658 info->flags &= ~ROCKET_MODE_MASK;
> 659 switch (pc104[board][line]) {
> 660 case 422:
> 661 info->flags |= ROCKET_MODE_RS422;
> 662 break;
>
> pc104[] is a 4 element array.
>
> board comes from for loop in rp_init().
>
> 2315 for (i = 0; i < NUM_BOARDS; i++) {
> 2316 if (init_ISA(i))
> 2317 isa_boards_found++;
> 2318 }
>
> NUM_BOARDS is is 8, so according to Smatch "board" can be 7
At that point in the code, 'board' can only be 0-3. The init_ISA(i)
function will return immediately if isa_io_addr[i] is 0. isa_io_addr[]
is a static 8-element array and only the first 4 elements are ever
assigned to. Therefore, isa_io_addr[4] is guaranteed to be 0, and the
loop above is never called with 'board' greater than 3.
> and no one knows what line is.
'line' is the serial port index within that board (e.g. 0-7 on a
8-port board, 0-31 on a 32-port board).
> Weird huh?
pc104 is a 4-element array of pointers. Those pointers point to
8-element arrays. 'board' can be 0-3, and 'line' can be 0-7.
The problem is that there are 32-port ISA boards, so I think pc104_1
through pc104_4 should be 32-element arrays. I suspect this hasn't
caused any failures because the flags set based on pc104[board][line]
are only used in the case of PC104 boards, and PC104 boards never have
more than 8 ports.
IOW, for boards with more than 8 ports, there is a read-past-end-of-array
but the result isn't used. Hmm -- there may be one case where the
bogus result _is_ used but it would take some work to figure out if
that happens or not. It would be better to just change pc104_1
through pc104_4 from 8-element arrays to 32-element arrays.
> But the code is ancient from before the git era so no one knows what
> it's supposed to do.
I can explain "it" to you if you specify to what "it" refers.
> Unless you are clever enough to solve this mystery.
If it would make people happy, feel free to make pc104[] a
MAX_NUMBEROF_BOARDS sized array. It's not possible to configure more
than 4 ISA boards, but if the objective is to make a static code
analyzer happy, then go ahead.
> This code gets run during init so presumably it got tested often
> ten years ago and it works.
Diclaimer: Comtrol offers no support for the in-tree driver. The
driver we officially support is at ftp://ftp.comtrol.com/html/default.htm
--
Grant Edwards
grant.edwards@comtrol.com
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: FW: puzzle for puzzle lovers
2011-10-04 7:26 puzzle for puzzle lovers Dan Carpenter
2011-10-04 16:16 ` FW: " Grant Edwards
@ 2011-10-04 16:55 ` Dan Carpenter
2011-10-04 17:14 ` Grant Edwards
2 siblings, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2011-10-04 16:55 UTC (permalink / raw)
To: kernel-janitors
Wow. Thanks for your response.
Kernel-janitors is sort of a newbie list, and people email me often
ask for something to look at. As I mentioned in my email, I was
pretty sure the code worked. But I thought it would make a good code
reading exercise for someone.
But you _nailed_ it. You nailed it to a basketball and then slam
dunked it!
Btw, I'm sorry if someone forwarded this to you as a support request.
I didn't mean to bother the maintainers with this.
regards,
dan carpeneter
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: FW: puzzle for puzzle lovers
2011-10-04 7:26 puzzle for puzzle lovers Dan Carpenter
2011-10-04 16:16 ` FW: " Grant Edwards
2011-10-04 16:55 ` Dan Carpenter
@ 2011-10-04 17:14 ` Grant Edwards
2 siblings, 0 replies; 4+ messages in thread
From: Grant Edwards @ 2011-10-04 17:14 UTC (permalink / raw)
To: kernel-janitors
On Tue, Oct 04, 2011 at 07:55:44PM +0300, Dan Carpenter wrote:
> Wow. Thanks for your response.
>
> Kernel-janitors is sort of a newbie list, and people email me often
> ask for something to look at. As I mentioned in my email, I was
> pretty sure the code worked.
I'm pretty sure it worked at some point in the past. :)
I don't know that any of the kernel maintainers have hardware to test
with, and we concentrate on our version of the driver that supports a
wide range of kernel versions.
> But I thought it would make a good code reading exercise for someone.
Parts of that driver are actually based on what was originally a
library of DOS code from 25 years ago, so some of it is rather oddly
structured.
> But you _nailed_ it. You nailed it to a basketball and then slam
> dunked it!
>
> Btw, I'm sorry if someone forwarded this to you as a support request.
Somebody here just happened to notice it and thought I might find it
interesting. I only put that disclaimer on there because I copied the
mailing list and I assume it would be publically visible.
> I didn't mean to bother the maintainers with this.
I checked our supported driver, and it has the same problem with
pc104_1[] through pc104_4[] being 8 elements when they need to be 32.
Depending on what value get's grabbed off the ends of those arrays, I
think it might potentially cause a problem for 16- and 32-port ISA
boards. I'm going to fix that in our driver, though I don't know how
many ISA boards are still in use out there. :)
--
Grant
^ permalink raw reply [flat|nested] 4+ messages in thread