Skip to content

Secureboot: PDDF: Do not always use -f to load custom PDDF modules#23732

Merged
lguohan merged 1 commit intosonic-net:masterfrom
bhouse-nexthop:secureboot-pddf-modules
Oct 18, 2025
Merged

Secureboot: PDDF: Do not always use -f to load custom PDDF modules#23732
lguohan merged 1 commit intosonic-net:masterfrom
bhouse-nexthop:secureboot-pddf-modules

Conversation

@bhouse-nexthop
Copy link
Copy Markdown
Collaborator

@bhouse-nexthop bhouse-nexthop commented Aug 17, 2025

Why I did it

When in kernel modules are signed for secureboot, passing -f to modprobe will fail with a module signing issue even though the module is signed properly. This appears to be due to some internal module modification to handle mismatched kernel versions which invalidates the signature.

Work item tracking
  • Microsoft ADO (number only):

How I did it

This modification first tries to load custom modules without the -f flag, then if (and only if) that fails, try again with the -f flag. No attempt is made to determine if the kernel module is signed as it was not necessary and would only complicate the code further.

It is not clear what the valid use-case is for using modprobe -f, however this behavior has been maintained in this modification.

How to verify it

Build a SONiC secure-boot enabled build and observe custom modules (like PDDF modules) load proper now, and previously failed without this patch.

Which release branch to backport (provide reason below if selected)

  • 202205
  • 202211
  • 202305
  • 202311
  • 202405
  • 202411
  • 202505

Tested branch (Please provide the tested image version)

master as of 20250817

Description for the changelog

Secureboot: PDDF: Do not always use -f to load custom PDDF modules

Link to config_db schema for YANG module changes

A picture of a cute animal (not mandatory but encouraged)

Fixes: #23406
Signed-off-by: Brad House bhouse@nexthop.ai

@bhouse-nexthop
TO BE UPSTREAMED: PDDF: Do not always use -f to load custom PDDF modules
e34703c 
When in secureboot, -f will fail with a module signing issue even
though the module is signed properly.  This implies it is doing
some module modification to force it to be loaded which invalidates
the signing.

First try to load custom modules without the -f flag, then on failure
use the -f flag to maintain compatibility with the existing
implementation.
@mssonicbld
Copy link
Copy Markdown
Collaborator

mssonicbld commented Aug 17, 2025

/azp run Azure.sonic-buildimage

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Aug 17, 2025

Azure Pipelines successfully started running 1 pipeline(s).

@bhouse-nexthop bhouse-nexthop mentioned this pull request Jul 30, 2025
Closed
@bhouse-nexthop
Copy link
Copy Markdown
Collaborator Author

bhouse-nexthop commented Aug 17, 2025

@DavidZagury @davidpil2002 @qiluo-msft please review

@bhouse-nexthop bhouse-nexthop mentioned this pull request Aug 17, 2025
Merged
@DavidZagury DavidZagury requested a review from Yarden-Z September 9, 2025 08:49
@bhouse-nexthop
Copy link
Copy Markdown
Collaborator Author

bhouse-nexthop commented Oct 16, 2025

@Yarden-Z any comments?

@lguohan lguohan closed this Oct 18, 2025
@lguohan lguohan reopened this Oct 18, 2025
@mssonicbld
Copy link
Copy Markdown
Collaborator

mssonicbld commented Oct 18, 2025

/azp run Azure.sonic-buildimage

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Oct 18, 2025

Azure Pipelines successfully started running 1 pipeline(s).

@lguohan lguohan merged commit 90c20d2 into sonic-net:master Oct 18, 2025
9 of 15 checks passed
@lguohan
Copy link
Copy Markdown
Collaborator

lguohan commented Oct 18, 2025

Here’s a review of the changes in PR #23732 for sonic-net/sonic-buildimage:

Summary

This PR refactors the handling of kernel object modules (KOS) in platform/pddf/i2c/utils/pddf_util.py. It separates kernel modules into three lists: perm_kos, std_kos, and custom_kos, and adjusts how modules are loaded and unloaded during driver install/uninstall.

Key Changes

1. Variable Refactoring

  • Before:
    A single list, kos, was used for all kernel modules, including permanent, standard, and custom modules.
  • After:
    Introduces three lists:
    • perm_kos: For permanent kernel modules.
    • std_kos: For standard kernel modules.
    • custom_kos: For custom kernel modules.

2. Module Loading Logic

  • Before:
    All modules in kos were loaded using modprobe or modprobe -f for custom modules.
  • After:
    • Loads perm_kos and std_kos with modprobe.
    • Loads custom_kos with modprobe, and if that fails, retries with modprobe -f.
    • Skips force-loading signed modules on the first try.

3. Module Unloading Logic

  • Before:
    Unloads all modules in kos (except for those in perm_kos or named i2c-i801) using string replacements.
  • After:
    • Unloads std_kos and custom_kos (not perm_kos).
    • Skips only modules containing i2c-i801.
    • Uses explicit "modprobe -rq" command for unloading.

Pros

  • Improved Clarity:
    Separating module categories makes the code easier to understand and maintain.
  • Safer Module Loading:
    The logic to retry loading custom modules with -f only after a standard attempt is cleaner and safer.
  • Simplified Unloading:
    The new approach is more explicit and less prone to string substitution errors.
  • Better Extensibility:
    Adding or modifying module handling logic in the future will be easier.

Cons / Suggestions

  • Backward Compatibility:
    If external scripts or tools depend on the old kos list structure, there could be integration issues. Review all callers.
  • Variable Initialization:
    Ensure all new lists (std_kos, custom_kos) are always initialized as lists, even if empty, to avoid NoneType issues.
  • Logging:
    Consider more detailed logging, especially when a module fails to load or unload.

Overall Assessment

This is a solid refactor that improves the clarity and reliability of kernel module handling in the PDDF utilities.
The code is easier to follow, less error-prone, and provides a better framework for future enhancements. Just ensure that all references to the old kos variable are updated and that empty list cases are handled.

Recommendation:
Looks good to merge after confirming backward compatibility and list initialization.

lguohan pushed a commit that referenced this pull request Oct 27, 2025
@bhouse-nexthop
Secureboot: Production secureboot signing enhancements (ephemeral key…
2b2004a 
…s and bugfixes) (#23405)

#### Why I did it

The current sonic secureboot implementation assumes all assets are signed with the DB Key.  There are roughly 3600 kernel modules that get signed in the process.

Organizations may have varying security policy requirements where any signing requests may require signoff by one or more parties, this may simply be infeasible for that many requests.

Given that the DB key may be a long-lived key, and only RSA 2048 is often used or available, it is a security best-practice to sign as few assets as possible with such a key.

This PR is fully backwards compatible with any pre-existing usages.  It introduces a new `rules/config` setting of `SECURE_UPGRADE_KERNEL_CAFILE` which is a path to a file for all trusted keys to embed into the kernel.  If that setting is not specified, it defaults to `SECURE_UPGRADE_SIGNING_CERT`.  There are no other infrastructure changes to support ephemeral keys for kernel module signing.  It is up to the organization to generate the ephemeral keys and pass them into their own signing scripts as required.

Dependent PR:
 * sonic-net/sonic-linux-kernel#499

Related PRs:
 * sonic-net/sonic-utilities#3989
 * #23732
 * #23733
 * #23734
 * #23735
 * #23736

Fixes #23406

##### Work item tracking

#### How I did it

Worked through production signing steps and hit issues when requiring signing approval with our HSM.  This was the most elegant solution that required the minimal changes.

This has been fully tested.

#### How to verify it

Since there are no provided production signing script examples this would require a vendor with an existing implementation to validate.  Nexthop has validated this internally.

#### Which release branch to backport (provide reason below if selected)


#### Tested branch (Please provide the tested image version)

master as of 20250721

#### Description for the changelog
Secureboot: Production signing enhancement to allow ephemeral kernel module keys

#### Link to config_db schema for YANG module changes
N/A

#### A picture of a cute animal (not mandatory but encouraged)

Signed-off-by: Brad House <bhouse@nexthop.ai>
FengPan-Frank pushed a commit to FengPan-Frank/sonic-buildimage that referenced this pull request Dec 4, 2025
@bhouse-nexthop @FengPan-Frank
PDDF: Do not always use -f to load custom PDDF modules (sonic-net#23732)
44b7c5c 
When in secureboot, -f will fail with a module signing issue even
though the module is signed properly.  This implies it is doing
some module modification to force it to be loaded which invalidates
the signing.

First try to load custom modules without the -f flag, then on failure
use the -f flag to maintain compatibility with the existing
implementation.

Signed-off-by: Feng Pan <fenpan@microsoft.com>
FengPan-Frank pushed a commit to FengPan-Frank/sonic-buildimage that referenced this pull request Dec 4, 2025
@bhouse-nexthop @FengPan-Frank
Secureboot: Production secureboot signing enhancements (ephemeral key…
c060ed4 
…s and bugfixes) (sonic-net#23405)

#### Why I did it

The current sonic secureboot implementation assumes all assets are signed with the DB Key.  There are roughly 3600 kernel modules that get signed in the process.

Organizations may have varying security policy requirements where any signing requests may require signoff by one or more parties, this may simply be infeasible for that many requests.

Given that the DB key may be a long-lived key, and only RSA 2048 is often used or available, it is a security best-practice to sign as few assets as possible with such a key.

This PR is fully backwards compatible with any pre-existing usages.  It introduces a new `rules/config` setting of `SECURE_UPGRADE_KERNEL_CAFILE` which is a path to a file for all trusted keys to embed into the kernel.  If that setting is not specified, it defaults to `SECURE_UPGRADE_SIGNING_CERT`.  There are no other infrastructure changes to support ephemeral keys for kernel module signing.  It is up to the organization to generate the ephemeral keys and pass them into their own signing scripts as required.

Dependent PR:
 * sonic-net/sonic-linux-kernel#499

Related PRs:
 * sonic-net/sonic-utilities#3989
 * sonic-net#23732
 * sonic-net#23733
 * sonic-net#23734
 * sonic-net#23735
 * sonic-net#23736

Fixes sonic-net#23406

##### Work item tracking

#### How I did it

Worked through production signing steps and hit issues when requiring signing approval with our HSM.  This was the most elegant solution that required the minimal changes.

This has been fully tested.

#### How to verify it

Since there are no provided production signing script examples this would require a vendor with an existing implementation to validate.  Nexthop has validated this internally.

#### Which release branch to backport (provide reason below if selected)

#### Tested branch (Please provide the tested image version)

master as of 20250721

#### Description for the changelog
Secureboot: Production signing enhancement to allow ephemeral kernel module keys

#### Link to config_db schema for YANG module changes
N/A

#### A picture of a cute animal (not mandatory but encouraged)

Signed-off-by: Brad House <bhouse@nexthop.ai>
Signed-off-by: Feng Pan <fenpan@microsoft.com>
xwjiang-ms pushed a commit to xwjiang-ms/sonic-buildimage that referenced this pull request Dec 22, 2025
@bhouse-nexthop @xwjiang-ms
Secureboot: Production secureboot signing enhancements (ephemeral key…
1a1f2a0 
…s and bugfixes) (sonic-net#23405)

#### Why I did it

The current sonic secureboot implementation assumes all assets are signed with the DB Key.  There are roughly 3600 kernel modules that get signed in the process.

Organizations may have varying security policy requirements where any signing requests may require signoff by one or more parties, this may simply be infeasible for that many requests.

Given that the DB key may be a long-lived key, and only RSA 2048 is often used or available, it is a security best-practice to sign as few assets as possible with such a key.

This PR is fully backwards compatible with any pre-existing usages.  It introduces a new `rules/config` setting of `SECURE_UPGRADE_KERNEL_CAFILE` which is a path to a file for all trusted keys to embed into the kernel.  If that setting is not specified, it defaults to `SECURE_UPGRADE_SIGNING_CERT`.  There are no other infrastructure changes to support ephemeral keys for kernel module signing.  It is up to the organization to generate the ephemeral keys and pass them into their own signing scripts as required.

Dependent PR:
 * sonic-net/sonic-linux-kernel#499

Related PRs:
 * sonic-net/sonic-utilities#3989
 * sonic-net#23732
 * sonic-net#23733
 * sonic-net#23734
 * sonic-net#23735
 * sonic-net#23736

Fixes sonic-net#23406

##### Work item tracking

#### How I did it

Worked through production signing steps and hit issues when requiring signing approval with our HSM.  This was the most elegant solution that required the minimal changes.

This has been fully tested.

#### How to verify it

Since there are no provided production signing script examples this would require a vendor with an existing implementation to validate.  Nexthop has validated this internally.

#### Which release branch to backport (provide reason below if selected)

#### Tested branch (Please provide the tested image version)

master as of 20250721

#### Description for the changelog
Secureboot: Production signing enhancement to allow ephemeral kernel module keys

#### Link to config_db schema for YANG module changes
N/A

#### A picture of a cute animal (not mandatory but encouraged)

Signed-off-by: Brad House <bhouse@nexthop.ai>
Signed-off-by: xiaweijiang <xiaweijiang@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lguohan lguohan Awaiting requested review from lguohan lguohan is a code owner

@Yarden-Z Yarden-Z Awaiting requested review from Yarden-Z

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Enhancement: Secureboot production signing of kernel modules using ephemeral keys + bug fixes

3 participants

@bhouse-nexthop @mssonicbld @lguohan