11
29
Fork
You've already forked ipmitool
33

Fru: Fix edit field not checking area existence #1

Merged
AlexanderAmelkin merged 14 commits from Lexden/ipmitool:master into master 2025年09月15日 16:57:49 +02:00
Contributor
Copy link

The current implementation of ipmitool fru edit does not perform proper
checks when attempting to resize the FRU. This results in undesireable
changes to the FRU in several instances:

  1. If the FRU is shrinking and a FRU area does not exist (offset 0),
    ipmitool may attempt to shift it forwards (decrementing the offset).
    This results in a wraparound to 0xFF, leaving an erroneous field offset.
  2. If the areas are not in the exact order given as an example in the
    FRU spec, ipmitool may shift the wrong fields, which would cause data
    loss. (the FRU spec does not specify a required order for FRU fields)
  3. If the FRU is being enlarged after a fru field edit, the FRU size is
    not properly modified before writing the FRU, so the end of the FRU
    becomes truncated, resulting in data loss.

This commit addresses these three issues by:

  1. Confirming that a area's does not have an offset of 0x00 before
    attempting to shift it.
  2. Ensuring that the area's offset is after the area that was modified
    before attempting to shift it.
  3. Properly edit the size of the FRU before the FRU is written.

Tested:
Shrinking a FRU was tested with and without the change:
New Header without change:
01 00 00 01 0a ff 00 f5
^^
Note that the Multi Record area now has an offset of 0xFF.

New Header with change:
01 00 00 01 0a 00 00 f4
^^
Note that the Multi Record area retains its offset of 0x00.

This change also includes printouts specifying what offsets are found
and when they are being shifted, as well as data being erased if the FRU
is being shrunk:
Offset: 0
Offset: 0
Offset: 8
Offset: 88 moving by -8 bytes.
Offset: 0
Erasing leftover data from 200 to 208

After shrinking the FRU, the FRU was reverted to its original state with
the fix in place:
01 00 00 01 0b 00 00 f3
^^
This resulted in only the product area offset being updated as expected.
Offset: 0
Offset: 0
Offset: 8
Offset: 80 moving by 8 bytes.
Offset: 0

The implementation of IPMI FRU write used in these tests errors out
without writing the FRU if a checksum fails to pass, so without this
fix, it was impossible to enlarge the FRU. This is because without the
change, the last 8 bytes of the FRU would be truncated which would
result in the checksum of the final FRU area being lost which would thus
trigger this FRU write failure.

Signed-off-by: Alex Schendel alex.schendel@intel.com

The current implementation of ipmitool fru edit does not perform proper checks when attempting to resize the FRU. This results in undesireable changes to the FRU in several instances: 1. If the FRU is shrinking and a FRU area does not exist (offset 0), ipmitool may attempt to shift it forwards (decrementing the offset). This results in a wraparound to 0xFF, leaving an erroneous field offset. 2. If the areas are not in the exact order given as an example in the FRU spec, ipmitool may shift the wrong fields, which would cause data loss. (the FRU spec does not specify a required order for FRU fields) 3. If the FRU is being enlarged after a fru field edit, the FRU size is not properly modified before writing the FRU, so the end of the FRU becomes truncated, resulting in data loss. This commit addresses these three issues by: 1. Confirming that a area's does not have an offset of 0x00 before attempting to shift it. 2. Ensuring that the area's offset is after the area that was modified before attempting to shift it. 3. Properly edit the size of the FRU before the FRU is written. Tested: Shrinking a FRU was tested with and without the change: New Header without change: 01 00 00 01 0a ff 00 f5 ^^ Note that the Multi Record area now has an offset of 0xFF. New Header with change: 01 00 00 01 0a 00 00 f4 ^^ Note that the Multi Record area retains its offset of 0x00. This change also includes printouts specifying what offsets are found and when they are being shifted, as well as data being erased if the FRU is being shrunk: Offset: 0 Offset: 0 Offset: 8 Offset: 88 moving by -8 bytes. Offset: 0 Erasing leftover data from 200 to 208 After shrinking the FRU, the FRU was reverted to its original state with the fix in place: 01 00 00 01 0b 00 00 f3 ^^ This resulted in only the product area offset being updated as expected. Offset: 0 Offset: 0 Offset: 8 Offset: 80 moving by 8 bytes. Offset: 0 The implementation of IPMI FRU write used in these tests errors out without writing the FRU if a checksum fails to pass, so without this fix, it was impossible to enlarge the FRU. This is because without the change, the last 8 bytes of the FRU would be truncated which would result in the checksum of the final FRU area being lost which would thus trigger this FRU write failure. Signed-off-by: Alex Schendel <alex.schendel@intel.com>
The current implementation of ipmitool fru edit does not perform proper
checks when attempting to resize the FRU. This results in undesireable
changes to the FRU in several instances:
1. If the FRU is shrinking and a FRU area does not exist (offset 0),
ipmitool may attempt to shift it forwards (decrementing the offset).
This results in a wraparound to 0xFF, leaving an erroneous field offset.
2. If the areas are not in the exact order given as an example in the
FRU spec, ipmitool may shift the wrong fields, which would cause data
loss. (the FRU spec does not specify a required order for FRU fields)
3. If the FRU is being enlarged after a fru field edit, the FRU size is
not properly modified before writing the FRU, so the end of the FRU
becomes truncated, resulting in data loss.
This commit addresses these three issues by:
1. Confirming that a area's does not have an offset of 0x00 before
attempting to shift it.
2. Ensuring that the area's offset is after the area that was modified
before attempting to shift it.
3. Properly edit the size of the FRU before the FRU is written.
Tested:
Shrinking a FRU was tested with and without the change:
New Header without change:
01 00 00 01 0a ff 00 f5
 ^^
Note that the Multi Record area now has an offset of 0xFF.
New Header with change:
01 00 00 01 0a 00 00 f4
 ^^
Note that the Multi Record area retains its offset of 0x00.
This change also includes printouts specifying what offsets are found
and when they are being shifted, as well as data being erased if the FRU
is being shrunk:
Offset: 0
Offset: 0
Offset: 8
Offset: 88 moving by -8 bytes.
Offset: 0
Erasing leftover data from 200 to 208
After shrinking the FRU, the FRU was reverted to its original state with
the fix in place:
01 00 00 01 0b 00 00 f3
 ^^
This resulted in only the product area offset being updated as expected.
Offset: 0
Offset: 0
Offset: 8
Offset: 80 moving by 8 bytes.
Offset: 0
The implementation of IPMI FRU write used in these tests errors out
without writing the FRU if a checksum fails to pass, so without this
fix, it was impossible to enlarge the FRU. This is because without the
change, the last 8 bytes of the FRU would be truncated which would
result in the checksum of the final FRU area being lost which would thus
trigger this FRU write failure.
Signed-off-by: Alex Schendel <alex.schendel@intel.com>
lib/ipmi_fru.c Outdated
@ -5229,0 +5224,4 @@
/* Check Chassis, Board, Product, and Multirecord Area offsets to see if they need
* to be moved.
*/
for (int i = 0; i < 5; i++)
First-time contributor
Copy link

where does 5 come from? consider making that a #define value

where does 5 come from? consider making that a #define value
Author
Contributor
Copy link

The length of the header offset struct is 5 (for the five areas of the FRU). I have added a #define for the FRU_AREA_COUNT and replaced the literal in both the array declaration and the for loop.

The length of the header offset struct is 5 (for the five areas of the FRU). I have added a #define for the FRU_AREA_COUNT and replaced the literal in both the array declaration and the for loop.
Lexden marked this conversation as resolved
lib/ipmi_fru.c Outdated
@ -5237,3 +5229,1 @@
product_len
);
header.offset.product += change_size_by_8;
#ifdef DBG_RESIZE_FRU
First-time contributor
Copy link

It's customary to align #ifdef/#endif with column 0. Here and other instances below.

It's customary to align #ifdef/#endif with column 0. Here and other instances below.
Author
Contributor
Copy link

I see, thank you for letting me know. I saw mixed usage in the existing code, with several at column 0 and several at the current indentation. I will stick with the former from now on.

I see, thank you for letting me know. I saw mixed usage in the existing code, with several at column 0 and several at the current indentation. I will stick with the former from now on.
Lexden marked this conversation as resolved
lib/ipmi_fru.c Outdated
@ -5240,0 +5242,4 @@
/* Internal Use Area length will be calculated by finding the closest area
* following it.
*/
if (internal_move && header.offsets[i] > header.offset.internal && header.offsets[i] < nearest_area)
First-time contributor
Copy link

wrap long lines to make it easier to read.

wrap long lines to make it easier to read.
Lexden marked this conversation as resolved 
Replaced literal int with macro and fixed formatting errors.
Signed-off-by: Schendel <alex.schendel@intel.com>
vmauery approved these changes 2023年06月16日 01:51:42 +02:00
Dismissed
vmauery left a comment
Copy link

LGTM.

LGTM.
fru edit command can increase fru size. Allocate new fru data with
the bigger size. This fixes ipmitool crash when fru edit increases fru
size.
Tested:
fru edit command that increases fru size complete successfully.
Signed-off-by: Zhikui Ren <zhikui.ren@intel.com>
Reviewed-on: Lexden/ipmitool#1
@ -46,6 +46,7 @@
#define GET_FRU_INFO 0x10
#define GET_FRU_DATA 0x11
#define SET_FRU_DATA 0x12
#define FRU_AREA_COUNT 5

There is FRU_AREAS_COUNT in the latest ipmi_fru.h since c27480c96e, please update and remove this definition.

There is FRU_AREAS_COUNT in the latest `ipmi_fru.h` since c27480c96eed0591ec325449f8b23013bd221c4f, please update and remove this definition.
Lexden marked this conversation as resolved
lib/ipmi_fru.c Outdated
@ -5006,0 +5006,4 @@
if (change > 0)
{
/* Fru area is padded to be 8 bytes aligned */
new_fru_size = fru.size + change + FRU_BLOCK_SZ;

This is a wrong formula. It should be (fru.size + change + FRU_BLOCK_SZ - 1) / FRU_BLOCK_SZ * FRU_BLOCK_SZ.

This is a wrong formula. It should be `(fru.size + change + FRU_BLOCK_SZ - 1) / FRU_BLOCK_SZ * FRU_BLOCK_SZ`.
Author
Contributor
Copy link

I do not know how I made that large of an error in my logic.
The idea here would be to get the size of the new FRU area, add the size of an extra, empty FRU block to ensure we do not trim the area, subtract 1 to ensure we round down and then divide by the FRU block size to get the number of FRU blocks needed to contain the whole FRU area, and then multiply by the FRU block size again to get the total size as a round number of FRU blocks, right?

I do not know how I made that large of an error in my logic. The idea here would be to get the size of the new FRU area, add the size of an extra, empty FRU block to ensure we do not trim the area, subtract 1 to ensure we round down and then divide by the FRU block size to get the number of FRU blocks needed to contain the whole FRU area, and then multiply by the FRU block size again to get the total size as a round number of FRU blocks, right?

Exactly.

Exactly.
Lexden marked this conversation as resolved
lib/ipmi_fru.c Outdated
@ -5065,7 +5074,7 @@ ipmi_fru_set_field_string_rebuild(struct ipmi_intf * intf, uint8_t fruId,
fru_data_old = calloc( fru.size, sizeof(uint8_t) );

I'd remove this empty string

I'd remove this empty string
Lexden marked this conversation as resolved
lib/ipmi_fru.c Outdated
@ -5214,1 +5208,3 @@
if (f_type == 'c' )
/* IPMI FRU Spec does not specify the order of areas in the FRU.
* Therefore, we must check each section's current offset in order to determine
* which areas much be adjusted.

typo. should be "must be".

typo. should be "must be".
Lexden marked this conversation as resolved
lib/ipmi_fru.c Outdated
@ -5215,0 +5220,4 @@
*/
/* The Internal Use Area does not require the area length be provided, so we must
* work to calculate the length.

You forget here that Multirecord area also doesn't have the common header or the total area length field and must be recalculated record by record. Otherwise you just lose the MR area.

You forget here that Multirecord area also doesn't have the common header or the total area length field and must be recalculated record by record. Otherwise you just lose the MR area.
Author
Contributor
Copy link

I'm not sure I understand this comment.
FRU Spec v1.0 rev 1.3 Section 8 specifies that the Common Header provides the MultiRecord Area Starting Offset as the sixth byte.
Section 16.1 specifies that the MultiRecord Area header contains the Record Length as the third byte.

I'm not sure I understand this comment. FRU Spec v1.0 rev 1.3 Section 8 specifies that the Common Header provides the MultiRecord Area Starting Offset as the sixth byte. Section 16.1 specifies that the MultiRecord Area header contains the Record Length as the third byte.
Author
Contributor
Copy link

I see my misunderstanding now, thank you. I have updated this.

I see my misunderstanding now, thank you. I have updated this.
Lexden marked this conversation as resolved
lib/ipmi_fru.c Outdated
@ -5240,0 +5236,4 @@
{
#ifdef DBG_RESIZE_FRU
printf("\n");
#endif

Let's not have those #ifdefs in master branch.
If you feel like some debugging may be needed, then let's make it possible to get it in runtime.
Use lprintf(LOG_DEBUG + 3, ..., and let's change the message to something more verbose like "Area %i original offset: %i"

Let's not have those `#ifdef`s in master branch. If you feel like some debugging may be needed, then let's make it possible to get it in runtime. Use `lprintf(LOG_DEBUG + 3, ...`, and let's change the message to something more verbose like `"Area %i original offset: %i"`
Author
Contributor
Copy link

I see, I can remove most of these and refactor the rest. However, I was following the existing code for debugging:

Line 5084 in ab5ce5b
#ifdef DBG_RESIZE_FRU


There already was a #ifdef DBG_RESIZE_FRU in the code upstream from at least 12 years ago, so I was trying to follow the existing code. From my count, there is the #define and seven subsequent #ifdef s in the code currently.

I see, I can remove most of these and refactor the rest. However, I was following the existing code for debugging: https://codeberg.org/IPMITool/ipmitool/src/commit/ab5ce5baff097ebb6e2a17a171858be213ee68d3/lib/ipmi_fru.c#L5084 There already was a #ifdef DBG_RESIZE_FRU in the code upstream from at least 12 years ago, so I was trying to follow the existing code. From my count, there is the #define and seven subsequent #ifdef s in the code currently.
lib/ipmi_fru.c Outdated
@ -5240,0 +5259,4 @@
if (last_area < header.offsets[i])
{
last_area = header.offsets[i];
end_of_fru = (header.offsets[i] + *(fru_data_old + (header.offsets[i] * 8) + 1)) * 8;

Please, let's not have magic numbers. Use FRU_BLOCK_SZ.
Besides, the formula looks wrong to me. It seems you're trying to point to the area length field by adding that 1, but the area_len field is at offset 2 and is uint16_t, so you'd need a cast.

Don't hesitate to introduce new local variables to make the formula easier to comprehend.
Also please avoid direct pointer arithmetics in favor of structures and type punning.
With ed6baeb4c3 I've added struct fru_area_hdr_s, so
you can use that:

last_area = header.offsets[i];
off_t area_offset = header.offsets[i] * FRU_BLOCK_SZ;
struct fru_area_hdr_s *area_hdr = (struct fru_area_hdr_s *)(fru_data_old + area_offset);
end_of_fru = last_area + area_hdr->area_len;
Please, let's not have magic numbers. Use `FRU_BLOCK_SZ`. Besides, the formula looks wrong to me. It seems you're trying to point to the area length field by adding that `1`, but the `area_len` field is at offset `2` and is `uint16_t`, so you'd need a cast. Don't hesitate to introduce new local variables to make the formula easier to comprehend. Also please avoid direct pointer arithmetics in favor of structures and type punning. With ed6baeb4c3af1eae686d984a242072383754e30b I've added `struct fru_area_hdr_s`, so you can use that: ```c last_area = header.offsets[i]; off_t area_offset = header.offsets[i] * FRU_BLOCK_SZ; struct fru_area_hdr_s *area_hdr = (struct fru_area_hdr_s *)(fru_data_old + area_offset); end_of_fru = last_area + area_hdr->area_len; ```
Author
Contributor
Copy link

Would this not only work for the Chassis, Board, and Product Info Areas? Also, I was wondering about this. Unless I am completely misunderstanding the FRU spec, I see it specifying the Length as the second byte (1 byte offset) and as 1 byte for every FRU area. The only exception is the MR area which has it as the third byte.

field length field
1 Product Area Format Version
 7:4 - reserved, write as 0000b
 3:0 - format version number = 1h for this specification
1 Product Area Length (in multiples of 8 bytes)
Would this not only work for the Chassis, Board, and Product Info Areas? Also, I was wondering about this. Unless I am completely misunderstanding the FRU spec, I see it specifying the Length as the second byte (1 byte offset) and as 1 byte for every FRU area. The only exception is the MR area which has it as the third byte. ``` field length field 1 Product Area Format Version 7:4 - reserved, write as 0000b 3:0 - format version number = 1h for this specification 1 Product Area Length (in multiples of 8 bytes) ```
Contributor
Copy link

The Multi-record byte 3 is the length of the current record. You have to check bit 7 of byte 2 to see if this is the last record. If it isn't you will find an appended multi-record entry. The multi-record is effectively a linked list of MR's.

The Multi-record byte 3 is the length of the _current_ record. You have to check bit 7 of byte 2 to see if this is the _last_ record. If it isn't you will find an appended multi-record entry. The multi-record is effectively a linked list of MR's.
Author
Contributor
Copy link

I see, I misunderstood how the MRA was formatted, I have looked back through the spec and I see what you mean. I will correct that part.

I see, I misunderstood how the MRA was formatted, I have looked back through the spec and I see what you mean. I will correct that part.
lib/ipmi_fru.c Outdated
@ -5245,0 +5295,4 @@
uint32_t length = nearest_area - header.offset.internal;
memcpy(
(fru_data_new + ((header.offset.internal + change_size_by_8) * 8)),
(fru_data_old + (header.offset.internal) * 8),

Please here as well use some intermediate variables for better readability and shorter lines.

Please here as well use some intermediate variables for better readability and shorter lines.
Lexden marked this conversation as resolved 
This commit removes magic numbers and references to magic numbers.
This commit also refactors several arithmetic calculations to make them
more readable.
This commit also fixes a logical error in one of these arithmetic
calculations.
Signed-off-by: Alex Schendel <alex.schendel@intel.com>
Lexden dismissed vmauery's review 2023年10月30日 23:51:13 +01:00
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

Previous commit dropped several changes unexpectedly. This commit
attempts to add them back in.
Signed-off-by: Alex Schendel <alex.schendel@intel.com>
This commit fixes the multi-record area length calculation to traverse
the list.
This commit also refactors the memcpy offsets to properly define them as
a uint8_t pointer.
Signed-off-by: Alex Schendel <alex.schendel@intel.com>
lib/ipmi_fru.c Outdated
@ -5004,2 +5001,2 @@
ipmi_fru_set_field_string_rebuild(intf,fruId,fru,header,f_type,f_index,f_string)
)
}
else {
First-time contributor
Copy link

nit: indentation

nit: indentation
Author
Contributor
Copy link

I'm not sure I see where there is indentation inconsistency... The else is indented to align with the if statement it follows.

I'm not sure I see where there is indentation inconsistency... The else is indented to align with the if statement it follows.
First-time contributor
Copy link

Maybe I missed something. I am simply comparing the old line 5000 and new line 5001 and assumed they should be indented the same

Maybe I missed something. I am simply comparing the old line 5000 and new line 5001 and assumed they should be indented the same

Yep, I'm also wondering why the original TAB got converted to 4 spaces here.
I don't see any other changes in this hunk.

Yep, I'm also wondering why the original TAB got converted to 4 spaces here. I don't see any other changes in this hunk.
lib/ipmi_fru.c Outdated
@ -5006,0 +5007,4 @@
{
/* Fru area is padded to be 8 bytes aligned */
int new_raw_size = fru.size + change;
int new_raw_padded = new_raw_size + FRU_BLOCK_SZ - 1;
First-time contributor
Copy link

nit: new_raw_padded->new_padded__max

nit: new_raw_padded->new_padded__max
Lexden marked this conversation as resolved
First-time contributor
Copy link

Can we add some of the log messages back that can be enabled easily when debugging issues.

Can we add some of the log messages back that can be enabled easily when debugging issues.
Previously, logging was removed because it had used ifdefs. This commit
adds logging back in using lprintf, refactored to account for the fact
that lprintf automatically adds a newline on print.
Signed-off-by: Alex Schendel <alex.schendel@intel.com>
This commit refactors the intermediate variable "new_raw_padded" to
"new_padded_max" for more clarity.
Signed-off-by: Alex Schendel <alex.schendel@intel.com>
lib/ipmi_fru.c Outdated
@ -5293,1 +5333,3 @@
);
end_of_fru += change_size_by_8 * 8;
int length_of_erase = change_size_by_8 * -1 * 8;
lprintf(LOG_DEBUG, "Erasing leftover data from %i to %i\n", end_of_fru, end_of_fru + length_of_erase);
First-time contributor
Copy link

use FRU_BLOCK_SZ instead of 8, change_size_by_8 can change_block_num or change_blocks or change_block_cnt

use FRU_BLOCK_SZ instead of 8, change_size_by_8 can change_block_num or change_blocks or change_block_cnt
Lexden marked this conversation as resolved
Author
Contributor
Copy link

I made a mistake which duplicated 18 commits. I've force pushed to clean that out.

I made a mistake which duplicated 18 commits. I've force pushed to clean that out.
AlexanderAmelkin approved these changes 2025年09月11日 16:28:06 +02:00
Dismissed
AlexanderAmelkin dismissed AlexanderAmelkin's review 2025年09月15日 16:56:24 +02:00
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

Made some mistakes when merging. Had to refix, rebase, squash, refactor and force-push. Now this is in 6d0a28f1c3

Made some mistakes when merging. Had to refix, rebase, squash, refactor and force-push. Now this is in 6d0a28f1c395f59bcd713c6efccaa3ea17a684b1
Sign in to join this conversation.
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
5 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
IPMITool/ipmitool!1
Reference in a new issue
IPMITool/ipmitool
No description provided.
Delete branch "Lexden/ipmitool:master"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?