[Bug ld/24055] _bfd_xcoff_swap_aux_in smashes the stack
https://sourceware.org/bugzilla/show_bug.cgi?id=24055 Alan Modra changed: What|Removed |Added Resolution|--- |FIXED Status|ASSIGNED|RESOLVED --- Comment #9 from Alan Modra --- I believe this one has been fixed, by e86fc4a5bc37 -- You are receiving this mail because: You are on the CC list for the bug.
[Bug ld/24055] _bfd_xcoff_swap_aux_in smashes the stack
https://sourceware.org/bugzilla/show_bug.cgi?id=24055 --- Comment #8 from Wolfgang Thaller --- Not quite yet... * The XCOFF specification seems quite clear that a C_FILE symbol can contain up to four different strings in four different aux entries, and in fact I have a few files with four aux entries per C_FILE that I want to link. So the assert would break things for me. * XCOFF never spreads a single string accross multiple aux entries (that's a Microsoft PE/COFF thing). Thus, the comment is very misleading. It's not "we" who don't support this, but *XCOFF* that handles long filenames in a different way. * Because of the above, no code in binutils expects us to copy multiple aux entries into one for XCOFF files, and with my patch we never do it, so there is no reason to limit the number of aux entries per C_FILE. * That assert and comment should probably go into the swap_aux_in implementation(s) that might need to handle filenames spread across aux entries - coffswap.h:coff_swap_aux_in and coffcode.h:coff_bigobj_swap_aux_in. -- You are receiving this mail because: You are on the CC list for the bug. ___ bug-binutils mailing list bug-binutils@gnu.org https://lists.gnu.org/mailman/listinfo/bug-binutils
[Bug ld/24055] _bfd_xcoff_swap_aux_in smashes the stack
https://sourceware.org/bugzilla/show_bug.cgi?id=24055 Nick Clifton changed: What|Removed |Added Attachment #11507|0 |1 is obsolete|| --- Comment #7 from Nick Clifton --- Created attachment 11526 --> https://sourceware.org/bugzilla/attachment.cgi?id=11526=edit Proposed patch (In reply to Wolfgang Thaller from comment #6) Hi Wolfgang, Thanks very much for investigating further. > I therefore resubmit my original patch, but it should probably not be copied > to other implementations of swap_aux_in without further discussion. Fair enough. I would like to propose one small extension to your patch however - an assertion that the numaux variable is less than 3. As in the revised version here. Will this be acceptable to you ? Cheers Nick -- You are receiving this mail because: You are on the CC list for the bug. ___ bug-binutils mailing list bug-binutils@gnu.org https://lists.gnu.org/mailman/listinfo/bug-binutils
[Bug ld/24055] _bfd_xcoff_swap_aux_in smashes the stack
https://sourceware.org/bugzilla/show_bug.cgi?id=24055 --- Comment #6 from Wolfgang Thaller --- So... here are my findings... The code handling multiple aux entries was introduced on 1999-05-10 to handle an undocumented feature of Microsoft's PE format: 1999-05-10 DJ Delorie [...] * coffgen.c (coff_get_normalized_symtab): Properly read long MS filename symbols, which use one *or more* auxents. * coffswap.h (coff_swap_aux_in): ditto Neither current MSVC nor binutils generates files that use this feature. This code later got copied to coff-rs6000, but it was never appropriate for XCOFF, where multiple aux entries on a C_FILE have a different meaning. I therefore resubmit my original patch, but it should probably not be copied to other implementations of swap_aux_in without further discussion. The existing PE code might still crash for some input files, but I don't know if they exist in practice. AUX Format All(?) COFF variants have in common that C_FILE symbols should have a name of ".file", with the actual file name stored in an aux entry. Apparently, Microsoft once used several consecutive entries to store names that don't fit into a single aux entry. Microsoft's documentation does not mention that possibility: https://docs.microsoft.com/en-us/windows/desktop/debug/pe-format#auxiliary-format-4-files XCOFF and some other COFF variants handle long file names by storing four zero bytes followed by a string table offset in the aux entry. XCOFF defines another field, x_ftype, in those AUX entries that define the type of the string - so we can have a C_FILE that has a file name, a time stamp and a compiler version string: https://www.ibm.com/support/knowledgecenter/ssw_aix_71/com.ibm.aix.files/XCOFF.htm#XCOFF__c0f91aa357jbau Binutils ignores the x_ftype field and resets it on output; this is also a bug, though a very low-priority one. - Call Sites Many call sites are never invoked for C_FILE symbols and are thus not relevant. coffgen.c:1709 Allocates enough space (contiguous array of internal auxentries, sized to match the input), and handles filenames spread over multiple aux entries when handling PE files. This is DJ Delorie's original hack. It still loops over all aux entries, so it will work with XCOFF-style multiple aux entries provided my patch. If the multi-aux behaviour is changed for PECOFF as well, this will need to be changed. cofflink.c:1761 Only allocates a single internal aux entry and loops over all external aux entries. Will crash if there are still COFF (not XCOFF) files with a filename spread over three or more aux entries (> 36 characters; 3 * 18 bytes will not fit in sizeof(internal_auxent)). xcofflink.c:4991 Only allocates one internal aux entry and loops over all external aux entries. gdb/coffread.c and gdb/xcoffread.c also contain several calls to bfd_coff_swap_aux_in. At first glance, none of them allocates extra memory. Danger of crashes here, as well. -- You are receiving this mail because: You are on the CC list for the bug. ___ bug-binutils mailing list bug-binutils@gnu.org https://lists.gnu.org/mailman/listinfo/bug-binutils
[Bug ld/24055] _bfd_xcoff_swap_aux_in smashes the stack
https://sourceware.org/bugzilla/show_bug.cgi?id=24055 --- Comment #5 from Wolfgang Thaller --- I'll have some free time left this weekend, so I'll have another look at those call sites and report back. -- You are receiving this mail because: You are on the CC list for the bug. ___ bug-binutils mailing list bug-binutils@gnu.org https://lists.gnu.org/mailman/listinfo/bug-binutils
[Bug ld/24055] _bfd_xcoff_swap_aux_in smashes the stack
https://sourceware.org/bugzilla/show_bug.cgi?id=24055 Nick Clifton changed: What|Removed |Added Status|UNCONFIRMED |ASSIGNED Last reconfirmed||2019-01-04 Ever confirmed|0 |1 --- Comment #4 from Nick Clifton --- (In reply to Wolfgang Thaller from comment #3) Hi Wolfgang, Yes you are right - my patch is completely wrong. :-( It looks like we need to revisit the callers of swap_aux_in and make sure that they allocate enough space for the auxillary entries. *sigh* Would you like to have a go at this ? :-) If not then I will look at it my copious free time... Cheers Nick -- You are receiving this mail because: You are on the CC list for the bug. ___ bug-binutils mailing list bug-binutils@gnu.org https://lists.gnu.org/mailman/listinfo/bug-binutils
[Bug ld/24055] _bfd_xcoff_swap_aux_in smashes the stack
https://sourceware.org/bugzilla/show_bug.cgi?id=24055 --- Comment #3 from Wolfgang Thaller --- Comment on attachment 11507 --> https://sourceware.org/bugzilla/attachment.cgi?id=11507 Proposed patch There are also some minor problems with your proposed patch: * The assert checks < sizeof(AUXENT), but AUXENT is the external auxentm and by definition AUXESZ == sizeof(AUXENT), so it looks like this assert will always fail. * The size for the memcpy is numaux * sizeof(AUXESZ), but AUXESZ is the size, not the structure, so sizeof(AUXESZ) == sizeof(18) == sizeof(int) == 4. -- You are receiving this mail because: You are on the CC list for the bug. ___ bug-binutils mailing list bug-binutils@gnu.org https://lists.gnu.org/mailman/listinfo/bug-binutils
[Bug ld/24055] _bfd_xcoff_swap_aux_in smashes the stack
https://sourceware.org/bugzilla/show_bug.cgi?id=24055 --- Comment #2 from Wolfgang Thaller --- Probably won't work for me; I added an assert myself at first and it did in fact trigger. I am working with an old XCOFF file provided by Apple back in the 90s, and it contains a C_FILE with a total of four aux entries. They are distinguished by their x_ftype field, and contain the source file name, the compile time stamp, the compiler version and some "compiler defined information", respectively. It does not look malformed to me, though of course Apple's 1990s interpretation of XCOFF could have diverged from the one that is still relevant today :-) Also, I am still a bit skeptical. So you are saying that swap_aux_in is supposed to copy all aux entries into one internal_auxent structure? In xcofflink.c:4490, functio xcoff_link_input_bfd, which is where I encountered the stack smashage, swap_aux_in is invoked in a loop over all n_numaux entries. Swap in one aux entry, do something, swap out the one aux entry, advance to next aux entry. The corresponding swap_aux_out function only copies back one at a time; it does not somehow extract multiple aux entries out of a single internal_auxent. In fact, I have not seen any other code that indicates that an iternal_auxent might correspond to more than one external_auxent. The code in xcofflink.c makes perfect sense to me under the assumption that swap_aux_in is supposed to copy only one aux entry, and no sense at all with the present implementation of swap_aux_in. The swap_aux_in function as written also leaves the internal_auxent structure entirely uninitialized if index is not null, which is really scary (and technically UB as well). Also, if the string contents of the AUX entry fit within 14 (E_FILNMLEN) characters, the string is stored inline, otherwise it is stored as a reference to the string table. In the string table case, swap_aux_in only copies the pointers for the current aux entry. The loop that calls it from xcoff_link_input_bfd handles the string table reference after the call. If the first aux entry contains an inline string (<= 14 bytes), then all entries are written into the auxent. -- You are receiving this mail because: You are on the CC list for the bug. ___ bug-binutils mailing list bug-binutils@gnu.org https://lists.gnu.org/mailman/listinfo/bug-binutils
[Bug ld/24055] _bfd_xcoff_swap_aux_in smashes the stack
https://sourceware.org/bugzilla/show_bug.cgi?id=24055 Nick Clifton changed: What|Removed |Added CC||nickc at redhat dot com --- Comment #1 from Nick Clifton --- Created attachment 11507 --> https://sourceware.org/bugzilla/attachment.cgi?id=11507=edit Proposed patch Hi Wolfgang, I agree that this is a problem, but I do not think that your patch will work. It will fail to copy auxiliary entries for file symbols, if that symbol has more than 1 auxiliary entry. I think that the issue is probably that file symbols should never have more than one or two auxiliary entries, although I could not find an actual specification of that rule. Please could you try out this alternate patch, which does not actually solve the problem, but it should generate an abort rather than allowing the stack to be smashed. (I am hoping that this will allow you to trace the reason for the abort back to a maformed file symbol). If the patch works as I hope, then I would also apply it to the other versions of the swap_aux_in() function that use the same memcpy functionality. Cheers Nick -- You are receiving this mail because: You are on the CC list for the bug. ___ bug-binutils mailing list bug-binutils@gnu.org https://lists.gnu.org/mailman/listinfo/bug-binutils
[Bug ld/24055] _bfd_xcoff_swap_aux_in smashes the stack
https://sourceware.org/bugzilla/show_bug.cgi?id=24055 Wolfgang Thaller changed: What|Removed |Added CC||wolfgang.thaller at gmx dot net -- You are receiving this mail because: You are on the CC list for the bug. ___ bug-binutils mailing list bug-binutils@gnu.org https://lists.gnu.org/mailman/listinfo/bug-binutils
