puzzle for puzzle lovers

puzzle for puzzle lovers

[Dan Carpenter]

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 and no
one knows what line is.

Weird huh?

But the code is ancient from before the git era so no one knows what
it's supposed to do.  Unless you are clever enough to solve this
mystery.

This code gets run during init so presumably it got tested often
ten years ago and it works.

regards,
dan carpenter

Re: FW: puzzle for puzzle lovers

[Grant Edwards]

> 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

Re: FW: puzzle for puzzle lovers

[Dan Carpenter]

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

Re: FW: puzzle for puzzle lovers

[Grant Edwards]

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