Parse Security Profile descriptors - #3528
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds parsing support for NBFT Security Profile descriptors in libnvme’s NBFT reader, exposing the descriptor’s flags/secret type and providing access to several heap-backed policy lists and the keypath URI.
Changes:
- Extends
struct libnbft_securityto include descriptor flags, secret type, and heap-backed policy list/keypath fields. - Implements raw heap object extraction (
__get_heap_obj_raw) for non-string heap objects (byte lists). - Implements
read_security()to populatelibnbft_securityentries from raw NBFT security descriptors.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| libnvme/src/nvme/nbft.h | Extends the public libnbft_security structure with parsed Security Profile fields. |
| libnvme/src/nvme/nbft.c | Adds raw heap object parsing helper and implements Security Profile descriptor parsing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| *output = NULL; | ||
| *length = le16_to_cpu(obj.length); | ||
| if (*length == 0) | ||
| return 0; | ||
| if (!in_heap(header, obj)) { | ||
| libnvme_msg(ctx, LIBNVME_LOG_DEBUG, | ||
| "file %s: field '%s' in descriptor '%s' has invalid offset or length\n", | ||
| filename, fieldname, descriptorname); | ||
| return -EINVAL; | ||
| } | ||
| *output = (unsigned char *)header + le32_to_cpu(obj.offset); | ||
| return 0; |
There was a problem hiding this comment.
Addressed in the revised parser commit. The helper clears the pointer and length before checking the object. It assigns the length only after the range check succeeds.
| /* TODO add fields */ | ||
| __u16 flags; | ||
| __u8 secret_type; | ||
| /* list fields point into the raw NBFT; not separately freed */ |
There was a problem hiding this comment.
Addressed in the revised parser commit. The public structure now states that every pointer refers to storage owned by the parent NBFT object. This includes the secret key URI.
c86bb84 to
47182bc
Compare
|
We found two crashes at commit The Security Profile list offset uses a 32-bit addition during its bounds check. A large offset can wrap to a small value. The production command then reads beyond the table and terminates with An NBFT table with the Control Descriptor valid bit cleared is accepted as a partly initialized object. The added Security Profile code later reads its null Host ID. This case also terminates with Each case terminated with I pushed a two-commit update on master at After the update, each malformed table completed three release-build runs without a signal. The same runs completed under AddressSanitizer with UndefinedBehaviorSanitizer. Once #3643 merges, the first commit can be removed during the rebase. |
| for (i = 0, cnt = 0; i < num_hfi; i++) { | ||
| if (read_hfi(ctx, nbft, &raw_hfi_array[i], | ||
| struct nbft_hfi *raw_hfi = | ||
| (struct nbft_hfi *)(raw_hfi_array + i * hfi_len); |
There was a problem hiding this comment.
I would suggest to introduce a helper for this calculation and re-use it everywhere.
There was a problem hiding this comment.
Done. All four descriptor readers now use one descriptor_at() helper to calculate each address from its recorded stride.
e67a3fa to
13a1950
Compare
|
The checkpatch warnings from run 30272756102 are fixed in 13a1950. Running the same command locally reports 0 errors and 0 warnings. The replacement GitHub run is waiting for maintainer approval. |
|
Let's give @tbzatek a chance to look at the changes. As far I can tell all looks good. |
Security Profile descriptors are skipped by the current reader. The associated subsystem records lose their security requirements. Parse fields enabled by descriptor flags and keep inactive fields clear. Add a valid parser fixture. Document pointer ownership. Signed-off-by: Flavien Solt <flavien@nus.edu.sg>
13a1950 to
bc35a2c
Compare
|
Yes. I rebased it onto current master in |
|
Thanks a lot! |
@tbzatek is not always around and @igaw is too fast... So what's the purpose of the new |
Summary
This change parses NBFT Security Profile descriptors that are currently skipped by
read_security().The parser records the descriptor flags and secret type. Policy list references are exposed when their corresponding flags permit them. The optional secret key URI is exposed as well.
Range validation
This branch includes the range validation change from #3643 in the same commit. That change prevents wrapped descriptor offsets from reaching memory beyond the table. It also rejects an invalid Control Descriptor before a partly initialized NBFT object reaches connection handling.
All four descriptor readers share one helper for address calculation. Raw object lengths remain zero until their range check passes. The public structure states that its pointers belong to the parent NBFT object.
Tests
A valid Security Profile table checks the parsed fields and the SSNS reference. Two malformed tables cover the wrapped Security Profile list and the invalid Control Descriptor.
The production command completed without a signal in three runs for each malformed table. The same checks passed under AddressSanitizer with UndefinedBehaviorSanitizer.
Checkpatch reports zero errors and zero warnings.
Related to linux-nvme/libnvme#1111 and linux-nvme/libnvme#1112.