Fix buffer underflow when serial peer spews garbage

[a3f]

A board I got spews garbage on hard reset. Sometimes that garbage is IAC and it leads to len here getting negative and the loop eventually underflows the buf.

Detect and avoid this silently.

==7021==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fffcab01d20 at pc 0x5599cefcc003 bp 0x7fffcab017e0 sp 0x7fffcab017d8
READ of size 1 at 0x7fffcab01d20 thread T0
    #0 0x5599cefcc002 in handle_receive_buf /home/afa/dl/microcom/mux.c:310
    #1 0x5599cefccc15 in mux_loop /home/afa/dl/microcom/mux.c:420
    #2 0x5599cefc971e in main /home/afa/dl/microcom/microcom.c:330
    #3 0x7f2eb7d752e0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x202e0)
    #4 0x5599cefc3419 in _start (/home/afa/dl/microcom/microcom+0x17419)

Address 0x7fffcab01d20 is located in stack of thread T0 at offset 1216 in frame
    #0 0x5599cefcc529 in mux_loop /home/afa/dl/microcom/mux.c:397

  This frame has 2 object(s):
    [32, 160) 'ready'
    [192, 1216) 'buf' <== Memory access at offset 1216 overflows this variable
HINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-buffer-overflow /home/afa/dl/microcom/mux.c:310 in handle_receive_buf
Shadow bytes around the buggy address:
  0x100079558350: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100079558360: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100079558370: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100079558380: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100079558390: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x1000795583a0: 00 00 00 00[f3]f3 f3 f3 00 00 00 00 00 00 00 00
  0x1000795583b0: 00 00 00 00 00 00 00 00 f1 f1 f1 f1 00 00 00 00
  0x1000795583c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 f4
  0x1000795583d0: f2 f2 f2 f2 00 00 00 00 00 00 00 00 00 00 00 00
  0x1000795583e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1000795583f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack partial redzone:   f4
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==7021==ABORTING

[ukleinek]

Without having looked deeply, I suspect this is the same bug as #4 which needs more work.

[a3f]

I agree it needs more work, but could this be at least merged to avoid the segfaults?

[ukleinek]

@a3f I authored a fix for #4 which looks better in my eyes. Would you mind looking at https://github.com/ukleinek/microcom/tree/issue-4 and check if this resolves your problem?

[a3f]

@ukleinek Haven't tested it, but looks good to me. I still think you should take my patch as well, because the underlying issue is still there. A buggy telnet server could still send the exact same sequence of IAC's and overflow the stack.

[ukleinek]

What should the malicious server send to trigger the bug?

I have the impression that your commit only papers over the problem but doesn't really fix it.

There is only a difference if len gets negative in either write_receive_buf or handle_receive_buf.

I fail to see how this could happen for write_receive_buf. For handle_receive_buf it could happen that handle_command consumes more chars than available (and thus interpreting the data that happens to just be in the buffer). This is what should be actually fixed instead.

[ukleinek]

@a3f Do you still see a way what a remote telnet server could send to trigger a buffer problem? If not, I'd close this PR.