Page MenuHomeFreeBSD
Differential D51187

Allow secure-netboot
ClosedPublic
Actions

Authored by sjg on Jul 6 2025, 11:54 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Oct 7, 2:48 AM
Unknown Object (File)
Wed, Oct 7, 2:24 AM
Unknown Object (File)
Wed, Oct 7, 2:03 AM
Unknown Object (File)
Tue, Oct 6, 6:09 PM
Unknown Object (File)
Sat, Oct 3, 3:38 PM
Unknown Object (File)
Wed, Sep 23, 6:31 PM
Unknown Object (File)
Fri, Sep 18, 5:42 PM
Unknown Object (File)
Sep 9 2026, 12:35 AM
View All Files
Subscribers
dab
ziaee

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.

Sponsored by: Juniper Networks, Inc.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 65584
Build 62467: arc lint + arc unit

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
imp added a comment.Jul 11 2025, 12:21 AM

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
310

This is the only non debug level change. Please do the debug level stuff as a separate commit.

stand/libsa/open.c
181

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...

sjg added a comment.Jul 11 2025, 10:38 PM
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

sjg updated this revision to Diff 158717.Jul 17 2025, 11:54 PM

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
181

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.

vs

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?

sjg updated this revision to Diff 158720.Jul 18 2025, 12:58 AM

Alternative means of skipping devopen for pkgfs

Harbormaster completed remote builds in B65547: Diff 158720.Jul 18 2025, 12:58 AM
sjg updated this revision to Diff 158749.Jul 18 2025, 4:09 PM

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
411

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...

425

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

This should be a separate commit, since it adds a different thing (even if it's related).

295

Is this fallback still needed? If so, document why and commit it separately.

stand/common/misc.c
204–207

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
158

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
425

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

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–207

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
158

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.

sjg updated this revision to Diff 158763.Jul 18 2025, 10:32 PM

Add flag to fs_ops for skipping devopen

Harbormaster completed remote builds in B65570: Diff 158763.Jul 18 2025, 10:32 PM
sjg marked 2 inline comments as done.Jul 18 2025, 10:35 PM
sjg updated this revision to Diff 158795.Jul 19 2025, 8:58 PM

Mention pkgfs_fsops in libsa.3

Herald added a subscriber: ziaee. · View Herald TranscriptJul 19 2025, 8:58 PM
Harbormaster completed remote builds in B65584: Diff 158795.Jul 19 2025, 8:58 PM
sjg updated this revision to Diff 158845.Jul 20 2025, 11:11 PM

Move install.c change to D51446
Move misc.c change to D51447

Harbormaster completed remote builds in B65599: Diff 158845.Jul 20 2025, 11:11 PM
sjg updated this revision to Diff 159590.Aug 1 2025, 5:39 PM

Split out fs_ops.fs_flag to D51684

Harbormaster completed remote builds in B65902: Diff 159590.Aug 1 2025, 5:39 PM
This revision was not accepted when it landed; it landed in state Needs Review.Aug 20 2025, 10:56 PM
Closed by commit rG5bfb3045d25b: Allow secure-netboot (authored by sjg). · Explain Why
This revision was automatically updated to reflect the committed changes.
sjg added a commit: rG5bfb3045d25b: Allow secure-netboot.

Revision Contents
Changeset List

PathSize
stand/
common/
32 lines
4 lines
6 lines
2 lines
libsa/
1 line
1 line
1 line
1 line
1 line
1 line
4 lines
1 line
1 line
33 lines
1 line
1 line
3 lines
154 lines
1 line
2 lines

Diff 158795

View Options

stand/common/dev_net.c

Loading...
View Options

stand/common/install.c

Loading...
View Options

stand/common/misc.c

Loading...
View Options

stand/defs.mk

Loading...
View Options

stand/libsa/bzipfs.c

Loading...
View Options

stand/libsa/cd9660.c

Loading...
View Options

stand/libsa/dosfs.c

Loading...
View Options

stand/libsa/ext2fs.c

Loading...
View Options

stand/libsa/globals.c

Loading...
View Options

stand/libsa/gzipfs.c

Loading...
View Options

stand/libsa/libsa.3

Loading...
View Options

stand/libsa/net.h

Loading...
View Options

stand/libsa/nfs.c

Loading...
View Options

stand/libsa/open.c

Loading...
View Options

stand/libsa/pkgfs.c

Loading...
View Options

stand/libsa/splitfs.c

Loading...
View Options

stand/libsa/stand.h

Loading...
View Options

stand/libsa/tftp.c

Loading...
View Options

stand/libsa/ufs.c

Loading...
View Options

stand/loader.mk

Loading...