From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=0.2 required=5.0 tests=DKIM_ADSP_CUSTOM_MED, DKIM_INVALID,DKIM_SIGNED,FREEMAIL_FROM autolearn=no autolearn_force=no version=3.4.4 Received: (qmail 2296 invoked from network); 5 Feb 2021 20:05:05 -0000 Received: from 1ess.inri.net (216.126.196.35) by inbox.vuxu.org with ESMTPUTF8; 5 Feb 2021 20:05:05 -0000 Received: from mail-wr1-f42.google.com ([209.85.221.42]) by 1ess; Thu Feb 4 16:19:19 -0500 2021 Received: by mail-wr1-f42.google.com with SMTP id c4so5207668wru.9 for <9front@9front.org>; Thu, 04 Feb 2021 13:19:10 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:message-id:to:cc:subject:date:mime-version :content-transfer-encoding; bh=JTD0e4tIkIPaGTRjPfrgQRami/Cmj0v+FpfamCv0htA=; b=p3rICrH2ipeEh7PYm7eoXhw28kgknQ4V3HI1/p332Kl71qHBevIyVjcmN7ve4yNZ0P vsWyT1cRdd/liUA8wrCgH7d4qwYcOKzV7d7zLTx/rqWJIzcOYUg9+HSI88IRRRyxhA+W hZ2nDXj/ti0ZM4p02c8MFEDfJH7DvnXmTDUnFbNjzda/qOGgv6kO8CV3O5HyagnQfJCz wNSIVsJ9RmWS7HVnpdMhuJ9dVJir4rMXkgI2FbdNf6nNJSB0RSjmh01vr9OzlkBcMaJJ H57+S1HnyUlvFFURvA5OKGTElu22slQN7j832HJzi6Q0iMR6dKHd8mJJsf+ohMEJrkAF vfmg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:message-id:to:cc:subject:date:mime-version :content-transfer-encoding; bh=JTD0e4tIkIPaGTRjPfrgQRami/Cmj0v+FpfamCv0htA=; b=ldx44y0Jwcy2VKZwtfBsak39DdEyXW8z8AyhROTMSdVaZblwIjU99JB7gwdOJ1Bs3N C06Oln+qCsMMfkzmV6ov9pAVlV7o7zQJduumpqPQ31ge28DQfly+ZNtWZzCRPAft1ie6 Unjy+ANlHHo82RmlYH9w3NvI6AAa2o2wJKRrXb4zSMdVGyq8LO2hpPOZ3gxunh36V+/c HJTSMz51Hyep6+3cZV2dZx98rcPIRvty94BdVdYHjeyMnevUPFUz1TmbF7P2Rewjs0rR GThlM+dBOHSo3Hsm+aS2jQUUJnmM+Macg3erkKwrFJbghlA/s9kxaCdoSWEeEMTC1dJO C6Uw== X-Gm-Message-State: AOAM532TGW0t4w7UgIS1C3jOIK+FTQFsR332YLpxSEo5xw1ELhPUD1lJ sky0y4CrmAshfBRSARikaOU= X-Google-Smtp-Source: ABdhPJwbKf8bjHAYXqxqNL6pRpIbGC5395oUOGTUZflgmaecu0BWUxKeAV/mAr5ruCfpQI53hsTV+A== X-Received: by 2002:a5d:554e:: with SMTP id g14mr1332229wrw.305.1612473549781; Thu, 04 Feb 2021 13:19:09 -0800 (PST) Return-Path: Received: from term.home ([185.64.155.70]) by smtp.gmail.com with ESMTPSA id l11sm9180349wrt.23.2021.02.04.13.19.09 (version=TLS1_2 cipher=ECDHE-ECDSA-CHACHA20-POLY1305 bits=256/256); Thu, 04 Feb 2021 13:19:09 -0800 (PST) From: boehm.igor@gmail.com X-Google-Original-From: igor@gmail.com Message-ID: <66A85B05FD8A4DCC2FBCCBC6BC5F9FFB@gmail.com> To: 9front@9front.org CC: boehm.igor@gmail.com Date: Thu, 04 Feb 2021 22:19:08 +0100 MIME-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit List-ID: <9front.9front.org> List-Help: X-Glyph: ➈ X-Bullshit: optimized content-addressed content-driven CMS method Subject: [9front] drawterm: fix suicide caused by off-by-one write and out-of-bound read in libmemdraw/draw.c (patch) Reply-To: 9front@9front.org Precedence: bulk Drawterm is my key interface to 9front VMs. Recently, I have been experiencing random SEGFAULTs, mostly with more complex window scenarious where I had many acme instances, netsurf, Nail, faces, etc. This was getting really annoying since I was also running various ssh tunnels from acme to compile and debug C++ code that I was editing in acme on 9front. Recreating that setup after every crash was a nuisance so I tried to figure out what the root cause was and even managed to fix it. Drawterm now runs smoothly without hickups ☺ Further below is the full story on how to reproduce the issues with some reasoning as to why things break and how to fix them so you can review and hopefully commit these fixes in one way or another. If you quickly want to test these changes they are also available in the following repo: • git@github.com:1g0rb0hm/drawterm.git Here is the pull request that contains the fix: • https://github.com/1g0rb0hm/drawterm/pull/3 Here is the diff generated with hg on 9front against: • https://code.9front.org/hg/drawterm cpu% hg diff . diff -r 8fd69c7dfe88 libmemdraw/draw.c --- a/libmemdraw/draw.c Wed Dec 23 15:58:36 2020 +0100 +++ b/libmemdraw/draw.c Thu Feb 04 21:55:51 2021 +0100 @@ -1476,7 +1476,7 @@ { Buffer b; Memimage *img; - int dx, isgrey, convgrey, alphaonly, copyalpha, i, nb; + int dx, isgrey, convgrey, alphaonly, copyalpha, i, j, nb; uchar *begin, *end, *r, *w, *rrepl, *grepl, *brepl, *arepl, *krepl; uchar ured, ugrn, ublu; ulong u; @@ -1526,7 +1526,8 @@ krepl = replbit[img->nbits[CGrey]]; for(i=0; i>img->shift[CAlpha]) & img->mask[CAlpha]]; } @@ -2136,8 +2137,8 @@ *dp ^= (v ^ *dp) & lm; dp++; } - memset(dp, v, dx); - dp += dx; + memset(dp, v, dx-1); + dp += dx-1; *dp ^= (v ^ *dp) & rm; } } To reproduce the errors consistently with enough information to trace down the root cuase, compile drawterm with the following options: CFDEBUG=-fno-omit-frame-pointer -fsanitize=address -O0 CFLAGS=$(CFDEBUG) -Wall -Wno-gnu-designator -Wno-missing-braces -g -I$(ROOT) -I$(ROOT)/include -I$(ROOT)/kern -c -D_THREAD_SAFE $(PTHREAD) LDDEBUG=-fsanitize=address LDADD=-g -framework AppKit -framework OpenGL $(LDDEBUG) These options use ASAN in combination with clang/llvm and ensure we get a proper stack trace pointing to the guilty statement. Without these options the stack trace you get is useless. # 1. Issue: off-by-one write in libmemdraw/draw.c:/^memoptdraw In libmemdraw/draw.c:/^memoptdraw an off by one write causes a SEGFAULT. The fault typically occurres in complex drawterm sessions (i.e. you have to stress the draw routines). The best way to reproduce it was to run two instances of netsurf, catclock, and doom. Running a complex drawterm session (where drawterm was compiled with above options) will then crash as follows: Application Specific Information: ================================================================= ==81878==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x61a0000b210c at pc 0x0001084224fb bp 0x70000d5065b0 sp 0x70000d5065a8 READ of size 1 at 0x61a0000b210c thread T20 #0 0x1084224fa in memoptdraw draw.c:2141 #1 0x10841ef62 in memimagedraw draw.c:158 #2 0x1084697cc in memdraw draw.c:68 #3 0x108290814 in drawmesg devdraw.c:1562 #4 0x1082a418d in drawwrite devdraw.c:1319 #5 0x1082fd0f2 in kwrite sysfile.c:474 #6 0x1082fd377 in _syspwrite sysfile.c:499 #7 0x1082ff7a4 in syspwrite sysfile.c:1059 #8 0x10832223b in slavewrite exportsrv.c:638 #9 0x108320fd0 in blockingslave exportsrv.c:495 #10 0x108313602 in tramp posix.c:119 #11 0x7fff682f8108 in _pthread_start+0x93 (libsystem_pthread.dylib:x86_64+0x6108) #12 0x7fff682f3b8a in thread_start+0xe (libsystem_pthread.dylib:x86_64+0x1b8a) SUMMARY: AddressSanitizer: heap-buffer-overflow draw.c:2141 in memoptdraw Shadow bytes around the buggy address: 0x1c34000163d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x1c34000163e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x1c34000163f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x1c3400016400: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x1c3400016410: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 =>0x1c3400016420: 00[04]fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x1c3400016430: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x1c3400016440: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x1c3400016450: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x1c3400016460: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x1c3400016470: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa The following diff snippet shows the problematic code together with its fix: ```c @@ -2136,8 +2137,8 @@ memoptdraw(Memdrawparam *par) *dp ^= (v ^ *dp) & lm; dp++; } - memset(dp, v, dx); - dp += dx; + memset(dp, v, dx-1); + dp += dx-1; *dp ^= (v ^ *dp) & rm; } } ``` The actual SEFGAULT occurrs at the statement: • `*dp ^= (v ^ *dp) & rm;` because in certain conditions it was writing out-of-bounds. That was identified by initially adding asserts(...) to narrow down the root cause. # 2.Issue: out-of-bound read in libmemdraw/draw.c:/^readbyte In libmemdraw/draw.c:/^readbyte an out of bound read would read garbage data and cause execution with ASAN to terminate with the following error: Application Specific Information: ================================================================= ==83236==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x61d00003798c at pc 0x00010d4ab58c bp 0x70000ad2e840 sp 0x70000ad2e838 READ of size 1 at 0x61d00003798c thread T15 #0 0x10d4ab58b in readbyte draw.c:1529 #1 0x10d4a4f5e in greymaskread draw.c:1250 #2 0x10d49c9e8 in alphadraw draw.c:720 #3 0x10d491faa in memimagedraw draw.c:171 #4 0x10d4dc7cc in memdraw draw.c:68 #5 0x10d303814 in drawmesg devdraw.c:1562 #6 0x10d31718d in drawwrite devdraw.c:1319 #7 0x10d3700f2 in kwrite sysfile.c:474 #8 0x10d370377 in _syspwrite sysfile.c:499 #9 0x10d3727a4 in syspwrite sysfile.c:1059 #10 0x10d39523b in slavewrite exportsrv.c:638 #11 0x10d393fd0 in blockingslave exportsrv.c:495 #12 0x10d386602 in tramp posix.c:119 #13 0x7fff682f8108 in _pthread_start+0x93 (libsystem_pthread.dylib:x86_64+0x6108) #14 0x7fff682f3b8a in thread_start+0xe (libsystem_pthread.dylib:x86_64+0x1b8a) ... SUMMARY: AddressSanitizer: heap-buffer-overflow draw.c:1529 in readbyte Shadow bytes around the buggy address: 0x1c3a00006ee0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x1c3a00006ef0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x1c3a00006f00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x1c3a00006f10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x1c3a00006f20: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 =>0x1c3a00006f30: 00[04]fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x1c3a00006f40: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x1c3a00006f50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x1c3a00006f60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x1c3a00006f70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x1c3a00006f80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 The problematic statement is: • `u = r[0] | (r[1]<<8) | (r[2]<<16) | (r[3]<<24);` The variable `r` is a uchar* pointer and we are, in certain conditions, reading more memory than we are supposed to in the above statement. We have to be careful that at the index where we dereference `r` we are not reading beyond the end of a buffer. The fix below adds a bounds check that never reads beyond those buffer bounds: ```c @@ -1526,7 +1526,8 @@ readbyte(Param *p, uchar *buf, int y) krepl = replbit[img->nbits[CGrey]]; for(i=0; i>img->shift[CAlpha]) & img->mask[CAlpha]]; } ``` Sorry about the length of this mail. Since this fixes bugs that have existed for a long time and are tricky, I always like to add as much context as possible to ease review. Cheers, Igor