Conversation
LCOV of commit
|
88a216e to
2ac8879
Compare
There was a problem hiding this comment.
Pull request overview
Adds a Linux-based example stack for libnat20, including kernel modules (device framework, crypto backend, software service) plus a userspace CLI and a Buildroot external tree to build/run the example (QEMU-focused).
Changes:
- Introduces kernel modules:
nat20device(char-dev framework),nat20crypto(Linux kernel crypto backend),nat20sw(software NAT20 service), andnat20lib(exports libnat20 symbols). - Adds
nat20cliuserspace tool + a shell script to exercise the kernel stack and verify cert chains. - Adds Buildroot
br_externalintegration (packages, configs, bootstrap/env scripts, QEMU runner) and expands license-check coverage to include these file types.
Reviewed changes
Copilot reviewed 32 out of 34 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| examples/linux/nat20sw/nat20sw.c | Software NAT20 service kernel module implementing dispatch + certificate read via nat20device. |
| examples/linux/nat20sw/Kbuild | Kbuild for building nat20sw against nat20 modules/symvers. |
| examples/linux/nat20lib/mod.c | Kernel module wrapper exporting selected libnat20 symbols for other modules. |
| examples/linux/nat20lib/Kbuild | Kbuild for compiling core libnat20 sources into a kernel module. |
| examples/linux/nat20device/nat20device.c | NAT20 device framework kernel module (char device + securityfs cert file). |
| examples/linux/nat20device/Kbuild | Kbuild for nat20device. |
| examples/linux/nat20device/include/nat20device.h | Public API for registering/unregistering NAT20 device drivers. |
| examples/linux/nat20crypto/nat20crypto.c | Linux-kernel-crypto-based implementation of nat20 crypto context. |
| examples/linux/nat20crypto/Kbuild | Kbuild for nat20crypto with include paths and symvers linkage. |
| examples/linux/nat20crypto/include/nat20crypto.h | Public header for opening/closing Linux crypto context + secret creation. |
| examples/linux/nat20cli/src/main.c | Userspace CLI to issue requests via /dev/nat20X and decode responses. |
| examples/linux/nat20cli/nat20clitest.sh | Script to load modules, run CLI flows, and verify produced cert chains. |
| examples/linux/nat20cli/CMakeLists.txt | CMake build for nat20cli against LibNat20 components. |
| examples/linux/br_external/utils/envsetup.sh | Helper functions/env exports for the Buildroot out-of-tree build dir. |
| examples/linux/br_external/run-qemu.sh | QEMU runner for the Buildroot-produced kernel + rootfs. |
| examples/linux/br_external/package/nat20sw/nat20sw.mk | Buildroot package definition for nat20sw kernel module. |
| examples/linux/br_external/package/nat20sw/Config.in | Buildroot menu entry for nat20sw. |
| examples/linux/br_external/package/nat20lib/nat20lib.mk | Buildroot package definition for nat20lib kernel module. |
| examples/linux/br_external/package/nat20lib/Config.in | Buildroot menu entry for nat20lib. |
| examples/linux/br_external/package/nat20device/nat20device.mk | Buildroot package definition for nat20device kernel module. |
| examples/linux/br_external/package/nat20device/Config.in | Buildroot menu entry + help text for nat20device. |
| examples/linux/br_external/package/nat20crypto/nat20crypto.mk | Buildroot package definition for nat20crypto kernel module. |
| examples/linux/br_external/package/nat20crypto/Config.in | Buildroot menu entry for nat20crypto. |
| examples/linux/br_external/package/nat20cli/nat20cli.mk | Buildroot package definition for installing nat20cli. |
| examples/linux/br_external/package/nat20cli/Config.in | Buildroot menu entry for nat20cli. |
| examples/linux/br_external/package/libnat20/libnat20.mk | Buildroot package definition for building/installing libnat20. |
| examples/linux/br_external/package/libnat20/Config.in | Buildroot menu entry for libnat20. |
| examples/linux/br_external/external.mk | Buildroot external-tree make include of all package .mks. |
| examples/linux/br_external/external.desc | Buildroot external-tree metadata. |
| examples/linux/br_external/Config.in | Top-level Buildroot external-tree menu sources for NAT20 packages. |
| examples/linux/br_external/bootstrap.sh | Bootstrap script to create out-of-tree Buildroot build directory and seed configs. |
| .github/license-check/license-config.json | Extends license header checking to additional Buildroot/script file types. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| void nat20device_unregister_driver(struct nat20device_driver* driver) { | ||
| struct nat20device_driver_instance* instance; | ||
|
|
||
| if (!driver) return; | ||
|
|
||
| instance = to_nat20device_instance(driver); | ||
|
|
||
| pr_info("NAT20: Unregistering driver instance %s%d\n", NAT20DEVICE_DEVICE_NAME, instance->id); | ||
|
|
There was a problem hiding this comment.
nat20device_unregister_driver() frees instance unconditionally, but open file descriptors keep a raw pointer to instance in file_priv->instance (set in nat20device_open). This can lead to use-after-free in subsequent read/write/release on those FDs after unregister. Consider reference counting the instance (e.g., kref) and only freeing after the last release, while making unregister mark the instance as inactive (e.g., set instance->ops = NULL) and remove device nodes/cdev.
| /* Dispatch the request */ | ||
| ret = instance->ops->dispatch(instance->ctx, request_buf, count, &file_priv->response); | ||
| if (ret < 0) goto out; | ||
|
|
There was a problem hiding this comment.
nat20device_write() replaces the response buffer but never resets the file position. If the previous response was fully read, *f_pos will remain at the old end offset and the next read() will immediately return 0 (EOF). Reset *f_pos (and/or filp->f_pos) to 0 when a new response is produced.
| /* | |
| * A new response buffer is now available for this file; rewind the | |
| * file position so the next read starts from the beginning. | |
| */ | |
| if (f_pos) *f_pos = 0; | |
| filp->f_pos = 0; |
| * The driver must allocate the response buffer, which will be freed by | ||
| * the framework using kfree after the read operation completes. |
There was a problem hiding this comment.
Header comment says the framework frees the response buffer "after the read operation completes", but the implementation only frees the buffer on the next write or on release() (it is not freed when read() reaches EOF). Please either update the documentation to match the actual lifetime or implement freeing when the response has been fully consumed.
| * The driver must allocate the response buffer, which will be freed by | |
| * the framework using kfree after the read operation completes. | |
| * The driver must allocate the response buffer and must not free it after | |
| * a successful return. The framework takes ownership of the buffer and | |
| * frees it with kfree during later request/file cleanup (for example, | |
| * before replacing it with a new response or when the device is released). |
| kmalloc(sizeof(struct shash_desc) + crypto_shash_descsize(md_tfm), GFP_KERNEL); | ||
| if (md_ctx == NULL) { | ||
| crypto_free_shash(md_tfm); | ||
| printk(KERN_ERR "Failed to allocate hash descriptor: %ld\n", PTR_ERR(md_ctx)); |
There was a problem hiding this comment.
On allocation failure md_ctx == NULL, the log uses PTR_ERR(md_ctx), but md_ctx is not an ERR_PTR (it's NULL). This prints a meaningless value and may confuse debugging. Log -ENOMEM or omit the error code here.
| printk(KERN_ERR "Failed to allocate hash descriptor: %ld\n", PTR_ERR(md_ctx)); | |
| printk(KERN_ERR "Failed to allocate hash descriptor: %d\n", -ENOMEM); |
| n20_rfc6979_k_generation( | ||
| &ctx->digest_ctx, digest_algorithm, dicemod_key->type, &key_slice, &gather_list, &k_bn, 0); |
There was a problem hiding this comment.
The return value from n20_rfc6979_k_generation() is ignored. If k-generation fails, subsequent operations (including modular inversion of k) may operate on an invalid/zero key and produce incorrect signatures or trigger errors. Capture and check the return value and fail early on error.
| n20_rfc6979_k_generation( | |
| &ctx->digest_ctx, digest_algorithm, dicemod_key->type, &key_slice, &gather_list, &k_bn, 0); | |
| err = n20_rfc6979_k_generation( | |
| &ctx->digest_ctx, digest_algorithm, dicemod_key->type, &key_slice, &gather_list, &k_bn, 0); | |
| if (err) { | |
| printk(KERN_ERR "Failed to generate signing nonce: %d\n", err); | |
| return err; | |
| } |
| exit(EXIT_FAILURE); | ||
| } | ||
|
|
||
| // Stage 1: Determine command |
There was a problem hiding this comment.
argv[optind] is used without checking that optind < argc. Invoking nat20cli with no command (or after consuming options) will read past argv bounds and likely crash. Add an argc/optind check and print usage when no command is provided.
| // Stage 1: Determine command | |
| // Stage 1: Determine command | |
| if (optind >= argc) { | |
| print_usage(argv[0]); | |
| exit(EXIT_FAILURE); | |
| } |
| n20_crypto_digest_context_t *digest_ctx = NULL; | ||
|
|
||
| n20_error_t n20_err = n20_crypto_nat20_open(&digest_ctx); | ||
| if (n20_err != n20_error_ok_e) { | ||
| fprintf( | ||
| stderr, "Failed to open digest context. libnat20 error: %d (0x%x)\n", n20_err, n20_err); | ||
| return -1; | ||
| } | ||
|
|
There was a problem hiding this comment.
n20_crypto_nat20_open(&digest_ctx) is called but digest_ctx is never used in main() and never closed. Either remove this initialization or ensure n20_crypto_nat20_close(digest_ctx) is called on all exit paths to avoid a resource leak.
| n20_crypto_digest_context_t *digest_ctx = NULL; | |
| n20_error_t n20_err = n20_crypto_nat20_open(&digest_ctx); | |
| if (n20_err != n20_error_ok_e) { | |
| fprintf( | |
| stderr, "Failed to open digest context. libnat20 error: %d (0x%x)\n", n20_err, n20_err); | |
| return -1; | |
| } |
| size_t pos = 0; | ||
| if ((len & 1) != 0) { | ||
| // Odd length, assume leading zero | ||
| *out_pos++ = nibble2bits(hex[0]); |
There was a problem hiding this comment.
For odd-length hex strings, hex_string_to_bytes_in_place() writes nibble2bits(hex[0]) without validating it. If the first character is not valid hex, the function will write 0xFF (from -1) and continue instead of failing. Check nibble2bits(hex[0]) < 0 and return an error like the even-length path does.
| *out_pos++ = nibble2bits(hex[0]); | |
| int8_t low = nibble2bits(hex[0]); | |
| if (low < 0) { | |
| return -1; // Invalid hex character | |
| } | |
| *out_pos++ = low; |
| # along with this program; if not, see | ||
| # <https://www.gnu.org/licenses/>. | ||
|
|
||
| LIBNAT20_VERSION = origin/main |
There was a problem hiding this comment.
Using *_VERSION = origin/main for a Buildroot git package makes builds non-reproducible and can unexpectedly change when the branch moves. Prefer pinning to a specific tag or commit hash (and updating intentionally), especially since this external tree lives inside the same repository.
| LIBNAT20_VERSION = origin/main | |
| # Pin to an immutable revision for reproducible Buildroot builds. | |
| LIBNAT20_VERSION = 0123456789abcdef0123456789abcdef01234567 |
| very useful for fleet management and establishing authenticity | ||
| and integrity to a remote relying party. It can also be | ||
| used for tracking, so protecting this feature from unauthorized | ||
| access is crutial for privacy on personal end user devices. |
There was a problem hiding this comment.
Typo in help text: "crutial" should be "crucial".
| access is crutial for privacy on personal end user devices. | |
| access is crucial for privacy on personal end user devices. |
bcbb5b5 to
be3d097
Compare
be3d097 to
a978a54
Compare
No description provided.