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

Require to disable Secure Boot #313

Closed
wants to merge 1 commit into from

Conversation

Jedoku
Copy link
Contributor

@Jedoku Jedoku commented Feb 27, 2024

No description provided.

Copy link
Contributor

@akihikodaki akihikodaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Secure boot shouldn't be enabled if the driver is not properly signed.
Otherwise, you should be able to keep secure boot enabled for all tests and there should be no need to switch secure and insecure mode.

@kostyanf14
Copy link
Contributor

kostyanf14 commented Feb 27, 2024

Secure boot shouldn't be enabled if the driver is not properly signed.

We don't know what driver was provided, so let's expect that all drivers are signed in this case.

Currently, in the config, we have only SVVP-related tests and to pass SVVP all drivers must be signed by MS, but these 3 tests required to boot the VM with SB enabled and proper key are in PEK, DBX.

Otherwise, you should be able to keep secure boot enabled for all tests and there should be no need to switch secure and insecure mode.

NO. There are a lot of drivers/apps that are unsigned and used by HLK (also during SVVP). Microsoft even provides a list of tests where you MUST disable SB. We have 2 scenarios of testing driver and SVVP. Currently, we don't have any driver tests that require SB enabled, and our use case is test driver after build (test-signed), so we can't enable SB by default for everything.

@akihikodaki
Copy link
Contributor

Secure boot shouldn't be enabled if the driver is not properly signed.

We don't know what driver was provided, so let's expect that all drivers are signed in this case.

Currently, in the config, we have only SVVP-related tests and to pass SVVP all drivers must be signed by MS, but these 3 tests required to boot the VM with SB enabled and proper key are in PEK, DBX.

Otherwise, you should be able to keep secure boot enabled for all tests and there should be no need to switch secure and insecure mode.

NO. There are a lot of drivers/apps that are unsigned and used by HLK (also during SVVP). Microsoft even provides a list of tests where you MUST disable SB. We have 2 scenarios of testing driver and SVVP. Currently, we don't have any driver tests that require SB enabled, and our use case is test driver after build (test-signed), so we can't enable SB by default for everything.

Thanks. It clarifies the motivation well.

Copy link
Contributor

@akihikodaki akihikodaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make it run tests without Secure Boot first, and then run tests with it instead of occasionally switching secure and insecure?

@kostyanf14
Copy link
Contributor

Can you make it run tests without Secure Boot first, and then run tests with it instead of occasionally switching secure and insecure?

Yes, but let's think about the next test_config. Our idea is to pass more tests automatically even if we need some specific configuration.

  1. USB Generic HID Test - requires running a specific script on the host that will reattach the MUTT device during the test.
  2. UEFI GOP mode test - required custom (1) OVMF, so we need to reboot the VM anyway.
  3. Hardware Security Testability Interface Test - required custom (2) OVMF, so we need to reboot the VM anyway. We discuss to push the HSTI table into OVMF upstream but currently, QE uses a custom binary for this specific test.

So, in the future, we will have more tests where we should stop VM and reconfigure it.

@akihikodaki
Copy link
Contributor

Can you make it run tests without Secure Boot first, and then run tests with it instead of occasionally switching secure and insecure?

Yes, but let's think about the next test_config. Our idea is to pass more tests automatically even if we need some specific configuration.

1. USB Generic HID Test - requires running a specific script on the host that will reattach the MUTT device during the test.

2. UEFI GOP mode test - required custom (1) OVMF, so we need to reboot the VM anyway.

3. Hardware Security Testability Interface Test  - required custom (2) OVMF, so we need to reboot the VM anyway. We discuss to push the HSTI table into OVMF upstream but currently, QE uses a custom binary for this specific test.

So, in the future, we will have more tests where we should stop VM and reconfigure it.

We can have an algorithm like the following:

# Classify tests first
test_configs = { "requires_secure_boot": ["Secure Boot Logo Test"] }
classified_tests = {}
test_configs.each do |config_name, classified_tests|
  a = tests.select { classified_tests.include? _1 }
  classified_tests[config_name] = a unless a.empty?
end
classified_tests['normal'] = tests.select do |test|
  !h.any? { _2.include? test }
end

# ...and run them
classified_tests.each do |config_name, tests|
  setup_clients config_name
  test tests
  teardown_clients config_name
end

@kostyanf14
Copy link
Contributor

  1. OptStandby - Video Memory Purge and Resume (viogpudo driver test) - required to have S3/S4 enabled for some platforms, but DF - Sleep with IO During (Reliability) required to have S3/S4 disabled. This is because of viogpudo limitation. We need to reboot the VM again.

So any case we will have some specific works to pass more test

@kostyanf14
Copy link
Contributor

# Classify tests first
test_configs = { "requires_secure_boot": ["Secure Boot Logo Test"] }
classified_tests = {}
test_configs.each do |config_name, classified_tests|
  a = tests.select { classified_tests.include? _1 }
  classified_tests[config_name] = a unless a.empty?
end
classified_tests['normal'] = tests.select do |test|
  !h.any? { _2.include? test }
end

# ...and run them
classified_tests.each do |config_name, tests|
  setup_clients config_name
  test tests
  teardown_clients config_name
end

This variant looks better. Also, I have comments about config.

{
    "playlists_path": "./playlists",
    "filters_path": "./filters/UpdateFilters.sql",
    "tests_config": [
        {
            "test": "Secure Boot Logo Test",
            "secure": true
        },
        {
            "test": "OptStandby - Video Memory Purge and Resume",
            "s3_state": true
        },
        {
           "test": "UEFI GOP mode test",
           "uefi_state": { "binary": { "insecure": "/path/to/custom/OVMF.fd" } } }
        }
    ]
}

@akihikodaki What do you think?

@akihikodaki
Copy link
Contributor

# Classify tests first
test_configs = { "requires_secure_boot": ["Secure Boot Logo Test"] }
classified_tests = {}
test_configs.each do |config_name, classified_tests|
  a = tests.select { classified_tests.include? _1 }
  classified_tests[config_name] = a unless a.empty?
end
classified_tests['normal'] = tests.select do |test|
  !h.any? { _2.include? test }
end

# ...and run them
classified_tests.each do |config_name, tests|
  setup_clients config_name
  test tests
  teardown_clients config_name
end

This variant looks better. Also, I have comments about config.

{
    "playlists_path": "./playlists",
    "filters_path": "./filters/UpdateFilters.sql",
    "tests_config": [
        {
            "test": "Secure Boot Logo Test",
            "secure": true
        },
        {
            "test": "OptStandby - Video Memory Purge and Resume",
            "s3_state": true
        },
        {
           "test": "UEFI GOP mode test",
           "uefi_state": { "binary": { "insecure": "/path/to/custom/OVMF.fd" } } }
        }
    ]
}

@akihikodaki What do you think?

Looks good to me. Let's also allow specifying multiple tests for a config:

{
    "playlists_path": "./playlists",
    "filters_path": "./filters/UpdateFilters.sql",
    "tests_config": [
        {
            "tests": ["Secure Boot Logo Test"],
            "secure": true
        },
        {
            "tests": ["OptStandby - Video Memory Purge and Resume"],
            "s3_state": true
        },
        {
           "tests": ["UEFI GOP mode test"],
           "uefi_state": { "binary": { "insecure": "/path/to/custom/OVMF.fd" } } }
        }
    ]
}

@Jedoku Jedoku force-pushed the Require-to-disable-Secure-Boot branch 3 times, most recently from 5370e6f to 98c414b Compare March 4, 2024 19:21
lib/engines/hcktest/hcktest.json Outdated Show resolved Hide resolved
lib/engines/hcktest/hcktest.jtd.json Outdated Show resolved Hide resolved
lib/engines/hcktest/tests.rb Outdated Show resolved Hide resolved
svvp.json Outdated Show resolved Hide resolved
@Jedoku Jedoku requested a review from akihikodaki March 5, 2024 14:33
lib/engines/hcktest/tests.rb Outdated Show resolved Hide resolved
Signed-off-by: Vitalii Chulak <[email protected]>
@Jedoku Jedoku force-pushed the Require-to-disable-Secure-Boot branch from 98c414b to 4188e8f Compare March 7, 2024 06:51
@Jedoku Jedoku requested review from akihikodaki and kostyanf14 March 7, 2024 08:13
Copy link
Contributor

@akihikodaki akihikodaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now the logic to start clients is duplicated in HCKTest and Tests. Can you unify the logic? I have two options in mind:

  • Just move the logic related to clients to Tests.
  • Create a new class managing machines, and use it from both HCKTest and Tests.

@Jedoku Jedoku requested a review from akihikodaki March 7, 2024 12:16
@Jedoku Jedoku closed this Jul 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants