[U-Boot] Bug in netconsole?

[U-Boot] Bug in netconsole?

[Nick Thompson]

I think there might be a bug in this commit:

http://git.denx.de/cgi-bin/gitweb.cgi?p=u-boot.git;a=commitdiff;h=2c8fe5120f8da013cbd789be2f10cce880972836

The commit makes "the netconsole buffer size configurable". It adds CONFIG_NETCONSOLE_BUFFER_SIZE and maintains the original 512 default value used to define the length of input_buffer[]. nc_input_packet uses sizeof this to read packet data into input_buffer[]. This appears fine.

The commit also adds to following in the output chain:

@@ -214,7 +218,7 @@ static void nc_puts(const char *s)
 
        len = strlen(s);
        while (len) {
-               int send_len = min(len, 512);
+               int send_len = min(len, sizeof(input_buffer));
                nc_send_packet(s, send_len);
                len -= send_len;
                s += send_len;

I can't see how this code relates to the sizeof input_buffer. The nc_puts data is written directly into NetTxPacket (plus header offsets) which is set to 1536 + alignment bytes long. If input_buffer is bigger than this, a buffer overflow will occur. Obviously the default value of 512 will not trigger the problem. The 512 magic number possibly ought to be derived from PKTSIZE_ALIGN (net.h), but I don't think sizeof(input_buffer) is appropriate here.

Regards,
Nick.

[U-Boot] Bug in netconsole?

[Joe Hershberger]

Hi Nick,

On Wed, Nov 14, 2012 at 3:59 AM, Nick Thompson <nick.thompson@ge.com> wrote:
> I think there might be a bug in this commit:
>
> http://git.denx.de/cgi-bin/gitweb.cgi?p=u-boot.git;a=commitdiff;h=2c8fe5120f8da013cbd789be2f10cce880972836
>
> The commit makes "the netconsole buffer size configurable". It adds CONFIG_NETCONSOLE_BUFFER_SIZE and maintains the original 512 default value used to define the length of input_buffer[]. nc_input_packet uses sizeof this to read packet data into input_buffer[]. This appears fine.
>
> The commit also adds to following in the output chain:
>
> @@ -214,7 +218,7 @@ static void nc_puts(const char *s)
>
>         len = strlen(s);
>         while (len) {
> -               int send_len = min(len, 512);
> +               int send_len = min(len, sizeof(input_buffer));
>                 nc_send_packet(s, send_len);
>                 len -= send_len;
>                 s += send_len;
>
> I can't see how this code relates to the sizeof input_buffer. The nc_puts data is written directly into NetTxPacket (plus header offsets) which is set to 1536 + alignment bytes long. If input_buffer is bigger than this, a buffer overflow will occur. Obviously the default value of 512 will not trigger the problem. The 512 magic number possibly ought to be derived from PKTSIZE_ALIGN (net.h), but I don't think sizeof(input_buffer) is appropriate here.

I agree with your assessment based on this email (though I haven't dug
into the code yet to fix it).  Thanks for the report.

-Joe