From 57d1743662b7fde305ea59225093d109e07c9997 Mon Sep 17 00:00:00 2001 From: Mark Brand Date: Fri, 7 Oct 2022 10:44:20 +0200 Subject: [PATCH] Fixup non-canonical fault addresses for amd64. This uses DisassemblerObjdump to add a processing step in MinidumpProcessor to compute the true faulting address from register state and disassembly of the fault instruction when the fault address is suspicious (-1). Bug: 901847 Change-Id: Ia1f77d542c4055c82ce2504db8c84a9e52001866 Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/3932957 Reviewed-by: Ivan Penkov --- .../processor/minidump_processor.h | 6 +- src/processor/minidump_processor.cc | 89 +++++++++++++++++- src/processor/minidump_processor_unittest.cc | 19 ++++ .../testdata/write_av_non_canonical.dmp | Bin 0 -> 27561 bytes 4 files changed, 110 insertions(+), 4 deletions(-) create mode 100644 src/processor/testdata/write_av_non_canonical.dmp diff --git a/src/google_breakpad/processor/minidump_processor.h b/src/google_breakpad/processor/minidump_processor.h index aa44e86c..137ef444 100644 --- a/src/google_breakpad/processor/minidump_processor.h +++ b/src/google_breakpad/processor/minidump_processor.h @@ -101,8 +101,10 @@ class MinidumpProcessor { // exception, if this information is available. This will be a code // address when the crash was caused by problems such as illegal // instructions or divisions by zero, or a data address when the crash - // was caused by a memory access violation. - static string GetCrashReason(Minidump* dump, uint64_t* address); + // was caused by a memory access violation. If enable_objdump is set, this + // may use disassembly to compute the faulting address. + static string GetCrashReason(Minidump* dump, uint64_t* address, + bool enable_objdump); // This function returns true if the passed-in error code is // something unrecoverable(i.e. retry should not happen). For diff --git a/src/processor/minidump_processor.cc b/src/processor/minidump_processor.cc index a80baf45..bf561dfa 100644 --- a/src/processor/minidump_processor.cc +++ b/src/processor/minidump_processor.cc @@ -31,6 +31,7 @@ #include #include +#include #include #include #include @@ -43,6 +44,7 @@ #include "google_breakpad/processor/process_state.h" #include "google_breakpad/processor/exploitability.h" #include "google_breakpad/processor/stack_frame_symbolizer.h" +#include "processor/disassembler_objdump.h" #include "processor/logging.h" #include "processor/stackwalker_x86.h" #include "processor/symbolic_constants_win.h" @@ -117,7 +119,7 @@ ProcessResult MinidumpProcessor::Process( has_requesting_thread = exception->GetThreadID(&requesting_thread_id); process_state->crash_reason_ = GetCrashReason( - dump, &process_state->crash_address_); + dump, &process_state->crash_address_, enable_objdump_); process_state->exception_record_.set_code( exception->exception()->exception_record.exception_code, @@ -758,8 +760,82 @@ bool MinidumpProcessor::GetProcessCreateTime(Minidump* dump, return true; } +static bool IsCanonicalAddress(uint64_t address) { + uint64_t sign_bit = (address >> 63) & 1; + for (int shift = 48; shift < 63; ++shift) { + if (sign_bit != ((address >> shift) & 1)) { + return false; + } + } + return true; +} + +static void CalculateFaultAddressFromInstruction(Minidump* dump, + uint64_t* address) { + MinidumpException* exception = dump->GetException(); + if (exception == NULL) { + BPLOG(INFO) << "Failed to get exception."; + return; + } + + MinidumpContext* context = exception->GetContext(); + if (context == NULL) { + BPLOG(INFO) << "Failed to get exception context."; + return; + } + + uint64_t instruction_ptr = 0; + if (!context->GetInstructionPointer(&instruction_ptr)) { + BPLOG(INFO) << "Failed to get instruction pointer."; + return; + } + + // Get memory region containing instruction pointer. + MinidumpMemoryList* memory_list = dump->GetMemoryList(); + MinidumpMemoryRegion* memory_region = + memory_list ? + memory_list->GetMemoryRegionForAddress(instruction_ptr) : NULL; + if (!memory_region) { + BPLOG(INFO) << "No memory region around instruction pointer."; + return; + } + + DisassemblerObjdump disassembler(context->GetContextCPU(), memory_region, + instruction_ptr); + fprintf(stderr, "%s %s %s\n", disassembler.operation().c_str(), + disassembler.src().c_str(), disassembler.dest().c_str()); + if (!disassembler.IsValid()) { + BPLOG(INFO) << "Disassembling fault instruction failed."; + return; + } + + // It's possible that we reach here when the faulting address is already + // correct, so we only update it if we find that at least one of the src/dest + // addresses is non-canonical. If both are non-canonical, we arbitrarily set + // it to the larger of the two, as this is more likely to be a known poison + // value. + + bool valid_read, valid_write; + uint64_t read_address, write_address; + + valid_read = disassembler.CalculateSrcAddress(*context, read_address); + valid_read &= !IsCanonicalAddress(read_address); + + valid_write = disassembler.CalculateDestAddress(*context, write_address); + valid_write &= !IsCanonicalAddress(write_address); + + if (valid_read && valid_write) { + *address = read_address > write_address ? read_address : write_address; + } else if (valid_read) { + *address = read_address; + } else if (valid_write) { + *address = write_address; + } +} + // static -string MinidumpProcessor::GetCrashReason(Minidump* dump, uint64_t* address) { +string MinidumpProcessor::GetCrashReason(Minidump* dump, uint64_t* address, + bool enable_objdump) { MinidumpException* exception = dump->GetException(); if (!exception) return ""; @@ -1985,6 +2061,15 @@ string MinidumpProcessor::GetCrashReason(Minidump* dump, uint64_t* address) { *address = GetAddressForArchitecture( static_cast(raw_system_info->processor_architecture), *address); + + // For invalid accesses to non-canonical addresses, amd64 cpus don't provide + // the fault address, so recover it from the disassembly and register state + // if possible. + if (enable_objdump + && raw_system_info->processor_architecture == MD_CPU_ARCHITECTURE_AMD64 + && std::numeric_limits::max() == *address) { + CalculateFaultAddressFromInstruction(dump, address); + } } return reason; diff --git a/src/processor/minidump_processor_unittest.cc b/src/processor/minidump_processor_unittest.cc index 6dfa54a6..1ca8c9fb 100644 --- a/src/processor/minidump_processor_unittest.cc +++ b/src/processor/minidump_processor_unittest.cc @@ -799,6 +799,25 @@ TEST_F(MinidumpProcessorTest, TestFastFailException) { ASSERT_EQ(state.crash_reason(), "FAST_FAIL_FATAL_APP_EXIT"); } +#ifdef __linux__ +TEST_F(MinidumpProcessorTest, TestNonCanonicalAddress) { + // This tests if we can correctly fixup non-canonical address GPF fault + // addresses. + // Dump is captured from a toy executable and is readable by windbg. + MinidumpProcessor processor(nullptr, nullptr /*&supplier, &resolver*/); + processor.set_enable_objdump(true); + + string minidump_file = GetTestDataPath() + + "write_av_non_canonical.dmp"; + + ProcessState state; + ASSERT_EQ(processor.Process(minidump_file, &state), + google_breakpad::PROCESS_OK); + ASSERT_TRUE(state.crashed()); + ASSERT_EQ(state.crash_address(), 0xfefefefefefefefeU); +} +#endif // __linux__ + } // namespace int main(int argc, char* argv[]) { diff --git a/src/processor/testdata/write_av_non_canonical.dmp b/src/processor/testdata/write_av_non_canonical.dmp new file mode 100644 index 0000000000000000000000000000000000000000..02da25eebf828e4864c900fafbff722cf24f28a3 GIT binary patch literal 27561 zcmeG_30PIt*6RW)*uZs4oNt9gnug(6rl?rgp1yF(dXEYO5)}0YXxOW~rjMdg60X_oM>wbxqro^ya}_*Kuo?{2pc*Wv15eW|7rcGZ|&gg$WO%2TnX&XQ6O)`PRLbQ7>1&}FSH1MwLi{AbH%*xvt=2d>KiRUEq_dF0ka)dumprt_@LpYq-y83J*0)$DXpefy0D73VV09 zE+~)GoOfn0o;Eyq&v(@kMpwm25Vi3ncsFmx*gXzi>7t8ZC|&4oNDZlLFBn~wXTuVI zZ&PU)G@EH#&6P*_3@q@L2gO4XG=I%d0X5 zuBeKE!OJhX^i_*48d04Q!MX@qb-5hHL2BL#sq1mhS9uKdiD?dV_g5q}ZE5%6Xelp( zp>)NguIiAwida`lOSoZlOQ>C&a{9#%U8jmJf>vG5dPr(olR^n~>tRXhz2uA}7wmb} z1>39RGVZYG>TO1-XAV{9)J0=f4OHg`T69&`sq>#7{%IZ(C3S{-B0E8gf_B$$v*?OI zT^W&FUId-GR1ZN?bGb!V-F#ihJ_I*hI@Ap>dq3*czLmYa+K4WKp>)ynbwfy94|BfC zBjCxWBH)5kQqM>`PFBdPhkk}*w*3ItP{_Y(B?aJ8?wHsJ#5l$R)-IL*y>Xp#H*QjQXc4^_- zy0TOAag)0JkE&|7WcliDG=avjbqfz3hF&<`SodL8q`^mES)YG8-j<9raU7KxxcxPb zMnAyN(~;q_4h*3fE4m3dprKIqWuy_DvY4T!7t;(JLj@LEqr=t6F%r&$RGdBL;Vh#r z%rNuVESw=1VCuCvr=_{_EWFFdc%hk*rowofdj@BrsVFHAXhxcibCWEj%0fNo1$0ez zNST1M<$RQ%g>&3QoV%tPZJ8)B7x@`tqumJp=_o4?rI1x5F(?JAp5*x0R{RA3!<^gg8PW)QW}uhm7~d9H>=@bz3A9F#fu%Cb_&ey8 z##oVuxuEhbz_o>-gvR)FHkPx`Na+jx%{s?Gi=@WnhiAvxrks5i8I38M*^-+nrpD6B ziq^QsTN0e7s|@T)esz7DVl0`9o+-d}sywVKGw>?FYYyfx|2Qop-aO>a!BU@TkhxvXAm^Xp^)|i<2CMMG`uA6rZp^2@!ZcN z-+t!1`y5^|Xz#2739^zPD++#*2R^aP4d7v%Aedh0T)F~*3x2k+qt*q7Z~bI#Nk^tn z6#G6K!g0-?(ydze_nX(e|588a$_5HGdW+#sBR^a6lM2b$g3Q78BLiV7c(Em*mGVr) zaNCVvbJ{Ywyx)78rzOMFEu|eBXDKp=48z*NWC^^X6CGG^HU24^h+I| zm7A5GpOu=Cm!6;9$Co(;NY;T5Ms&$q)po?>v_T_slYL8R@H?|0GdDLtv3~sz51u+U zC2~mi&YUBwKD&!3rf22N%*stXH$ZgpJxycQ`g=ds`l?f|TEAm(t02)7)EOWf`|S6R zO}}}2!q%2uw{4ESc@dGNXU<9Ya**y8XrL^6AZ&zBLP=(Fwq$^MfQ-)uLo$W-gEI<^I}8K|Qvdzj#Szhh5{gUmx?s$oGaV^PMk(GG^rk zy>oQ@fCK0EOrN&5Xyn^_ZdgJjqS$X+6vt|Zpo-FLK$Jg_QnybEl`L%DKjxQmBzI^nyp`d9)XS^uTvZ)TS zb@TY?mNHdCw!TStcuI!IG(qY+9N@X#+{Kvq2mtNUbTj$w_WR6~^RofdOqZuqPymi6 z^gY!K#k-LK7<8$`%TY5AQ~LzfC@^p&Z>mwQDOAeRhE5T?_LTf_GC;9X0(cPToFd%S ziRo2N#bKUWs66VHRTRO=IF&S;u!2-NrNOgW#y`u(JYb^>3}HXbj3kSQj>5xNPc>7N zVp&qE{6?liIwiVChQ@mYAAPl7lz8}^^d?QMx8UrA+*A3L96nWXRQ&T-9E!9Co*esv9F z&TsWh$-%>>Zz?BDX&lq*^X3*gx)l9J;*{vKJpk|Qy>a&oPc3_l+6#>0)F}d7Zo1(~ z6t&aZmLS#^RF+P+G?92HX#Dqv*5&)0VL!Y$3{PSCCvp9#YBkri168+Y8>qIFoW}l9 zI>oEqYl_&vnz#H;+&-y4wH;KxU6QWxO&#s+nQkSJYpwn~iMQ$Dl2&$lm7jnU*eYD_ zBS;6-@O+vHg^qTw-y`|v;2p>kY>J~l!J(tczq8-K?R;|x4l4NhQoW)Eg8c#WYyYAB zl+tOtto?xMQ9G1Qmo8IeytGC7EA4O7`~{p3S@N#9N#lI@?8(3U%!e){KWdN4pW$7c zyn^@T6=k4bVrh?azkg*;C)3_&yg#my4=_jBnQ+VwT@n_?0N{GH$gx$W$s3Mv`=;X% z#Ww`CUO3a2oXY)Yc+1S`8IFgy%MtmG~(sOf~GePoVM)a?bacb;d1t)q>t7U>_^r3g*9T(a?|`7A@%$mi@#(&+o`Pa za?@R)u0Lpcg4_&Lgd8bQIYD|pc&QL;Z59H)dBgu z^Iv@(qnoO>-HUHk*h10-?}A+(Z_&6oZ_&rbu)f}+n1F3!Q{0%M;b_Rl?xTCX?p;i` zXj7NbMXy)IZF;NErr0hEP0@@gdUq|2Z+?vG8XZ|P&ow(Twuv*HvK%U%_K!lP)B0Gb zblP1Cl}`I%A=1BH{$aCF>1%1-Dg^)9y~jwW`CKUeeWBC8y<%->`}T+CUwhyf=?9OI zPOm&!?Jtp+esvvKA@1-lrhUJ~uP{WmUlsJ|drzQ`LIC`qaJlc}c|@fnKFhKWN%J

gB7jyt9omViPKnmM4T8?5-y`$hfD*M2{vv^&Zlg%&Lnea*Q z$RWkS72BVDH&m`Jgu^ftqK!+f^qkVT-<1DledV9V_9q=t_@ElSB8KAig$?g;`MKZA zGx?}+9Fa^T1?$QHfqMzZn}ek|Em>=GOd_+cn?!9GABeEL1*4 z90Tr~7qr((s^y?|K}iX}pbDJ-B#FE6Woc{J^n~3W&EFWA7$5GQ&-*^YZQ5wB)8bb{?hDgXa=MbFsgTs$Z&A-H-;IJ={F6E_m z`@gW;oAkLoPJCpdlfF=%50&fwlKHegmv?19c?Cb-eC`KdQ4qZHDO~ii3(Q%1L0qua z-VUn2-W+m~S*Pd;XBo;bgkIY_m8bBnb@CFluJ_NuNT{b$nE-~zI4s)T|@eM2=k>cGyP>>FE{nGvq8_Uq_pA+-nv3 zvg(P(KMAemJc`h%{Z)=~^NZfgzG8hOSJ!}umDY7cgaP9#nU2OCR9AIhOWV7&Kz-u; zAI`Shq4h&$O7+($K#rYWo7rlSLys#xU;5Z;zLib|sUE-3E5D}u#Lg1&8?VbvhFMqe zJ{_+&;-{I8@;NH=mM=2v$8bE6oIl-9dtKzHU6oSrD@0*CUItV3&_mJ#2)P-4g!MV` z*+M_zU@)Eh;F0oBdd+7sBIu(AJtW;~2jT6lFb=haE>#6Npj#ZQP~~Ypl0@IKom@YX z#a?~>NgU}-pd0*H5CsyMf|vEvLG-by;0K0|??Y3hr{W#0M_f*-kDv*XAB`$Z8^wHS z@;oS+U)JZ#19Fvv<=?JRClmord0Nk(zR0mJEc9EXey;h#-oIh}YGKv`rn7`s1}Q(&Yr4Xu{rvoTrC*@)V%G5t^?MX=qX!pREXHH| zbDx+INj%lqz;GMPdeU;)4%L$||3_n|>hm06y*En*4u53t_}V@xjxhL>c|gY%;g9Tb z!6j|v^K^P{G3h&Q9~vrFnJ&RU+!-tA+@j9=E*ivg=v=%lI6BH=hkQ!bF(mONbl`Ko ziYs$3x1Ia7$vy!iy633>w5@;I&U!kQNOEy zV>fI6!%lLGuM|GN)Sgaf6sPS^Q9Gu#XZgyfQ0;#88-5j# z=&7IEzKi+k<9`&K_Ou$w@|4~WoNuPC`(DP8sE1mw%K*^N4h8zZIOcQmtDLoB*XJ^D zAGC+I@~Qbw(|j{q zqkhTWb7H|m`60ckgFiYMjO(11ga_6Ju%&QUqVUm;F3}(eRK!ZAT>v z9=XwZw$?KME?4#Q*ynUU!gv~MQcymQ;d)ghr$>o?!qboyFUQDLIqHXQZI~oEkbjBm zdb-*-(FK!u-oX03%@|gQ9mOg@;@66^wOlI>Nq*)Yu7{h`d3{jp;i6c$l}yp~dWw_WK{C$~GtE1AbbrG)_VxP|Ee|2o zd?}yS<0}QB>-WvEC9sBo-C~&9r}bO?T(^FUoFIc$EPS<(>&Y^-ll-}-51(&OddWV1 zQYNjh<+WTd{eks%ju!^n6OJpN+L^D7gG=}W^h~K{`UD>+ZxNLACC`UbgkR+=okHcm z{b{CGyw(%tBS_^*&mYxqeGXNAz+NK6p!o6#wqNBcr}Fda6o95{+o5npR!Sr3c{!8>^gSKn4W8pgbFukz{^GD;4LX1&;S%#OXf7nJX z!h712F^g&hG0<~btL=@!;CM#LTgNfAdykrrJn9z@JV-~^gLtjSYY<*!5AB1}2l`79 z(#QNW1tuVzh&o6&!8E+|BmeZVehLKjo<8_L5pnc^WV+lJ0z5vQKe|5x^F|*wrm@yK zg!J)b`mjIACYn$|3L9v z#y-oLNc~3;Nn%G2fe8Zj`3pREZncY{U+fLMhm7^l!#Fa<^i|gWsC}PjW6mZ5iw#8A zU=TX>pZa&8d@@V%4e_i0jWHIYKl0qFMQxqihTbcyLl{PcJrMRl*aKk?ggp@Uz#ry; F{|E1dC6oXF literal 0 HcmV?d00001