Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature]: Prevent user loading wrong config xml file in ConfigEditor #418

Open
kanechen66 opened this issue Nov 6, 2024 · 4 comments
Open
Labels
state:needs-maintainer-feedback Needs more information from a maintainer to determine next steps state:needs-triage Needs to triaged to determine next steps type:feature-request A new feature proposal urgency:low Little to no impact

Comments

@kanechen66
Copy link
Contributor

Feature Overview

In the current ConfigEditor design, there is no version check for xml.
It may lead to problems such as user loading wrong xml file so that the config is not taken effect or is not shown properly on UI.

Solution Overview

We have a proposal to add BIOS version and sha information in the xml file
ex:

  <bios_info version="c219b.0.bs.3a02.gn.1" sha="ecd618ada779bb6c699f1d94009047c70bd78d3a" />

Then the configeditor can read BIOS sha from smbios type 11 oemstring and bios version from smbios type 0

it can provide a hint that user is using the wrong xml and unexpected behavior issues could happen.

Feel free to provide your thoughts on this :)

Alternatives Considered

No response

Urgency

Low

Are you going to implement the feature request?

I will implement the feature

Do you need maintainer feedback?

Maintainer feedback requested

Anything else?

No response

@kanechen66 kanechen66 added state:needs-triage Needs to triaged to determine next steps type:feature-request A new feature proposal labels Nov 6, 2024
@github-actions github-actions bot added state:needs-maintainer-feedback Needs more information from a maintainer to determine next steps urgency:low Little to no impact labels Nov 6, 2024
@os-d
Copy link
Contributor

os-d commented Nov 6, 2024

@kanechen66 , this is a good idea, thank you for implementing.

@MarcChen46
Copy link
Contributor

Since the BIOS SHA is the git commit id, and not all BIOS will implement it in SMBIOS, and it is possible that multiple BIOS are using the same XML content, which should still be allowed, so I have different thought about how to check it.

  1. Calculate the sha256 of the config XML file in PreBuild phase of plugin during BIOS build time and set it to MU_SCHEMA_FILE_SHA256 in PlatformBuild.py
  2. During POST, BIOS publish the MU_SCHEMA_FILE_SHA256 value to a MuScemaFileSha256 NVRAM variable.
  3. When ConfigEditor open the XML file, it also calculate the SHA256 of the XML file, and compare with the MuScemaFileSha256 NVRAM variable, then we can know if we are using the exact the same XML file when build the BIOS.

@kanechen66
Copy link
Contributor Author

Hi all,
I have some other thoughts, and it extends Marc's idea.
To prevent the below cases.

  1. Additional space or line break added in the xml
  2. Someone uses Linux to edit the xml, the line break btwn Linux and windows is different.

I propose using all the elements (including tag) in xml to hash and BIOS save this hash to a variable.
When user opens ConfigEditor, it calc the hash and compare with the hash in BIOS var.
Again, this version check won't block user using the ConfigEditor. it will just show some warning in the status box.

let me know if it's ok, if it's ok, i will implement in this way.
thanks

@kanechen66
Copy link
Contributor Author

kanechen66 commented Nov 12, 2024

I created a PR here #421
Saving hash in BIOS can also prevent adding new node in xml.
Adding new node in xml could break the backward compatibility because it requires SetupDataPkg\Tools\configschema.xsd change
Let me know your thoughts on this. thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state:needs-maintainer-feedback Needs more information from a maintainer to determine next steps state:needs-triage Needs to triaged to determine next steps type:feature-request A new feature proposal urgency:low Little to no impact
Projects
None yet
Development

No branches or pull requests

3 participants