Re: [gpsd-dev] possible driver_ubx runtime bug introduced

[Eric S. Raymond]

Jon Schlueter <address@hidden>:
> I was looking back through commits and noticed that the ubx_write
> parameters changed even though the commit should have been just a
> coverty warning fixup.  This change could result in a very subtle run
> time bug in probing/setting up a UBX gps.  I don't have one to check
> with but thought I should bring it up on the list.
> 
> Jon Schlueter
> 
> 9d7dc31b595b91aeb258faca459909b0b5b7965e
> 
> @@ -714,7 +714,7 @@ static bool ubx_speed(struct gps_device_t *session,
>       usart_mode |= 0x2000;   /* zero value means 1 stop bit */
>      putle32(buf, 4, usart_mode);
>      putle32(buf, 8, speed);
> -    (void)ubx_write(session, 0x06, 0x00, &buf[6], 20);       /* send back
> with all other settings intact */
> +    (void)ubx_write(session, 0x06, 0x00, buf, sizeof(buf));  /* send
> back with all other settings intact */
>      /*@ -charint +usedef +compdef */
>      return true;
>  }
> 
> also
> 
> dbf6d322945e68fce36afd6dba75ab56d60b0c86
> 
> @@ -676,7 +675,7 @@ static void ubx_nmea_mode(struct gps_device_t
> *session, int mode)
>       buf[14] |= 0x01;        /* turn on UBX output on this port */
>      }
>      /*@ -charint +usedef @*/
> -    (void)ubx_write(session, 0x06u, 0x00, &buf[6], 20);      /* send back
> with all other settings intact */
> +    (void)ubx_write(session, 0x06u, 0x00, buf, sizeof(buf)); /* send
> back with all other settings intact */
>  }

This semantic change was intentional.  If you look at the definition of buf,

    unsigned char buf[sizeof(session->driver.ubx.original_port_settings)];

and then at the definition of session->driver.ubx.original_port_settings) in
gpsd.h

    unsigned char original_port_settings[20];

it's only 20 characters long.  Thus, the old code would skip 6 chars
from the start of the buffer and write 6 characters from the stack
past the end of the buf auto variable.  This what Coverity rightly
identified as a static buffer overrun.

This code has been broken for a long time.  There's a bug in the TODO
noting that UBX mode change doesn't work and I'm pretty certain this
is why.

I think what happened here was confusion between whole messages and
payloads.  uBlox messages have a six-character prefix (2 header bytes,
message class, message type, 2 bytes of payload length).  The mode-get
response and mode-set messages have 20-character payloads.  

The original-port-settings buffer is supposed to get filled by the
UBX_CFG_PRT response to a query message set when the device is
activated.  What the mode-change and speed-change messages are supposed to 
be doing is taking a copy of those 20 characters of payload, tweaking
some bits, and shipping that.  

The problem Coverity spotted is that the send code was addressing
that buffer as if it were whole-message, not payload!

Now that I look at this again, I think it's still buggy in a subtle way
that would only show up under repeated mode changes.  We shouldn't be
saving *original* settings but *current* settings, and there should be
no buffer copying - we should always tweak the current-settings buffer
and then ship it as payload.  With the present code, two successive
mode or speed changes could restore unwanted state fom the original
settings.

Easily enough fixed.  I'll test this, if my new uBlox-based Macx-1 
does binary mode.
-- 
                <a href="http://www.catb.org/~esr/";>Eric S. Raymond</a>