Details

Reviewers

imp
stevek
kevans
manu

Commits

rG5bfb3045d25b: Allow secure-netboot

Summary

When doing file verification, tftp needs to be able to handle multiple
open files concurrently.
We also need tftp_stat() to provide useful values for st_dev and st_ino.

Allow an architecture to define NETPROTO_DEFAULT.
The default is NET_NFS for backwards compatability.

In net_parse_rootpath() fix parsing of
<scheme>://<ip>[:<port]/<path>
and ensure we return INADDR_NONE unless we successfully
parsed an addr, so we don't end up clobbering rootip obtained
from bootp().

In install() if proto has not been determined, but
netproto is NET_TFTP set proto to tftp_fsops.

Put a common DEBUG_PRINTF impl in stand.h so we can over time have
more consistent approach to debugging bits of the loader.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

sjg created this revision.Jul 6 2025, 11:54 PM

Herald added a reviewer: manu. · View Herald TranscriptJul 6 2025, 11:54 PM

Herald added a subscriber: dab. · View Herald Transcript

sjg requested review of this revision.Jul 6 2025, 11:54 PM

Harbormaster completed remote builds in B65247: Diff 158075.Jul 6 2025, 11:54 PM

sjg mentioned this in D51094: Fix secure netboot with UEFI.Jul 6 2025, 11:55 PM

Can you do the debug level stuff first so we get that out of the way? There's also a lot of different fixes bundled together which makes me super nervous for future bisecting...

stand/common/dev_net.c
311	This is the only non debug level change. Please do the debug level stuff as a separate commit.
stand/libsa/open.c
164 ↗	(On Diff #158075)	Ugg why? Seems like an extreme layering violation... this whole force it so do devopen is super weird too. Also why? I really hate hacks like this... almost all the other ones like it have bit my a** at some point...

In D51187#1170063, @imp wrote:

Can you do the debug level stuff first so we get that out of the way? There's also a lot of different fixes bundled together which makes me super nervous for future bisecting...

See D51269 since it is purely DEBUG_PRINTF (as used by libsecureboot) I added the fixes for pkgfs.c and bootp.c

Rebase after commit DEBUG_PRINTF

Harbormaster completed remote builds in B65545: Diff 158717.Jul 17 2025, 11:54 PM

sjg added inline comments.Jul 18 2025, 12:17 AM

stand/libsa/open.c

164 ↗

(On Diff #158075)

that is a good question, need to do some archeology this was originally a marcel change that got merged in 2017

Will see what I can find, I can confirm PXE boot breaks without this:

Testing hash: sha256                            Passed
Testing hash: sha384                            Passed
Testing verify certificate: EngineeringEcCA     Passed
Verified /boot/../manifest signed by PackageDevelopmentECP256_2021
Verified /boot/boot.4th
Junos PXE installer bootstrap for EFI
Trying: /935db760-789f-11eb-9f46-6c0b84655934.tgz...
Trying: /58-9c-fc-00-74-c6.tgz...
Trying: /C0A84544.tgz...
Trying: /C0A8454.tgz...
Trying: /C0A845.tgz...
Trying: /C0A84.tgz...
Trying: /C0A8.tgz...
Trying: /C0A.tgz...
Trying: /C0.tgz...
Trying: /C.tgz...
Trying: /default.tgz...
Verified /manifest signed by PackageDevelopmentECP256_2021

can't load 'kernel'

Type '?' for a list of commands, 'help' for more detailed help.

Setting currdev to net0:
Testing hash: sha1                              Passed
Testing hash: sha256                            Passed
Testing hash: sha384                            Passed
Testing verify certificate: EngineeringEcCA     Passed
Verified /boot/../manifest signed by PackageDevelopmentECP256_2021
Verified /boot/boot.4th
Junos PXE installer bootstrap for EFI
Trying: /935db760-789f-11eb-9f46-6c0b84655934.tgz...
Trying: /58-9c-fc-00-74-c6.tgz...
Trying: /C0A84544.tgz...
Trying: /C0A8454.tgz...
Trying: /C0A845.tgz...
Trying: /C0A84.tgz...
Trying: /C0A8.tgz...
Trying: /C0A.tgz...
Trying: /C0.tgz...
Trying: /C.tgz...
Trying: /default.tgz...
Verified /manifest signed by PackageDevelopmentECP256_2021
Verified /manifest signed by PackageDevelopmentECP256_2023
/kernel text=0xc65e8 text=0x5e28f0 text=0xe05e4 data=0x140 data=0x556100 0x8+0xd6a70+0x8+0xb972f/
Verified /kernel
/crypto.ko size 0x48820 at 0x10e8000
Verified /crypto.ko
/ichwd.ko size 0x6a70 at 0x1131000
Verified /ichwd.ko
/iflib.ko size 0x1f1f0 at 0x1138000
Verified /iflib.ko
/miibus.ko size 0x385d8 at 0x1158000
Verified /miibus.ko
/if_em.ko size 0x697c8 at 0x1191000
Verified /if_em.ko
/if_fxp.ko size 0xf3a8 at 0x11fb000
Verified /if_fxp.ko
/virtio.ko size 0x5340 at 0x120b000
Verified /virtio.ko

etc

What about adding another field to fs_ops so a flag like skip devopen can be set?

Alternative means of skipping devopen for pkgfs

Harbormaster completed remote builds in B65547: Diff 158720.Jul 18 2025, 12:58 AM

Make the code in open() neater

Harbormaster completed remote builds in B65566: Diff 158749.Jul 18 2025, 4:09 PM

imp added inline comments.Jul 18 2025, 5:19 PM

stand/common/dev_net.c
418	the trip through uinptr_t I think isn't needed. portp and ptr are both char * and can just be subtracted. Maybe the cast to (int) after is still needed, though...
432	The changes in this file, I think, should be a separate commit, but no real need to break them down further.
stand/common/install.c
141 ↗	(On Diff #158749)	This should be a separate commit, since it adds a different thing (even if it's related).
295 ↗	(On Diff #158749)	Is this fallback still needed? If so, document why and commit it separately.
stand/common/misc.c
204 ↗	(On Diff #158749)	Why is NET special here? We do the same env_setenv if the mount succeeds, with an unmount if there was a previous value. Why does net need to skip that unmount?
stand/libsa/globals.c
20	The = 0 is redundant.
stand/libsa/open.c
159 ↗	(On Diff #158749)	So I really don't understand this. for pkgfs, exclusive_file_system will start out non-NULL, since we're opening the package (with exclsuive_filesyste_set just before the open. We then set it back to NULL and do new_package (which just reads the file that opened, so there's no opens. We then set the exclusive_fielsystem to pkgfs, but also set the _devopen bool to 1 in that case. So, we always go through devopen for all future opens, when before we wouldn't. If that succeeds, then we now have a handle on the raw device... then we go to the overriding filesystem. And I'm not even sure I have all the double negatives parsed right, and the _devopen is ever unset unless another package comes in. This seems way too convoluted. Seems a simpler to have a flag to communicate something like this, as you've suggested below.
stand/libsa/tftp.c
903	These changes look fairly independent... You could split the debugging stuff out, but there's not likely to be a need to bisect around it, so I wouldn't bother splitting below this file level.

sjg marked 3 inline comments as done.Jul 18 2025, 10:12 PM

sjg added inline comments.

stand/common/dev_net.c
432	I don't mind breaking into separate commits, but at this point would like to avoid additional reviews, if the code itself is ok.
stand/common/install.c
295 ↗	(On Diff #158749)	We can get to here via PXE boot, or via an explicit install command at loader prompt, and possibly one other route on u-boot platforms. IIRC one of those can get us here with proto NULL Will add a comment.
stand/common/misc.c
204 ↗	(On Diff #158749)	Its the mount we're skipping - which makes no sense via tftp. With a FreeBSD tftp server the attempt doesn't do a lot of harm, but with other servers, it can result in a 15 minute delay in booting which is not acceptible.
stand/libsa/open.c
159 ↗	(On Diff #158749)	So I really don't understand this. for pkgfs, exclusive_file_system will start out non-NULL, since we're opening the package (with exclsuive_filesyste_set just before the open. It is complicated for sure. pkgfs_init can be called with proto initially set to something other than pkgfs_fsops, and if so we need to do the devopen (eg for net0:) but for pkgfs_fsops devopen will not work. We then set it back to NULL and do new_package (which just reads the file that opened, so there's no opens. We then set the exclusive_fielsystem to pkgfs, but also set the _devopen bool to 1 in that case. So, we always go through devopen for all future opens, when before we wouldn't. If that succeeds, then we now have a handle on the raw device... then we go to the overriding filesystem. And I'm not even sure I have all the double negatives parsed right, and the _devopen is ever unset unless another package comes in. This seems way too convoluted. Seems a simpler to have a flag to communicate something like this, as you've suggested below. I'm ok with adding a flag to fsops - this was at minimum an experiment to verify that approach would actually work.