NetBSD Problem Report #49208

From khorben@defora.org  Wed Sep 17 00:00:20 2014
Return-Path: <khorben@defora.org>
Received: from mail.netbsd.org (mail.netbsd.org [149.20.53.66])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(Client CN "mail.netbsd.org", Issuer "Postmaster NetBSD.org" (verified OK))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 2941FCE2F5
	for <gnats-bugs@gnats.NetBSD.org>; Wed, 17 Sep 2014 00:00:20 +0000 (UTC)
Message-Id: <20140916235752.B166319A3D@sleak.defora.rom>
Date: Wed, 17 Sep 2014 01:57:52 +0200 (CEST)
From: Pierre Pronchery <khorben@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Subject: The first-stage FAT bootloader does not work
X-Send-Pr-Version: 3.95

>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:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Wed Sep 17 00:05:00 +0000 2014
>Closed-Date:    Mon Oct 27 11:37:23 +0000 2014
>Last-Modified:  Mon Oct 27 11:37:23 +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:/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>
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:

>Release-Note:

>Audit-Trail:
From: "Pierre Pronchery" <khorben@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/49208 CVS commit: src/common/lib/libc/arch/i386/string/small
Date: Mon, 22 Sep 2014 20:31:56 +0000

 Module Name:	src
 Committed By:	khorben
 Date:		Mon Sep 22 20:31:56 UTC 2014

 Modified Files:
 	src/common/lib/libc/arch/i386/string/small: strchr.S

 Log Message:
 Look for the character to locate before checking for the NUL character

 As documented in PR port-i386/49208, this fixes strchr(s, '\0'), as used by
 the FAT first-stage bootloader on x86 (bootxx_msdos).
 strchr(s, '\0') is otherwise equivalent to strlen(string), which would
 probably look nicer in the original file, dosfs.c from libsa.

 Confirmed working in qemu and on real hardware.
 ok joerg@

 XXX pull-up to netbsd-6 and netbsd-7


 To generate a diff of this commit:
 cvs rdiff -u -r1.2 -r1.3 src/common/lib/libc/arch/i386/string/small/strchr.S

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: open->pending-pullups
State-Changed-By: khorben@NetBSD.org
State-Changed-When: Fri, 26 Sep 2014 11:43:38 +0000
State-Changed-Why:
See pull-ups #1158 for netbsd-6 and #118 for netbsd-7


From: "SAITOH Masanobu" <msaitoh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/49208 CVS commit: [netbsd-7] src/common/lib/libc/arch/i386/string/small
Date: Mon, 29 Sep 2014 15:31:01 +0000

 Module Name:	src
 Committed By:	msaitoh
 Date:		Mon Sep 29 15:31:01 UTC 2014

 Modified Files:
 	src/common/lib/libc/arch/i386/string/small [netbsd-7]: strchr.S

 Log Message:
 Pull up following revision(s) (requested by khorben in ticket #118):
 	common/lib/libc/arch/i386/string/small/strchr.S: revision 1.3
 Look for the character to locate before checking for the NUL character
 As documented in PR port-i386/49208, this fixes strchr(s, '\0'), as used by
 the FAT first-stage bootloader on x86 (bootxx_msdos).
 strchr(s, '\0') is otherwise equivalent to strlen(string), which would
 probably look nicer in the original file, dosfs.c from libsa.
 Confirmed working in qemu and on real hardware.
 ok joerg@
 XXX pull-up to netbsd-6 and netbsd-7


 To generate a diff of this commit:
 cvs rdiff -u -r1.2 -r1.2.4.1 \
     src/common/lib/libc/arch/i386/string/small/strchr.S

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: "SAITOH Masanobu" <msaitoh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/49208 CVS commit: [netbsd-6] src/common/lib/libc/arch/i386/string/small
Date: Mon, 27 Oct 2014 10:51:16 +0000

 Module Name:	src
 Committed By:	msaitoh
 Date:		Mon Oct 27 10:51:16 UTC 2014

 Modified Files:
 	src/common/lib/libc/arch/i386/string/small [netbsd-6]: strchr.S

 Log Message:
 Pull up following revision(s) (requested by khorben in ticket #1158):
 	common/lib/libc/arch/i386/string/small/strchr.S: revision 1.3
 Look for the character to locate before checking for the NUL character
 As documented in PR port-i386/49208, this fixes strchr(s, '\0'), as used by
 the FAT first-stage bootloader on x86 (bootxx_msdos).
 strchr(s, '\0') is otherwise equivalent to strlen(string), which would
 probably look nicer in the original file, dosfs.c from libsa.
 Confirmed working in qemu and on real hardware.
 ok joerg@
 XXX pull-up to netbsd-6 and netbsd-7


 To generate a diff of this commit:
 cvs rdiff -u -r1.1 -r1.1.4.1 \
     src/common/lib/libc/arch/i386/string/small/strchr.S

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: pending-pullups->closed
State-Changed-By: msaitoh@NetBSD.org
State-Changed-When: Mon, 27 Oct 2014 11:37:23 +0000
State-Changed-Why:
Fixed and pulled up to netbsd-[67].
Thanks.


>Unformatted:

NetBSD Home
NetBSD PR Database Search

(Contact us) $NetBSD: query-full-pr,v 1.39 2013/11/01 18:47:49 spz Exp $
$NetBSD: gnats_config.sh,v 1.8 2006/05/07 09:23:38 tsutsui Exp $
Copyright © 1994-2014 The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.