pppd: Fix potential buffer overflow in lcp_rtt_update_buffer()

[nomis]

It's possible for ring_header[2] to be modified by another process when reading it twice through a volatile pointer, causing it to change from a small value (which doesn't need to wrap around) to a large value which would exceed the size of the buffer.

[nomis]

There are other things I don't like about this file format:

• Timestamps are in truncated seconds so the data could contain the same value twice and/or skip a second
• There's no synchronisation at all and data is written in an unsafe order, so it's impossible to read safely

These can't be fixed without changing the file format making it incompatible with any existing user

I have my own implementation that uses a socket instead of a file, I just need to rewrite it to be more generic

[jkroonza]

I don't like this mechanism at all. I do approve of the change though. Other comments about this below.

I'd personally use a struct to overlay onto the mmap()'ed memory, eg, something like:

struct rtt_file_header {
   uint32_t magic; /* might have two magics, one for "old" format, and one for "new", with an option which to use */
   uint32_t status;
   uint32_t index;
   uint32_t interval;
   struct {
      time_t time; /* heads up: this could be 64 bits, might as well use struct timeval to sort your concern of the timestamps */
      uint8_t lost;
      uint32_t rtt:24; /* needs to be verified */
   } ring[LCP_RTT_ELEMENTS] __attribute__((packed));
};

If two formats is made, then the ring array at the end should be a union of two structs, eg, ring_0 (old format) and ring_1 (new format). Naming it _0 and _1 assumes that we will possibly update it again later to _2. This does complicate the ftruncate option in file open.

If you reed the comments about msync() just below the code you changed, it looks more like you need an msync() after each write to the memory region, including the ring header initilization code. Specifically:

2303      * This means that a process reading the file could see the index
2304      * having been updated before the element that the index points to had
2305      * been written.

Thus the issue you're describing is known. I recommend you rather add an msync() between ring_bugger[next_entry + 1] = ... and ring_header[2] = ... as well. This will at least fix the synchronization problem you mentioned, not sure MS_ASYNC is good enough for that, probably either MS_SYNC and/or MS_INVALIDATE.

[nomis]

I will not be attempting to fix the file format or the synchronisation.

The underlying problem is that all writes to the file will be visible immediately to other processes in the shared memory used for the filesystem cache. The msync() call can only affect the content on storage (but this file should be in tmpfs so there isn't any).

I don't want to add locking because that introduces a risk of blocking pppd indefinitely.

[jkroonza]

I do agree with the change. Not that any other processes SHOULD be writing to the file. But if you have the same file for multiple pppd processes, then at least this will stop the buffer overflow.