Multiple options= are parsed incorrectly making setting persistent options impossible

[Joao Andre Simioni]

[Impact]

When passing multiple options= flags into bootdev and bootparam commands, options= parsing is incorrect, only applying the final flag, and if there were any persistent boot options set, they get wiped out, leading to options not being persistent.

This makes it impossible to set a persistent efiboot flag for example.

The bug is a simple logic error, as seen in the final hunk of the patch:

@@ -1768,8 +2021,8 @@ ipmi_chassis_main(struct ipmi_intf * intf, int argc, char ** argv)
     }
     for (op = options; op->name; ++op) {
      if (strcmp(token, op->name) == 0) {
- flags[op->i] &= op->mask;
- flags[op->i] |= op->value;
+ flags[op->offset] &= ~(op->mask);
+ flags[op->offset] |= op->value;
       break;
      }
     }

flags[op->i] &= op->mask; was clearing the values of all flags, breaking persistence, while the fix is negate op->mask, to keep the other flags the same while clearing the value of the flag being set, as it is set as a bitwise OR on the next line as flags[op->offset] |= op->value;.

[Testcase]

You need a bare metal server that supports IPMI v2 or later.

If the server is set to boot from BIOS, you can attempt to efiboot it with:

$ sudo ipmitool chassis bootdev pxe options=persistent,efiboot
Set Boot Device to pxe
$ sudo ipmitool raw 0x00 0x09 0x05 0x0 0x0
 01 05 a0 04 00 00 00 <--- "a0", but expecting "e0"

This will boot in efi mode, but on the version in -updates, it will fail to be persistent. If you boot the server again, it will be a BIOS boot.

If you install the test package in the below ppa:

https://launchpad.net/~mruffell/+archive/ubuntu/sf359128-test

When you run the bootdev command, efiboot will be persistent, across multiple boots.

$ sudo ipmitool chassis bootdev pxe options=persistent,efiboot
Set Boot Device to pxe
swqa@luna-tdc5:~/jamien$ sudo ipmitool raw 0x00 0x09 0x05 0x0 0x0
 01 05 e0 04 00 00 00 <--- shows "e0" this time

[Where problems could occur]

We are changing how chassis bootdev and bootparam options are being parsed. If a regression were to occur, then options may be parsed differently to what the user was expecting, or had implemented in their scripts.

Due to flag persistence being fixed, flags are no longer clobbered out each time one of these commands are run, and options will remain persistent if they have been asked to be set as persistent. Any users relying on the faulty behaviour of clobbering instead of actually turning options off when they are finished may have undesired results.

If a regression were to occur, a user might have their systems options set incorrectly, and for example, swapping between BIOS and EFI boot, could lead to their server being unbootable and needing a system administrator to remote in over IPMI and set the correct boot options.

ipmitool is usually used during maintenance windows, but it could catch some users out.

[Other info]

Upstream bug report:
https://github.com/ipmitool/ipmitool/issues/163

Upstream pull request:
https://github.com/ipmitool/ipmitool/pull/165

Note the pull request has six commits. The first is the only one required to fix the issue. The other five are refactors and spelling corrections, and have very little change in logic. The other five do not backport cleanly at all, and some have fixups.

We will stick to including the first patch that resolves the problem, and nothing more:

commit 4b89f1b42d94a9bb96093d5e5237c7f1cfd84580
From: Alexander Amelkin <email address hidden>
Date: Wed, 6 Nov 2019 11:40:15 +0300
Subject: chassis: bootparam/bootdev: Refactor for less magic
Link: https://github.com/ipmitool/ipmitool/commit/4b89f1b42d94a9bb96093d5e5237c7f1cfd84580

[Athos Ribeiro (athos-ribeiro)]

Hi Joao,

Thanks for reporting this one and linking the upstream bug and fix.

Would you be willing to drive this fix as a potential SRU?

The first step would be to file the SRU paperwork in this bug:
https://wiki.ubuntu.com/StableReleaseUpdates#SRU_Bug_Template

Then, you could either provide a debdiff or an MP in LP so we can test/apply/upload it if it that is the proper fix for the issue.

In case you do not have the capacity to handle this ATM, I am adding this to the server team backlog to be analyzed and worked on by the server team as time allows.

[Matthew Ruffell (mruffell)]

Attached is a patch that fixes this issue on Kinetic

[Matthew Ruffell (mruffell)]

Attached is a patch that solves this issue on Jammy

[Matthew Ruffell (mruffell)]

Attached is a patch that fixes this issue on Focal

[Mauricio Faria de Oliveira (mfo)]

This is apparently waiting on testing and does not need sponsorship at the moment. Thanks!

[Heitor Alves de Siqueira (halves)]

Hi Matthew,

thanks for the debdiffs! I've reviewed them, and even though they seem fairly large most of the changes are just renames/defines with no functional difference. The core logic changes are fairly simple, and your backports look and test correctly.

I've also checked that the fixes were introduced with ipmitool version 1.8.19, so Ubuntu releases starting with lunar will already have them.

Sponsored for F/J/K. Thank you for the effort!

[Robie Basak (racb)]

SRU review

> even though they seem fairly large most of the changes are just renames/defines with no functional difference

Then they don't belong in the SRU. Please re-upload with the most minimal fix. If necessary, this can be your own patch with just the required fix. It doesn't have to be the same as what upstream did.

If more substantial changes are necessary, please justify them.

See SRU policy:

"... the requirements for stable updates are not necessarily the same as those in the development release. When preparing future releases, one of our goals is to construct the most elegant and maintainable system possible, and this often involves fundamental improvements to the system's architecture, rearranging packages to avoid bundled copies of other software so that we only have to maintain it in one place, and so on. However, once we have completed a release, the priority is normally to minimise risk caused by changes not explicitly required to fix qualifying bugs, and this tends to be well-correlated with minimising the size of those changes. As such, the same bug may need to be fixed in different ways in stable and development releases."

[Heitor Alves de Siqueira (halves)]

Thank you for the feedback, Robie! I'll sync up with Matthew and we'll rebase the fixes as requested.

[Matthew Ruffell (mruffell)]

Attached is a V2 that contains a heavily edited backport of the patch that contains a single hunk that fixes this specific issue. For focal.

[Matthew Ruffell (mruffell)]

Attached is a V2 that contains a heavily edited backport of the patch that contains a single hunk that fixes this specific issue. For Jammy.

[Matthew Ruffell (mruffell)]

Hi Heitor, the V2 debdiffs for Focal and Jammy are ready to be sponsored again.

[Heitor Alves de Siqueira (halves)]

Thanks for the revised debdiffs, Matthew! Sponsored for F/J.

[Timo Aaltonen (tjaalton)]

Hello Joao, or anyone else affected,

Accepted ipmitool into jammy-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/ipmitool/1.8.18-11ubuntu2.1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, what testing has been performed on the package and change the tag from verification-needed-jammy to verification-done-jammy. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-jammy. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

[Timo Aaltonen (tjaalton)]

Hello Joao, or anyone else affected,

Accepted ipmitool into focal-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/ipmitool/1.8.18-8ubuntu0.2 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, what testing has been performed on the package and change the tag from verification-needed-focal to verification-done-focal. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-focal. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

[Matthew Ruffell (mruffell)]

Performing verification for jammy

My user installed ipmitool 1.8.18-11ubuntu2 from jammy -release, and attempted
to set a persistent efiboot of their arm64 device:

$ apt policy ipmitool
ipmitool:
  Installed: 1.8.18-11ubuntu2
  Candidate: 1.8.18-11ubuntu2.1
  Version table:
     1.8.18-11ubuntu2.1 500
        500 http://ports.ubuntu.com/ubuntu-ports jammy-proposed/universe arm64 Packages
 *** 1.8.18-11ubuntu2 500
        500 http://ports.ubuntu.com/ubuntu-ports jammy/universe arm64 Packages
        100 /var/lib/dpkg/status
$ sudo ipmitool chassis bootdev pxe options=persistent,efiboot
Set Boot Device to pxe
$ sudo ipmitool raw 0x00 0x09 0x05 0x0 0x0
 01 05 a0 04 00 00 00 <--- "a0", but should be "e0"

The efiboot option was dropped incorrectly, showing "a0" in hexidecimal raw
view, versus the expected "e0" and efiboot being set.

My user then installed 1.8.18-11ubuntu2.1 from -proposed, and ran the same
command:

$ apt policy ipmitool
ipmitool:
  Installed: 1.8.18-11ubuntu2.1
  Candidate: 1.8.18-11ubuntu2.1
  Version table:
 *** 1.8.18-11ubuntu2.1 500
        500 http://ports.ubuntu.com/ubuntu-ports jammy-proposed/universe arm64 Packages
        100 /var/lib/dpkg/status
     1.8.18-11ubuntu2 500
        500 http://ports.ubuntu.com/ubuntu-ports jammy/universe arm64 Packages
$ sudo ipmitool chassis bootdev pxe options=persistent,efiboot
Set Boot Device to pxe
$ sudo ipmitool raw 0x00 0x09 0x05 0x0 0x0
 01 05 e0 04 00 00 00 <--- is "e0" now

Efiboot is now set in a persistent fashion, and the raw values show "e0".

The package in -proposed fixes the issue, happy to mark verified for jammy.

[Matthew Ruffell (mruffell)]

Performing verification for focal

My user installed ipmitool 1.8.18-8ubuntu0.1 from focal -updates, and attempted
to set a persistent efiboot of their arm64 device:

$ apt policy ipmitool
ipmitool:
  Installed: 1.8.18-8ubuntu0.1
  Candidate: 1.8.18-8ubuntu0.2
  Version table:
     1.8.18-8ubuntu0.2 500
        500 http://ports.ubuntu.com/ubuntu-ports focal-proposed/universe arm64 Packages
 *** 1.8.18-8ubuntu0.1 500
        500 http://ports.ubuntu.com/ubuntu-ports focal-updates/universe arm64 Packages
        100 /var/lib/dpkg/status
$ sudo ipmitool chassis bootdev pxe options=persistent,efiboot
Set Boot Device to pxe
$ sudo ipmitool raw 0x00 0x09 0x05 0x0 0x0
 01 05 a0 04 00 00 00 <--- "a0", but should be "e0"

The efiboot option was dropped incorrectly, showing "a0" in hexidecimal raw
view, versus the expected "e0" and efiboot being set.

My user then installed 1.8.18-8ubuntu0.2 from -proposed, and ran the same
command:

$ apt policy ipmitool
ipmitool:
  Installed: 1.8.18-8ubuntu0.2
  Candidate: 1.8.18-8ubuntu0.2
  Version table:
 *** 1.8.18-8ubuntu0.2 500
        500 http://ports.ubuntu.com/ubuntu-ports focal-proposed/universe arm64 Packages
        100 /var/lib/dpkg/status
     1.8.18-8ubuntu0.1 500
        500 http://ports.ubuntu.com/ubuntu-ports focal-updates/universe arm64 Packages
$ sudo ipmitool chassis bootdev pxe options=persistent,efiboot
Set Boot Device to pxe
$ sudo ipmitool raw 0x00 0x09 0x05 0x0 0x0
 01 05 e0 04 00 00 00 <--- is "e0" now

Efiboot is now set in a persistent fashion, and the raw values show "e0".

The package in -proposed fixes the issue, happy to mark verified for focal.

[Launchpad Janitor (janitor)]

This bug was fixed in the package ipmitool - 1.8.18-11ubuntu2.1

---------------
ipmitool (1.8.18-11ubuntu2.1) jammy; urgency=medium

  * Fix persistent boot options when multiple options are set. Previously
    flags were wiped out due to a logic error, leaving only the final flag
    set. This set of patches ensures that all flags are set correctly.
    (LP: #2017701)
    - d/p/lp2017701-chassis-bootparam-bootdev-Refactor-for-less-magic.patch

 -- Matthew Ruffell <email address hidden> Thu, 13 Jul 2023 15:16:17 +1200

[Robie Basak (racb)]

The verification of the Stable Release Update for ipmitool has completed successfully and the package is now being released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.

[Launchpad Janitor (janitor)]

This bug was fixed in the package ipmitool - 1.8.18-8ubuntu0.2

---------------
ipmitool (1.8.18-8ubuntu0.2) focal; urgency=medium

  * Fix persistent boot options when multiple options are set. Previously
    flags were wiped out due to a logic error, leaving only the final flag
    set. This set of patches ensures that all flags are set correctly.
    (LP: #2017701)
    - d/p/lp2017701-chassis-bootparam-bootdev-Refactor-for-less-magic.patch

 -- Matthew Ruffell <email address hidden> Thu, 13 Jul 2023 15:23:17 +1200