On 20-06-05 22:42:17, Klemens Nanni wrote:
> On Mon, Jun 01, 2020 at 11:48:05PM +0200, Klemens Nanni wrote:
> > Installing an unstripped boot loader on softraid on sparc64 fails
> > without proper error message.
> >
> > Make BIOCINSTALLBOOT return a proper errno, make installboot(8) use it
> > to provide proper usage errors plus unique code paths for debugging.
> >
> > At first, I made sr_ioctl_installboot() use sr_error() in the relevant
> > spot and this made the kernel print my errors, however the following
> > piece in softraid.c:sr_bio_handler() means using sr_error() will hide
> > the error code from the ioctl(2) call, i.e. installboot(8) would
> > report no error message on stderr and exit zero:
> >
> > if (sc->sc_status.bs_msg_count > 0)
> > rv = 0;
> >
> > So instead, use a proper errno that yields a simple
> >
> > # ./obj/installboot sd2 /usr/mdec/bootblk /ofwboot.new
> > installboot.sr: softraid installboot failed: File too large
> >
> >
> >
> > Background: I built ofwboot on one machine ("make" without
> > "make install", then copy obj/ofwboot over), the resulting executable
> > was not stripped during build (happens during "make install") and was
> > about twice as big:
> >
> > # ls -l /ofwboot*
> > -rw-r--r-- 1 root wheel 106688 May 23 22:42 /ofwboot
> > -rwxr-xr-x 1 kn wheel 272452 May 24 00:20 /ofwboot.new
> > -rwxr-xr-x 1 root wheel 106752 May 24 01:21 /ofwboot.new.strip
> >
> > It took me longer than anticipated to find out that installboot(8)
> > fails beause my new boot loader was too big:
> >
> > # installboot sd2 /usr/mdec/bootblk /ofwboot.new
> > installboot: softraid installboot failed
> >
> > # installboot -v sd2 /usr/mdec/bootblk /ofwboot.new
> > Using / as root
> > installing bootstrap on /dev/rsd2c
> > using first-stage /usr/mdec/bootblk, second-stage /ofwboot.new
> > boot block is 6882 bytes (14 blocks @ 512 bytes = 7168 bytes)
> > sd2: softraid volume with 1 disk(s)
> > sd2: installing boot loader on softraid volume
> > installboot: softraid installboot failed
> >
> > That's it, no details or additional messages from the kernel.
> > While this was primarily my own fault, perhaps there are more legitimate
> > reasons foor bootblk/ofwboot builds to exceed their maximum size.
>
> In a i386 VM with root on crypto softraid, I built a much bigger second
> stage boot loader and performed the same tests as on sparc64: i386 does
> not try to install the bogus boot code due to checks in i386_softraid.c
> up front:
>
> # ls -l /usr/mdec/boot /boot.efbig
> -r-xr-xr-x 1 root bin 89728 Jun 5 03:02 /usr/mdec/boot
> -rwxr-xr-x 1 root wheel 176172 Jun 5 22:16 /boot.efbig
> # installboot -v sd1 /usr/mdec/biosboot /boot.efbig
> Using / as root
> installing bootstrap on /dev/rsd1c
> using first-stage /usr/mdec/biosboot, second-stage /boot.efbig
> sd1: softraid volume with 1 disk(s)
> installboot: boot code will not fit
>
> So for installboot(8) on sparc64 and i386 as the only two users of
> BIOCINSTALLBOOT, this makes root on softraid on sparc64 the only use
> that actually hits size checks in the ioctl code.
Keep in mind that the i386 installboot code is used on both i386
and amd64.
> sr_ioctl_installboot() seems inconsistent to me in how it reports some
> errors through sr_error() (causing ioctl() to return zero) and returing
> proper error codes (causing ioctl() and therefore installboot to fail);
sr_error() is used to add detail - the installboot code should be
checking and handling the case where bs->bs_status is non-zero.
IIRC the reason the ioctl has to succeed for this to work, is that
on failure there is no copyout().
> Assuming my EFBIG approach is still sensible (for sparc64), diff below
> adjusts the errx() call to err() in installboot for both platforms, even
> though i386 never reaches it.
>
> Feedback? OK?
While this works, you would be better off making use of the error
reporting mechanism that exists. A compile tested only diff for
the kernel side is below. A diff to installboot would be needed to
graft some code similar to that in bioctl's bio_status() function.
Index: softraid.c
===================================================================
RCS file: /cvs/src/sys/dev/softraid.c,v
retrieving revision 1.401
diff -u -p -u -p -r1.401 softraid.c
--- softraid.c 14 Apr 2020 07:38:21 -0000 1.401
+++ softraid.c 6 Jun 2020 14:32:56 -0000
@@ -3704,11 +3704,17 @@ sr_ioctl_installboot(struct sr_softc *sc
goto done;
}
- if (bb->bb_bootblk_size > SR_BOOT_BLOCKS_SIZE * DEV_BSIZE)
+ if (bb->bb_bootblk_size > SR_BOOT_BLOCKS_SIZE * DEV_BSIZE) {
+ sr_error(sc, "boot block is too large (%d > %d)",
+ bb->bb_bootblk_size, SR_BOOT_BLOCKS_SIZE * DEV_BSIZE);
goto done;
+ }
- if (bb->bb_bootldr_size > SR_BOOT_LOADER_SIZE * DEV_BSIZE)
+ if (bb->bb_bootldr_size > SR_BOOT_LOADER_SIZE * DEV_BSIZE) {
+ sr_error(sc, "boot loader is too large (%d > %d)",
+ bb->bb_bootldr_size, SR_BOOT_LOADER_SIZE * DEV_BSIZE);
goto done;
+ }
secsize = sd->sd_meta->ssdi.ssd_secsize;
