[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openbsd-tech
Subject:    Re: efiboot: fix PXE -> disk booting
From:       Mark Kettenis <mark.kettenis () xs4all ! nl>
Date:       2019-04-24 20:52:13
Message-ID: 416167e124b5eecc () bloch ! sibelius ! xs4all ! nl
[Download RAW message or body]

> Date: Wed, 24 Apr 2019 22:32:29 +0200
> From: Christian Weisgerber <naddy@mips.inka.de>
> 
> If you boot efiboot via PXE and at the prompt enter a different boot
> device, e.g.
> 
>   boot> boot sd0a:/bsd
> 
> then efiboot will pretend to honor this
> 
>   booting sd0a:/bsd: ...
> 
> but in fact ignore the device and boot from TFTP instead.
> 
> 
> In the MI part of the boot loader, open() calls devopen(), which
> picks the right device from the MD devsw[] table in conf.c.  open()
> will then try the xxx_open() routines from the file_system[] table
> until one succeeds.  This assumes that devices and filesystems are
> entirely orthogonal, which is a bit iffy since the TFTP and disk
> code are independent.  tftp_open() succeeds "on" a disk device that
> it never looks at, resulting in the bug above.
> 
> The patch below
> (1) adds a check to tftp_open() that we are actually opening a TFTP
>     device;
> (2) clears the bootmac variable in devopen() for non-TFTP devices,
>     to prevent the kernel from going into netboot mode.  (Woe, where
>     is my root NFS?)
> 
> Tested with arm64 BOOTAA64.EFI and amd64 BOOTX64.EFI.
> The arm64 code can be copied verbatim to armv7.  Not done below,
> and not tested for lack of hardware.
> 
> amd64/i386 pxeboot already solves this problem with a slightly
> different approach.  It has an MD version of open() that allows
> devopen() to directly set the specific file_system[] entry to be
> used.  We could pull this modification to open() into the MI code,
> but the contortions in devopen() won't get any prettier.
> 
> 
> Opinions?
> OKs (including dup'ing the arm64 code to armv7)?

My mind always parses

   if (strcmp(a, b))

to have the opposite meaning.  Please consider changing this into

  if (strcmp(a, b) != 0)

Please do commit the arm64 change to armv7 as well.

> Index: sys/arch/arm64/stand/efiboot/efiboot.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/arm64/stand/efiboot/efiboot.c,v
> retrieving revision 1.22
> diff -u -p -r1.22 efiboot.c
> --- sys/arch/arm64/stand/efiboot/efiboot.c	31 Jan 2019 14:35:06 -0000	1.22
> +++ sys/arch/arm64/stand/efiboot/efiboot.c	24 Apr 2019 15:35:51 -0000
> @@ -749,6 +749,14 @@ devopen(struct open_file *f, const char 
>  	dp = &devsw[dev];
>  	f->f_dev = dp;
>  
> +	if (strcmp("tftp", dp->dv_name)) {
> +		/*
> +		 * Clear bootmac, to signal that we loaded this file from a
> +		 * non-network device.
> +		 */
> +		bootmac = NULL;
> +	}
> +
>  	return (*dp->dv_open)(f, unit, part);
>  }
>  
> Index: sys/arch/arm64/stand/efiboot/efipxe.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/arm64/stand/efiboot/efipxe.c,v
> retrieving revision 1.4
> diff -u -p -r1.4 efipxe.c
> --- sys/arch/arm64/stand/efiboot/efipxe.c	31 Mar 2018 17:43:53 -0000	1.4
> +++ sys/arch/arm64/stand/efiboot/efipxe.c	24 Apr 2019 15:00:39 -0000
> @@ -157,6 +157,9 @@ mtftp_open(char *path, struct open_file 
>  	EFI_STATUS status;
>  	UINT64 size;
>  
> +	if (strcmp("tftp", f->f_dev->dv_name))
> +		return ENXIO;
> +
>  	if (PXE == NULL)
>  		return ENXIO;
>  
> @@ -295,6 +298,9 @@ mtftp_readdir(struct open_file *f, char 
>  int
>  efitftp_open(char *path, struct open_file *f)
>  {
> +	if (strcmp("tftp", f->f_dev->dv_name))
> +		return ENXIO;
> +
>  	if (NET == NULL || PXE == NULL)
>  		return ENXIO;
>  
> Index: sys/arch/amd64/stand/efiboot/efipxe.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/amd64/stand/efiboot/efipxe.c,v
> retrieving revision 1.3
> diff -u -p -r1.3 efipxe.c
> --- sys/arch/amd64/stand/efiboot/efipxe.c	30 Jan 2018 20:19:06 -0000	1.3
> +++ sys/arch/amd64/stand/efiboot/efipxe.c	24 Apr 2019 19:33:06 -0000
> @@ -127,6 +127,9 @@ tftp_open(char *path, struct open_file *
>  	EFI_STATUS status;
>  	UINT64 size;
>  
> +	if (strcmp("TFTP", f->f_dev->dv_name))
> +		return ENXIO;
> +
>  	if (PXE == NULL)
>  		return ENXIO;
>  
> Index: sys/arch/amd64/stand/libsa/dev_i386.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/amd64/stand/libsa/dev_i386.c,v
> retrieving revision 1.21
> diff -u -p -r1.21 dev_i386.c
> --- sys/arch/amd64/stand/libsa/dev_i386.c	25 Nov 2017 19:02:07 -0000	1.21
> +++ sys/arch/amd64/stand/libsa/dev_i386.c	24 Apr 2019 19:40:47 -0000
> @@ -78,6 +78,16 @@ devopen(struct open_file *f, const char 
>  #endif
>  		if ((rc = (*dp->dv_open)(f, file)) == 0) {
>  			f->f_dev = dp;
> +#ifdef EFIBOOT
> +			if (strcmp("TFTP", dp->dv_name)) {
> +				/*
> +				 * Clear bootmac, to signal that we loaded
> +				 * this file from a non-network device.
> +				 */
> +				extern char *bootmac;
> +				bootmac = NULL;
> +			}
> +#endif
>  			return 0;
>  		}
>  #ifdef DEBUG
> -- 
> Christian "naddy" Weisgerber                          naddy@mips.inka.de
> 
> 

[prev in list] [next in list] [prev in thread] [next in thread]