port-i386/49208: The first-stage FAT bootloader does not work

[Pierre Pronchery <khorben%netbsd.org@localhost>]

>Number:         49208
>Category:       port-i386
>Synopsis:       bootxx_msdos fails to load the second-stage bootloader
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    port-i386-maintainer
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Wed Sep 17 00:05:00 +0000 2014
>Originator:     Pierre Pronchery
>Release:        NetBSD 6.1_STABLE
>Organization:
The NetBSD Foundation
>Environment:
System: NetBSD sleak.defora.rom 6.1_STABLE NetBSD 6.1_STABLE (GENERIC) #1: Fri 
Aug 1 10:00:52 UTC 2014 
edgebsd%altar.edgebsd.org@localhost:/home/edgebsd/amd64/obj/sys/arch/amd64/compile/GENERIC
 amd64
Architecture: x86_64
Machine: amd64
>Description:
My latest attempts at booting NetBSD from a USB memory stick formatted with
either the FAT16 or FAT32 filesystem all failed with the following error at
boot-time: "Can't open /boot".

I identified the error message to be coming from
 src/sys/arch/i386/stand/bootxx/boot1.c:

 62 const char *
 63 boot1(uint32_t biosdev, uint64_t *sector)
 64 {
[...]
109         /* if we fail here, so will fstat, so keep going */
110         if (fd == -1 || fstat(fd, &sb) == -1)
111                 return "Can't open /boot\r\n";

Digging through the code, and after planting a few printf()'s^W^W^Wputstr()'s
in strategic places, I found the problem to originate from
src/sys/lib/libsa/dosfs.c:

470 static int
471 namede(DOS_FS *fs, const char *path, const struct direntry **dep)
472 {
[...]
483         while (*path) {
484                 if (!(s = strchr(path, '/')))
485                         s = strchr(path, 0);
486                 if ((n = s - path) > 255)
487                         return ENAMETOOLONG;

Where strchr("boot", 0) on line 485 would unexpectedly return NULL and the
following check underflow. A quick disassembly of strchr() from
$OBJDIR/sys/arch/i386/stand/bootxx/bootxx_msdos/lib/kern/libkern.a(strchr.o)
(as bootxx_msdos.map suggests) showed that the size-optimized i386 assembly
version of strchr() checks for the string termination character (thus
returning NULL) before checking that the NUL character is not what is actually
the subject of the search:

 34 ENTRY(strchr)
[...]
 41 1:
 42         cmpb    %cl, 0(%eax)
 43         je 3f
 44         cmpb    $0, 0(%eax)
 45         je 2f

Inverting the checks lines 42-43 and 44-45 solved the problem for me.

>How-To-Repeat:
Follow the instructions from installboot(8) to create a bootable USB memory
stick:

[partition 0 with type 11, offset 63, take everything]
# fdisk -u sd0
[be sure the first partition is active]
# fdisk -fa0 sd0
[remove and insert the USB stick again to be sure the disklabel matches]
# disklabel sd0
[you should see the "e" partition of type "MSDOS"]
# newfs_msdos -F 32 -r 16 sd0e
[it should say "MBR type: 11" just like we already selected]
# mkdir -p /mnt/usb
# mount /dev/sd0e /mnt/usb
# cp /usr/mdec/boot /mnt/usb/boot
# fetch -o /mnt/usb/netbsd.gz \
  
http://ftp.edgebsd.org/pub/EdgeBSD/EdgeBSD-7/amd64/binary/kernel/netbsd-INSTALL.gz
# umount /mnt/usb
# installboot -t raw /dev/rsd0e /usr/mdec/bootxx_msdos

It is also possible to use vnconfig(8) and test from qemu(1), which was very
useful during my tests:

$ dd if=/dev/zero of=msdos.img bs=512 count=200000 conv=sparse
$ sudo vnconfig -c vnd0 msdos.img
[perform all of the above, using vnd0 instead of sd0]
$ sudo vnconfig -u vnd0
$ qemu-system-x86_64 -net none -boot once=c,strict=on msdos.img
[this disables network boot to better see the messages from the bootloader]

>Fix:
This patch solved the issue for me. Let me know if I can commit it.
If so I will request pull-ups as appropriate.

commit 7fce87e97f920fae760e4fccd4db25c382784211
Author: Pierre Pronchery <khorben%EdgeBSD.org@localhost>
Date:   Tue Sep 16 19:43:09 2014 +0200

    Test for the character to match before testing for end of string
    
    This fixes strchr(string, '\0') for the users of this file, like
    bootxx_msdos which did not work without this fix.

diff --git a/common/lib/libc/arch/i386/string/small/strchr.S 
b/common/lib/libc/arch/i386/string/small/strchr.S
index 5b33347..bfa3461 100644
--- a/common/lib/libc/arch/i386/string/small/strchr.S
+++ b/common/lib/libc/arch/i386/string/small/strchr.S
@@ -39,10 +39,10 @@ ENTRY(strchr)
        pushl   %eax
        pushl   %edx
 1:
-       cmpb    $0, 0(%eax)
-       je 2f
        cmpb    %cl, 0(%eax)
        je 3f
+       cmpb    $0, 0(%eax)
+       je 2f
        incl    %eax
        jmp 1b
 2: