hvconsole.c : possible problem

[Benjamin Herrenschmidt]

Hi !

I've been reviewing arch/ppc64/kernel/hvconsole.c from ameslab tree
and I have a few comments:

 - The use of static variables names "write, read, ring" is a bit
annoying for anybody trying to debug since they will appear "as-is"
in System.map. Even for static variables, it's useful to have a prefix.

 - There is a potential buffer overflow, I think, in hvsi_load_chunk(),
in this line :

	/* copy up to 16 bytes into the write buffer */
	chunklen = hvsi_read(vtty, write->data.pkt + write->got, 16);

The packet is defined to be 256 bytes. Now, if "got" is 254 bytes for
example and we call that routine, I let you figure out what happens. It
may not be possible for valid packets but I suppose an attacker can
always forge something.

Also, you load the ring buffer without limit, that means you can
eventually overflow and reuse a buffer that is filled with data
instead of breaking out. The whole function seems dodgy, it should
rather ask for the exact amount it can fit in the packet, not more,
and get rid of the overflow thing, don't you think ?

A much simpler loop basically, asking only as much is needed to fill
the header if that isn't filled, then as much is needed to fill the
data (maxed out at 16) until the packet is filled. That would allow
to always deal only with packet boundaries, let throttling work properly
at HV level, and simplify the code significantly...


 - There is historical hunt for null characters in hvterm_get_chars(),
that paulus added because xmon was getting confused. Where do those NULL
chars come from ? I doubt HV itself is adding anything to the stream so
I suppose it's whatever console program sits at the other hand ? I'm
asking because hvsi_read() doesn't do that, and so we end up with 2
routines having the exact same prototype: hvsi_read() and
hvc_hvterm_get_chars(), both calling the same HV routine, but not
doing exactly the same thing and I was wondering if this inconsistency
was still necessary.


 - The whole wait_for_packet() is completely synchronous (doesn't
schedule), that doesn't sound very good.

That's all for now ;)

Cheers,
BenH.


** Sent via the linuxppc64-dev mail list. See http://lists.linuxppc.org/