Bug 202326 - libteken assert() fail and result in kernel panic
Summary: libteken assert() fail and result in kernel panic
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: FreeBSD bugs mailing list
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-08-14 16:22 UTC by kcwu
Modified: 2015-09-14 09:32 UTC (History)
3 users (show)

See Also:


Attachments
test cases (334 bytes, application/x-compressed)
2015-08-14 16:22 UTC, kcwu
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description kcwu 2015-08-14 16:22:19 UTC
Created attachment 159862 [details]
test cases

Because syscons (a kernel driver) uses libteken, an assertion failure in libteken would result in kernel panic.

Please see the attach files.
To reproduce:
1. switch to console
2. cat teken-*
-> kernel panic

teken-104 could trigger assert() fail in teken.c line 104.
teken-106 for line 106, and so on.

Depends on terminal state, not all of them always trigger panics. To reproduce the assertions reliably, you can feed those files to teken_input() directly like src/sys/teken/stress/teken_stress.c does.


This is very low risk. However, this may be used for DoS attack by combining other flaws.

This issue is found by afl-fuzz
Comment 1 commit-hook freebsd_committer 2015-08-15 08:43:04 UTC
A commit references this bug:

Author: ed
Date: Sat Aug 15 08:42:33 UTC 2015
New revision: 286798
URL: https://svnweb.freebsd.org/changeset/base/286798

Log:
  Stop parsing digits if the value already exceeds USHRT_MAX.

  There is no need for us to support parsing values that are larger than
  the maximum terminal window size. In this case that would be the maximum
  of unsigned short.

  The problem with parsing larger values is that they can cause integer
  overflows when adjusting the cursor position, leading to all sorts of
  failing assertions.

  PR:		202326
  Reported by:	kcwu csie org
  MFC after:	1 month

Changes:
  head/sys/teken/teken.c
Comment 2 Ed Schouten freebsd_committer 2015-08-15 08:44:44 UTC
Hi there!

That's awesome! The idea behind turning the terminal emulator into a library was to exactly make things like fuzzing easier. I just fixed all of these test cases in r286798 and will MFC this change in a month from now.

Be sure to continue fuzzing libteken. I'll make sure to address any issues that you can uncover.

Thanks,
Ed
Comment 3 kcwu 2015-08-15 16:52:22 UTC
Thanks for your quick response. I confirmed r286798 fixed all my local crash test cases. I will continue fuzzing and let you know if I found anything.
Comment 4 kcwu 2015-08-15 18:45:50 UTC
BTW, IIUC, your fix assumed sizeof(unsigned short) < sizeof(unsigned int), which is not always true in theory. Not sure is this an issue or not (somebody might take libteken to use in 16bit processor someday?).
Comment 5 Ed Schouten freebsd_committer 2015-08-16 13:09:50 UTC
That's a good point. I was merely focussing on cases where short is 16 bits and integers are >= 32 bits.

Do you think using something like `UINT_MAX / k` is a better idea? If we were to pick k == 100, the possible value would top out at approximately `UINT_MAX / 10` and I think it would be pretty safe to perform arithmetic on that.
Comment 6 kcwu 2015-08-16 13:46:29 UTC
Sounds good to me
Comment 7 commit-hook freebsd_committer 2015-08-16 13:59:49 UTC
A commit references this bug:

Author: ed
Date: Sun Aug 16 13:59:12 UTC 2015
New revision: 286827
URL: https://svnweb.freebsd.org/changeset/base/286827

Log:
  Pick UINT_MAX / 100 as an upperbound.

  The fix that I applied in r286798 is already good, but it assumes that
  sizeof(int) > sizeof(short). Express the upperbound in terms of
  UINT_MAX. By dividing that by 100, we're sure that the resulting value
  is never larger than approximately UINT_MAX / 10, which is safe.

  PR:		202326
  Discussed with:	kcwu csie org
  MFC after:	1 month

Changes:
  head/sys/teken/teken.c
Comment 8 commit-hook freebsd_committer 2015-09-14 09:13:08 UTC
A commit references this bug:

Author: ed
Date: Mon Sep 14 09:12:29 UTC 2015
New revision: 287776
URL: https://svnweb.freebsd.org/changeset/base/287776

Log:
  MFC r286798 and r286827:

    Stop parsing digits if the value already exceeds UINT_MAX / 100.

    There is no need for us to support parsing values that are larger than
    the maximum terminal window size. In this case that would be the maximum
    of unsigned short.

    The problem with parsing larger values is that they can cause integer
    overflows when adjusting the cursor position, leading to all sorts of
    failing assertions.

  MFC r286981 and r287098:

    Don't truncate cursor arithmetic to 16 bits.

    When updating the row number when the cursor position escape sequence is
    issued, we should make sure to store the intermediate result in a 32-bit
    integer. If we fail to do this, the cursor may be set above the origin
    region, which is bad.

    This could cause libteken to crash when INVARIANTS is enabled, due to
    the strict set of assertions that libteken has.

  PR:		202326, 202540, 202612
  Submitted by:	kwcu csie org

Changes:
_U  stable/10/
  stable/10/sys/teken/teken.c
  stable/10/sys/teken/teken_subr.h
Comment 9 commit-hook freebsd_committer 2015-09-14 09:31:14 UTC
A commit references this bug:

Author: ed
Date: Mon Sep 14 09:31:01 UTC 2015
New revision: 287777
URL: https://svnweb.freebsd.org/changeset/base/287777

Log:
  MFC r286798 and r286827:

    Stop parsing digits if the value already exceeds UINT_MAX / 100.

    There is no need for us to support parsing values that are larger than
    the maximum terminal window size. In this case that would be the maximum
    of unsigned short.

    The problem with parsing larger values is that they can cause integer
    overflows when adjusting the cursor position, leading to all sorts of
    failing assertions.

  MFC r286981 and r287098:

    Don't truncate cursor arithmetic to 16 bits.

    When updating the row number when the cursor position escape sequence is
    issued, we should make sure to store the intermediate result in a 32-bit
    integer. If we fail to do this, the cursor may be set above the origin
    region, which is bad.

    This could cause libteken to crash when INVARIANTS is enabled, due to
    the strict set of assertions that libteken has.

  PR:		202326, 202540, 202612
  Submitted by:	kwcu csie org

Changes:
_U  stable/9/
_U  stable/9/sys/
  stable/9/sys/teken/teken.c
  stable/9/sys/teken/teken_subr.h
Comment 10 Ed Schouten freebsd_committer 2015-09-14 09:32:10 UTC
All merged back to FreeBSD 9 and 10. Thanks again for reporting these issues!