Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix buffer underflow when serial peer spews garbage #10

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

a3f
Copy link
Member

@a3f a3f commented Sep 14, 2018

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

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.

Signed-off-by: Ahmad Fatoum <[email protected]>
@ukleinek
Copy link
Contributor

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

@a3f
Copy link
Member Author

a3f commented May 4, 2019

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

@ukleinek
Copy link
Contributor

ukleinek commented May 4, 2019

@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 added a commit to a3f/microcom that referenced this pull request May 4, 2019
If microcom crashes, e.g. due to the buffer underflow described in pengutronix#10,
it may leave a stale lock file behind. Teach microcom detection of
these stale lock files.

Note that this is racy, two processes may detect a stale lock file
and one might unlink the other's lock.

Signed-off-by: Ahmad Fatoum <[email protected]>
a3f added a commit to a3f/microcom that referenced this pull request May 4, 2019
If microcom crashes, e.g. due to the buffer underflow described in pengutronix#10,
it may leave a stale lock file behind. Teach microcom detection of
these stale lock files.

Signed-off-by: Ahmad Fatoum <[email protected]>
a3f added a commit to a3f/microcom that referenced this pull request Aug 25, 2019
If microcom crashes, e.g. due to the buffer underflow described in pengutronix#10,
it may leave a stale lock file behind. Teach microcom detection of
these stale lock files.

Signed-off-by: Ahmad Fatoum <[email protected]>
@a3f
Copy link
Member Author

a3f commented Aug 25, 2019

@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
Copy link
Contributor

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 ukleinek added the invalid This doesn't seem right label Aug 25, 2019
@ukleinek
Copy link
Contributor

ukleinek commented Jun 9, 2023

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants