bugGNU GRUB - Bugs: bug #36672, GRUB fails to read FAT filesystem...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #36672: GRUB fails to read FAT filesystem label

Submitter:  <qwertial>
Submitted:  Sun 17 Jun 2012 08:52:24 AM UTC
   
 
Category:  None Severity:  Major
Priority:  5 - Normal Item Group:  Software Error
Status:  Fixed Privacy:  Public
Assigned to:  None Originator Name: 
Open/Closed:  Closed Release:  Bazaar - trunk
Release:  Reproducibility:  None
Planned Release:  None

Jump to the original submission

Mon 25 Jun 2012 02:22:14 PM UTC, comment #10: 

Seems it does. Thank you.

<qwertial>
Mon 25 Jun 2012 09:18:19 AM UTC, comment #9: 

This patch should help

(file #26103)

Vladimir Serbinenko <phcoder>
Group administrator
Sun 24 Jun 2012 08:04:38 PM UTC, comment #8: 

MS-DOS 6.22 labeling utility lets me use spaces. And it does mark the record with the "archive" bit set. (And actually, raw DOS API lets one create regular files with spaces in their names, without VFAT - they just were not very popular, because of how command line works in there.)

My copy of dosfstools lets me put any character in a label. I guess it should not, but I like it. MS-DOS and FreeDOS read it without complaining. A more modern operating system might not, but I doubt it.

$ mkdosfs -n '/;^<>' test.img
mkdosfs 3.0.12 (29 Oct 2011)
$ dosfslabel test.img
/;^<>
$ dosfslabel test.img :?*
$ dosfslabel test.img
:?*
$ dosfsck test.img
dosfsck 3.0.12, 29 Oct 2011, FAT32, LFN
test.img: 1 files, 1/1046526 clusters
$

<qwertial>
Sun 24 Jun 2012 06:18:36 PM UTC, comment #7: 

Well the problem with the industry standards like FAT is what is actually correct. mkdosfs actually rejects anything you said. QEMU is unfortunately known not to be very accurate. What other programs create such a FAT?

Vladimir Serbinenko <phcoder>
Group administrator
Sun 24 Jun 2012 04:03:23 PM UTC, comment #6: 

Sorry for the delay.

GRUB no longer crashes, which is certainly an improvement. Yet it still does not work perfectly, because some FAT volume label records have not only the "volume label" bit set in their attribute bitfield, but also "archive"; even QEMU does that in their FAT emulation code. FreeDOS ignores even "read-only" and "device" (0x40) bits on label records. No filesystem checking tool treats extra "archive" bits as errors. I already mentioned it before, I thought you would notice that.

GRUB will also stop reading the label after a space or other atypical character. Slashes may be borderline, but spaces are perfectly legal in labels.

DOS seems to be fine with reading all of the above correctly.

<qwertial>
Wed 20 Jun 2012 01:43:13 PM UTC, comment #5: 

It's usually pretty futile to avoid hitting compiler problems by playing with optimisations. Attached patch restructures functions to avoid trampoline here.

(file #26070)

Vladimir Serbinenko <phcoder>
Group administrator
Wed 20 Jun 2012 06:32:23 AM UTC, comment #4: 

I figured. And did just that.

Does the Debian build disable optimizations? Because some fiddling with the compiler reveals that enabling optimizations is crucial for this to work. Um, fail to work. It seems this off-by-one jump is deliberate. When a nested routine is called from the containing function, the context pointer is passed in %esi, which is pushed onto the stack in the routine and then read from there. When doing a trampoline call, it is the trampoline which pushes the context pointer onto the stack, so it needs to skip the push instruction in the routine, which happens to be one byte long, and at the very beginning of the function... Except when we are optimizing, in which case all bets are off. I was unable to find out which particular optimization causes this, but -O1 is enough to trigger the bug.

A workaround is to disable optimizations, preferably using _attribute_((optimize(0))).

As for the GRUB problem, it looks like file system labels on partitions I tested are marked with attribute value 0x28 (archive and volume label), rather than 0x08 (just label). At least that is what iter_hook receives from its caller. FreeDOS, dosfstools and blkid seem to have no problems with that. Ignoring archive and read-only attributes should do it.

I think I did more than enough; and I really should not be spending any more time on this bug. You know what needs to be done.


<qwertial>
Tue 19 Jun 2012 05:37:41 PM UTC, comment #3: 

GRUB GDB stub isn't very good and lacks several abilities. It's better to use qemu stub.

Vladimir Serbinenko <phcoder>
Group administrator
Tue 19 Jun 2012 03:05:16 PM UTC, comment #2: 

Strange. I recompiled trunk GRUB from scratch with a freshly installed gcc 4.7.1 (Arch), built a minimal image, ran it in QEMU connected to GDB and I still see the same wrongly-generated code. How did you test?

While we are at it, the load_all_modules macro in the gdb_grub script does not work unless "$this->mod" is changed to "$this". Even with a fixed script, however, GDB is unusably slow and fragile when connecting to GRUB's own stub through QEMU's serial emulation. I do not know whose fault is that.

And well, even if it is purely a compiler issue, it would be nice to add a testcase or two (or perhaps a test in ./configure) to check if trampolines are b0rken.

<qwertial>
Mon 18 Jun 2012 09:19:14 PM UTC, comment #1: 

Hm, it seems to happen under Fedora with 4.7.0 but not under Debian with 4.7.1 which suggest that bug is perhaps fixed in 4.7.1

Vladimir Serbinenko <phcoder>
Group administrator
Sun 17 Jun 2012 08:52:24 AM UTC, original submission:  

Steps to reproduce:
1. Start GRUB on a system with a disk containing a FAT partition with a label. It may even be QEMU with a VVFAT.
2. ls -l
3. Stare at the machine in confusion as it reboots.

As I investigated, it turned out that the root problem is probably a compiler bug, so it probably should be sent to the GCC team, but...

When gdb.mod is loaded, GRUB catches exception 0xd (GPF) and prints a stack trace pointing to the FAT module. From what I have found out, GRUB returns to an invalid address from iter_hook inside grub_fat_label. While digging through the assembly, I found that the jump target in the trampoline points to iter_hook+1 instead of iter_hook. Here is the disassembly for iter_hook:

=> 0x7ffa644 <iter_hook>: cmpb   $0x8,0xb(%edx)
   0x7ffa648 <iter_hook+4>: jne    0x7ffa662 <iter_hook+30>
   0x7ffa64a <iter_hook+6>: push   %esi
   0x7ffa64b <iter_hook+7>: push   %ebp
   0x7ffa64c <iter_hook+8>: mov    %esp,%ebp
   0x7ffa64e <iter_hook+10>: push   %ecx
   0x7ffa64f <iter_hook+11>: call   0xd120 <grub_strdup>
   0x7ffa654 <iter_hook+16>: mov    0x4(%ebp),%edx
   0x7ffa657 <iter_hook+19>: mov    (%edx),%edx
   0x7ffa659 <iter_hook+21>: mov    %eax,(%edx)
   0x7ffa65b <iter_hook+23>: mov    $0x1,%eax
   0x7ffa660 <iter_hook+28>: jmp    0x7ffa665 <iter_hook+33>
   0x7ffa662 <iter_hook+30>: xor    %eax,%eax
   0x7ffa664 <iter_hook+32>: ret   
   0x7ffa665 <iter_hook+33>: leave 
   0x7ffa666 <iter_hook+34>: pop    %edx
   0x7ffa667 <iter_hook+35>: ret

Before jumping into iter_hook, the trampoline pushes a pointer to the enclosing function context, but as we can see from disassembly, iter_hook does not pop that pointer at all (and it seems to refer to it wrongly). It seems that this routine is not the only one for which wrong trampoline is generated, but because most of them start with a one-byte push instruction, the resulting stack pointer misadjustments cancel out. This one does not, but it resynchronises with the correct instruction stream quite quickly:

   0x7ffa645 <iter_hook+1>: jp     0x7ffa652 <iter_hook+14>
   0x7ffa647 <iter_hook+3>: or     %dh,0x18(%ebp)
   0x7ffa64a <iter_hook+6>: push   %esi
   0x7ffa64b <iter_hook+7>: push   %ebp
   0x7ffa64c <iter_hook+8>: mov    %esp,%ebp

Compiling with gcc version 4.7.0 20120505 (prerelease), on x86_64-unknown-linux-gnu, targeting i386-pc.

Interestingly, when manually fixing the return address, or when using the x86_64-efi version, GRUB tells me that "filesystem `fat' does not support labels"; and the grub-probe tool (on x86_64) prints "(nil)" as the filesystem label. So the compiler bug may be not the only one here, or at least not just on i386.

<qwertial>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #26103:  fat.diff added by phcoder (2KiB - text/x-diff)
file #26070:  fat.diff added by phcoder (13KiB - text/x-diff)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by qwertial (Posted a comment)
  • -email is unavailable- added by phcoder (Posted a comment)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2012-06-25 phcoder Attached File- Added fat.diff, #26103
    2012-06-20 phcoder StatusNone Fixed
        Open/ClosedOpen Closed
    2012-06-20 phcoder Attached File- Added fat.diff, #26070
    2012-06-19 qwertial Carbon-CopyRemoved 88440 -

    Back to the top

    Powered by Savane 3.13-caa5.
    Corresponding source code