From e60e9902e656150678e6e22fcf53fa57e540c059 Mon Sep 17 00:00:00 2001 From: Mathieu Gravel Date: Mon, 29 Jul 2024 13:54:18 -0700 Subject: [PATCH 01/16] UEFI HID DXE V2 to improve Idiomatic Rust patterns (#504) - use more rust idiomatic naming convention - use `let else` syntax for better readability - Organize import with the same logic in every file. 1. crate from rust 2. crate from crate.io 3. crate from Microsoft 4. use from the same crate - [ ] Impacts functionality? - [ ] Impacts security? - [ ] Breaking change? - [ ] Includes tests? - [ ] Includes documentation? ## How This Was Tested Test that the keyboard was still working. ## Integration Instructions N/A --- HidPkg/UefiHidDxeV2/src/boot_services.rs | 1 + HidPkg/UefiHidDxeV2/src/hid.rs | 14 +- HidPkg/UefiHidDxeV2/src/hid_io.rs | 36 +- .../src/{keyboard/mod.rs => keyboard.rs} | 97 +++-- HidPkg/UefiHidDxeV2/src/keyboard/key_queue.rs | 92 ++--- .../src/keyboard/simple_text_in.rs | 77 ++-- .../src/keyboard/simple_text_in_ex.rs | 330 ++++++++---------- HidPkg/UefiHidDxeV2/src/lib.rs | 8 +- HidPkg/UefiHidDxeV2/src/main.rs | 3 +- .../src/{pointer/mod.rs => pointer.rs} | 9 +- .../src/pointer/absolute_pointer.rs | 50 ++- 11 files changed, 337 insertions(+), 380 deletions(-) rename HidPkg/UefiHidDxeV2/src/{keyboard/mod.rs => keyboard.rs} (93%) rename HidPkg/UefiHidDxeV2/src/{pointer/mod.rs => pointer.rs} (97%) diff --git a/HidPkg/UefiHidDxeV2/src/boot_services.rs b/HidPkg/UefiHidDxeV2/src/boot_services.rs index cd19af0518..4460913c0e 100644 --- a/HidPkg/UefiHidDxeV2/src/boot_services.rs +++ b/HidPkg/UefiHidDxeV2/src/boot_services.rs @@ -11,6 +11,7 @@ //! SPDX-License-Identifier: BSD-2-Clause-Patent //! use core::{ffi::c_void, fmt::Debug, sync::atomic::AtomicPtr}; + #[cfg(test)] use mockall::automock; use r_efi::efi; diff --git a/HidPkg/UefiHidDxeV2/src/hid.rs b/HidPkg/UefiHidDxeV2/src/hid.rs index 37fbd2dc1d..df3f4cc48a 100644 --- a/HidPkg/UefiHidDxeV2/src/hid.rs +++ b/HidPkg/UefiHidDxeV2/src/hid.rs @@ -41,22 +41,20 @@ //! //! SPDX-License-Identifier: BSD-2-Clause-Patent //! +use alloc::{boxed::Box, vec::Vec}; use core::ffi::c_void; +#[cfg(test)] +use mockall::automock; +use r_efi::efi; +use rust_advanced_logger_dxe::{debugln, DEBUG_ERROR}; + use crate::{ boot_services::UefiBootServices, driver_binding::DriverBinding, hid_io::{HidIo, HidIoFactory, HidReportReceiver}, }; -use alloc::{boxed::Box, vec::Vec}; - -use r_efi::efi; -use rust_advanced_logger_dxe::{debugln, DEBUG_ERROR}; - -#[cfg(test)] -use mockall::automock; - /// This trait defines an abstraction for getting a list of receivers for HID reports. /// /// This is used to specify to a HidFactory how it should instantiate new receivers for HID reports. diff --git a/HidPkg/UefiHidDxeV2/src/hid_io.rs b/HidPkg/UefiHidDxeV2/src/hid_io.rs index 805bbedc8d..1726740709 100644 --- a/HidPkg/UefiHidDxeV2/src/hid_io.rs +++ b/HidPkg/UefiHidDxeV2/src/hid_io.rs @@ -10,18 +10,17 @@ //! //! SPDX-License-Identifier: BSD-2-Clause-Patent //! -use core::{ffi::c_void, slice::from_raw_parts_mut}; - use alloc::{boxed::Box, vec}; +use core::{ffi::c_void, ptr, slice::from_raw_parts_mut}; + +#[cfg(test)] +use mockall::automock; +use r_efi::efi; use hid_io::protocol::HidReportType; use hidparser::ReportDescriptor; -use r_efi::efi; use rust_advanced_logger_dxe::{debugln, DEBUG_ERROR}; -#[cfg(test)] -use mockall::automock; - use crate::boot_services::UefiBootServices; /// Defines an interface to be implemented by logic that wants to receive hid reports. @@ -97,7 +96,7 @@ impl UefiHidIo { controller: efi::Handle, owned: bool, ) -> Result { - let mut hid_io_ptr: *mut hid_io::protocol::Protocol = core::ptr::null_mut(); + let mut hid_io_ptr: *mut hid_io::protocol::Protocol = ptr::null_mut(); let attributes = { if owned { @@ -110,7 +109,7 @@ impl UefiHidIo { let status = boot_services.open_protocol( controller, &hid_io::protocol::GUID as *const efi::Guid as *mut efi::Guid, - core::ptr::addr_of_mut!(hid_io_ptr) as *mut *mut c_void, + ptr::addr_of_mut!(hid_io_ptr) as *mut *mut c_void, agent, controller, attributes, @@ -158,8 +157,8 @@ impl HidIo for UefiHidIo { let mut report_descriptor_size: usize = 0; match (self.hid_io.get_report_descriptor)( self.hid_io, - core::ptr::addr_of_mut!(report_descriptor_size), - core::ptr::null_mut(), + ptr::addr_of_mut!(report_descriptor_size), + ptr::null_mut(), ) { efi::Status::BUFFER_TOO_SMALL => (), efi::Status::SUCCESS => return Err(efi::Status::DEVICE_ERROR), @@ -171,7 +170,7 @@ impl HidIo for UefiHidIo { match (self.hid_io.get_report_descriptor)( self.hid_io, - core::ptr::addr_of_mut!(report_descriptor_size), + ptr::addr_of_mut!(report_descriptor_size), report_descriptor_buffer_ptr as *mut c_void, ) { efi::Status::SUCCESS => (), @@ -227,6 +226,7 @@ impl HidIo for UefiHidIo { mod test { use core::{ ffi::c_void, + ptr, slice::{from_raw_parts, from_raw_parts_mut}, }; @@ -270,7 +270,7 @@ mod test { report_descriptor_size: *mut usize, report_descriptor_buffer: *mut c_void, ) -> efi::Status { - assert_ne!(this, core::ptr::null()); + assert_ne!(this, ptr::null()); unsafe { if *report_descriptor_size < MINIMAL_BOOT_KEYBOARD_REPORT_DESCRIPTOR.len() { *report_descriptor_size = MINIMAL_BOOT_KEYBOARD_REPORT_DESCRIPTOR.len(); @@ -301,10 +301,10 @@ mod test { report_buffer_size: usize, report_buffer: *mut c_void, ) -> efi::Status { - assert_ne!(this, core::ptr::null()); + assert_ne!(this, ptr::null()); assert_eq!(report_type, hid_io::protocol::HidReportType::OutputReport); assert_ne!(report_buffer_size, 0); - assert_ne!(report_buffer, core::ptr::null_mut()); + assert_ne!(report_buffer, ptr::null_mut()); let report_slice = unsafe { from_raw_parts(report_buffer as *mut u8, report_buffer_size) }; @@ -326,8 +326,8 @@ mod test { callback: hid_io::protocol::HidIoReportCallback, context: *mut c_void, ) -> efi::Status { - assert_ne!(this, core::ptr::null()); - assert_ne!(context, core::ptr::null_mut()); + assert_ne!(this, ptr::null()); + assert_ne!(context, ptr::null_mut()); assert!(callback == UefiHidIo::report_callback); callback(TEST_REPORT0.len() as u16, TEST_REPORT0.as_ptr() as *mut c_void, context); @@ -339,7 +339,7 @@ mod test { this: *const hid_io::protocol::Protocol, callback: hid_io::protocol::HidIoReportCallback, ) -> efi::Status { - assert_ne!(this, core::ptr::null()); + assert_ne!(this, ptr::null()); assert!(callback == UefiHidIo::report_callback); efi::Status::SUCCESS } @@ -362,7 +362,7 @@ mod test { boot_services.expect_open_protocol().returning(|handle, protocol, interface, agent, controller, attributes| { assert_eq!(handle, 0x1234 as efi::Handle); assert_eq!(unsafe { *protocol }, hid_io::protocol::GUID); - assert_ne!(interface, core::ptr::null_mut()); + assert_ne!(interface, ptr::null_mut()); assert_eq!(agent, 0x4321 as efi::Handle); assert_eq!(controller, 0x1234 as efi::Handle); assert_eq!(attributes, efi::OPEN_PROTOCOL_BY_DRIVER); diff --git a/HidPkg/UefiHidDxeV2/src/keyboard/mod.rs b/HidPkg/UefiHidDxeV2/src/keyboard.rs similarity index 93% rename from HidPkg/UefiHidDxeV2/src/keyboard/mod.rs rename to HidPkg/UefiHidDxeV2/src/keyboard.rs index 0fea4402e4..5f7aedd8e5 100644 --- a/HidPkg/UefiHidDxeV2/src/keyboard/mod.rs +++ b/HidPkg/UefiHidDxeV2/src/keyboard.rs @@ -13,19 +13,21 @@ mod key_queue; mod simple_text_in; mod simple_text_in_ex; -use core::ffi::c_void; - use alloc::{ boxed::Box, collections::{BTreeMap, BTreeSet}, vec, vec::Vec, }; +use core::{ffi::c_void, ptr}; + +use r_efi::{efi, hii, protocols}; + use hidparser::{ report_data_types::{ReportId, Usage}, ArrayField, ReportDescriptor, ReportField, VariableField, }; -use r_efi::{efi, hii, protocols}; +use rust_advanced_logger_dxe::{debugln, function, DEBUG_ERROR, DEBUG_VERBOSE, DEBUG_WARN}; use crate::{ boot_services::UefiBootServices, @@ -33,8 +35,6 @@ use crate::{ keyboard::key_queue::OrdKeyData, }; -use rust_advanced_logger_dxe::{debugln, function, DEBUG_ERROR, DEBUG_VERBOSE, DEBUG_WARN}; - // usages supported by this module const KEYBOARD_MODIFIER_USAGE_MIN: u32 = 0x000700E0; const KEYBOARD_MODIFIER_USAGE_MAX: u32 = 0x000700E7; @@ -182,10 +182,7 @@ impl KeyboardHidHandler { Err(efi::Status::DEVICE_ERROR)?; } - report_builder.report_size = report.size_in_bits / 8; - if (report.size_in_bits % 8) != 0 { - report_builder.report_size += 1; - } + report_builder.report_size = usize::div_ceil(report.size_in_bits, 8); for field in &report.fields { match field { @@ -219,10 +216,8 @@ impl KeyboardHidHandler { // Helper routine to handle variable keyboard input report fields fn handle_variable_key(&mut self, field: VariableField, report: &[u8]) { match field.field_value(report) { - Some(x) if x != 0 => { - self.current_keys.insert(field.usage); - } - None | Some(_) => (), + Some(x) if x != 0 => _ = self.current_keys.insert(field.usage), + _ => (), } } @@ -244,7 +239,7 @@ impl KeyboardHidHandler { self.current_keys.insert(Usage::from(usage)); } } - None | Some(_) => (), + _ => (), } } @@ -262,7 +257,7 @@ impl KeyboardHidHandler { // LEDs. fn generate_led_output_reports(&mut self) -> Vec<(Option, Vec)> { let mut output_vec = Vec::new(); - let current_leds: BTreeSet = self.key_queue.get_active_leds().iter().cloned().collect(); + let current_leds: BTreeSet = self.key_queue.active_leds().iter().cloned().collect(); if current_leds != self.led_state { self.led_state = current_leds; for output_builder in self.output_builders.clone() { @@ -292,14 +287,14 @@ impl KeyboardHidHandler { let context = LayoutChangeContext { boot_services: self.boot_services, keyboard_handler: self as *mut Self }; let context_ptr = Box::into_raw(Box::new(context)); - let mut layout_change_event: efi::Event = core::ptr::null_mut(); + let mut layout_change_event: efi::Event = ptr::null_mut(); let status = self.boot_services.create_event_ex( efi::EVT_NOTIFY_SIGNAL, efi::TPL_NOTIFY, Some(on_layout_update), context_ptr as *mut c_void, &protocols::hii_database::SET_KEYBOARD_LAYOUT_EVENT_GUID, - core::ptr::addr_of_mut!(layout_change_event), + ptr::addr_of_mut!(layout_change_event), ); if status.is_error() { Err(status)?; @@ -322,26 +317,26 @@ impl KeyboardHidHandler { //instance. Leaking context allows context usage in the callback should it fire. debugln!(DEBUG_ERROR, "Failed to close layout_change_event event, status: {:x?}", status); unsafe { - (*self.layout_context).keyboard_handler = core::ptr::null_mut(); + (*self.layout_context).keyboard_handler = ptr::null_mut(); } return Err(status); } // safe to drop layout change context. drop(unsafe { Box::from_raw(self.layout_context) }); - self.layout_context = core::ptr::null_mut(); - self.layout_change_event = core::ptr::null_mut(); + self.layout_context = ptr::null_mut(); + self.layout_change_event = ptr::null_mut(); } Ok(()) } // Installs a default keyboard layout. fn install_default_layout(&mut self) -> Result<(), efi::Status> { - let mut hii_database_protocol_ptr: *mut protocols::hii_database::Protocol = core::ptr::null_mut(); + let mut hii_database_protocol_ptr: *mut protocols::hii_database::Protocol = ptr::null_mut(); let status = self.boot_services.locate_protocol( &protocols::hii_database::PROTOCOL_GUID as *const efi::Guid as *mut efi::Guid, - core::ptr::null_mut(), - core::ptr::addr_of_mut!(hii_database_protocol_ptr) as *mut *mut c_void, + ptr::null_mut(), + ptr::addr_of_mut!(hii_database_protocol_ptr) as *mut *mut c_void, ); if status.is_error() { debugln!( @@ -356,12 +351,12 @@ impl KeyboardHidHandler { hii_database_protocol_ptr.as_mut().expect("Bad pointer returned from successful locate protocol.") }; - let mut hii_handle: hii::Handle = core::ptr::null_mut(); + let mut hii_handle: hii::Handle = ptr::null_mut(); let status = (hii_database_protocol.new_package_list)( hii_database_protocol_ptr, hii_keyboard_layout::get_default_keyboard_pkg_list_buffer().as_ptr() as *const hii::PackageListHeader, - core::ptr::null_mut(), - core::ptr::addr_of_mut!(hii_handle), + ptr::null_mut(), + ptr::addr_of_mut!(hii_handle), ); if status.is_error() { @@ -377,7 +372,6 @@ impl KeyboardHidHandler { hii_database_protocol_ptr, &hii_keyboard_layout::DEFAULT_KEYBOARD_LAYOUT_GUID as *const efi::Guid as *mut efi::Guid, ); - if status.is_error() { debugln!(DEBUG_ERROR, "keyboard::install_default_layout: Failed to set keyboard layout: {:x?}", status); Err(status)?; @@ -394,7 +388,7 @@ impl KeyboardHidHandler { on_layout_update(self.layout_change_event, self.layout_context as *mut c_void); //install a default layout if no layout is installed. - if self.key_queue.get_layout().is_none() { + if self.key_queue.layout().is_none() { self.install_default_layout()?; } Ok(()) @@ -483,7 +477,7 @@ impl KeyboardHidHandler { /// Returns the set of keys that have pending callbacks, along with the vector of callback functions associated with /// each key. - pub fn get_pending_callbacks( + pub fn pending_callbacks( &mut self, ) -> (Option, Vec) { @@ -501,12 +495,12 @@ impl KeyboardHidHandler { } /// Returns the agent associated with this KeyboardHidHandler - pub fn get_agent(&self) -> efi::Handle { + pub fn agent(&self) -> efi::Handle { self.agent } /// Returns the controller associated with this KeyboardHidHandler. - pub fn get_controller(&self) -> Option { + pub fn controller(&self) -> Option { self.controller } @@ -529,11 +523,8 @@ impl HidReportReceiver for KeyboardHidHandler { fn initialize(&mut self, controller: efi::Handle, hid_io: &dyn HidIo) -> Result<(), efi::Status> { let descriptor = hid_io.get_report_descriptor()?; self.process_descriptor(descriptor)?; - self.install_protocol_interfaces(controller)?; - self.initialize_keyboard_layout()?; - Ok(()) } @@ -568,7 +559,6 @@ impl HidReportReceiver for KeyboardHidHandler { report.len() ); debugln!(DEBUG_VERBOSE, "report: {:x?}", report); - //break 'report_processing; } //reset currently active keys to empty set. @@ -632,7 +622,6 @@ impl HidReportReceiver for KeyboardHidHandler { let result = hid_io.set_output_report(id.map(|x| u32::from(x) as u8), &output_report); if let Err(result) = result { debugln!(DEBUG_ERROR, "unexpected error sending output report: {:?}", result); - let _ = result; } } } @@ -641,17 +630,17 @@ impl HidReportReceiver for KeyboardHidHandler { impl Drop for KeyboardHidHandler { fn drop(&mut self) { if let Some(controller) = self.controller { - let status = simple_text_in::SimpleTextInFfi::uninstall(self.boot_services, self.agent, controller); - if status.is_err() { + if let Err(status) = simple_text_in::SimpleTextInFfi::uninstall(self.boot_services, self.agent, controller) + { debugln!(DEBUG_ERROR, "KeyboardHidHandler::drop: Failed to uninstall simple_text_in: {:?}", status); } - let status = simple_text_in_ex::SimpleTextInExFfi::uninstall(self.boot_services, self.agent, controller); - if status.is_err() { + if let Err(status) = + simple_text_in_ex::SimpleTextInExFfi::uninstall(self.boot_services, self.agent, controller) + { debugln!(DEBUG_ERROR, "KeyboardHidHandler::drop: Failed to uninstall simple_text_in: {:?}", status); } } - let status = self.uninstall_layout_change_event(); - if status.is_err() { + if let Err(status) = self.uninstall_layout_change_event() { debugln!(DEBUG_ERROR, "KeyboardHidHandler::drop: Failed to close layout_change_event: {:?}", status); } } @@ -670,11 +659,11 @@ extern "efiapi" fn on_layout_update(_event: efi::Event, context: *mut c_void) { let keyboard_handler = unsafe { context.keyboard_handler.as_mut() }.expect("bad keyboard handler"); - let mut hii_database_protocol_ptr: *mut protocols::hii_database::Protocol = core::ptr::null_mut(); + let mut hii_database_protocol_ptr: *mut protocols::hii_database::Protocol = ptr::null_mut(); let status = context.boot_services.locate_protocol( &protocols::hii_database::PROTOCOL_GUID as *const efi::Guid as *mut efi::Guid, - core::ptr::null_mut(), - core::ptr::addr_of_mut!(hii_database_protocol_ptr) as *mut *mut c_void, + ptr::null_mut(), + ptr::addr_of_mut!(hii_database_protocol_ptr) as *mut *mut c_void, ); if status.is_error() { @@ -690,9 +679,9 @@ extern "efiapi" fn on_layout_update(_event: efi::Event, context: *mut c_void) { let mut layout_buffer_len: u16 = 0; match (hii_database_protocol.get_keyboard_layout)( hii_database_protocol_ptr, - core::ptr::null_mut(), + ptr::null_mut(), &mut layout_buffer_len as *mut u16, - core::ptr::null_mut(), + ptr::null_mut(), ) { efi::Status::NOT_FOUND => break 'layout_processing, status if status != efi::Status::BUFFER_TOO_SMALL => { @@ -710,7 +699,7 @@ extern "efiapi" fn on_layout_update(_event: efi::Event, context: *mut c_void) { let mut keyboard_layout_buffer = vec![0u8; layout_buffer_len as usize]; let status = (hii_database_protocol.get_keyboard_layout)( hii_database_protocol_ptr, - core::ptr::null_mut(), + ptr::null_mut(), &mut layout_buffer_len as *mut u16, keyboard_layout_buffer.as_mut_ptr() as *mut protocols::hii_database::KeyboardLayout<0>, ); @@ -1213,8 +1202,8 @@ mod test { | protocols::simple_text_input_ex::CAPS_LOCK_ACTIVE ); - assert_eq!(keyboard_handler.get_controller(), keyboard_handler.controller); - assert_eq!(keyboard_handler.get_agent(), keyboard_handler.agent); + assert_eq!(keyboard_handler.controller(), keyboard_handler.controller); + assert_eq!(keyboard_handler.agent(), keyboard_handler.agent); } #[test] @@ -1295,7 +1284,7 @@ mod test { key_data.key_state.key_shift_state = protocols::simple_text_input_ex::SHIFT_STATE_VALID; key_data.key_state.key_toggle_state = protocols::simple_text_input_ex::TOGGLE_STATE_VALID; - let (callback_key_data, callbacks) = keyboard_handler.get_pending_callbacks(); + let (callback_key_data, callbacks) = keyboard_handler.pending_callbacks(); assert_eq!(OrdKeyData(key_data), OrdKeyData(callback_key_data.unwrap())); assert!(callbacks.contains( &(mock_key_notify_callback @@ -1306,7 +1295,7 @@ mod test { as extern "efiapi" fn(*mut protocols::simple_text_input_ex::KeyData) -> efi::Status) )); - let (callback_key_data, callbacks) = keyboard_handler.get_pending_callbacks(); + let (callback_key_data, callbacks) = keyboard_handler.pending_callbacks(); assert!(callback_key_data.is_none()); assert!(callbacks.is_empty()); @@ -1320,7 +1309,7 @@ mod test { keyboard_handler.receive_report(report, &hid_io); loop { - let (callback_key_data, callbacks) = keyboard_handler.get_pending_callbacks(); + let (callback_key_data, callbacks) = keyboard_handler.pending_callbacks(); if let Some(callback_key_data) = callback_key_data { match callback_key_data.key.unicode_char { char if char == 'a' as u16 || char == 'c' as u16 => { @@ -1347,7 +1336,7 @@ mod test { let report: &[u8] = &[0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]; keyboard_handler.receive_report(report, &hid_io); - let (callback_key_data, callbacks) = keyboard_handler.get_pending_callbacks(); + let (callback_key_data, callbacks) = keyboard_handler.pending_callbacks(); assert!(callback_key_data.is_none()); assert!(callbacks.is_empty()); } diff --git a/HidPkg/UefiHidDxeV2/src/keyboard/key_queue.rs b/HidPkg/UefiHidDxeV2/src/keyboard/key_queue.rs index f7fcd9b068..bee0bcf55f 100644 --- a/HidPkg/UefiHidDxeV2/src/keyboard/key_queue.rs +++ b/HidPkg/UefiHidDxeV2/src/keyboard/key_queue.rs @@ -9,19 +9,17 @@ //! SPDX-License-Identifier: BSD-2-Clause-Patent //! -use core::sync::atomic::Ordering; - use alloc::{ collections::{BTreeSet, VecDeque}, vec::Vec, }; +use core::{ops::Deref, sync::atomic::Ordering}; use hidparser::report_data_types::Usage; -use hii_keyboard_layout::{EfiKey, HiiKey, HiiKeyDescriptor, HiiKeyboardLayout, HiiNsKeyDescriptor}; +use hii_keyboard_layout::{EfiKey, HiiKey, HiiKeyboardLayout, HiiNsKeyDescriptor}; use r_efi::{ efi, protocols::{self, hii_database::*, simple_text_input::InputKey, simple_text_input_ex::*}, }; - use rust_advanced_logger_dxe::{debugln, DEBUG_WARN}; use crate::RUNTIME_SERVICES; @@ -31,7 +29,8 @@ use crate::RUNTIME_SERVICES; const KEYBOARD_MODIFIERS: &[u16] = &[ LEFT_CONTROL_MODIFIER, RIGHT_CONTROL_MODIFIER, LEFT_SHIFT_MODIFIER, RIGHT_SHIFT_MODIFIER, LEFT_ALT_MODIFIER, RIGHT_ALT_MODIFIER, LEFT_LOGO_MODIFIER, RIGHT_LOGO_MODIFIER, MENU_MODIFIER, PRINT_MODIFIER, SYS_REQUEST_MODIFIER, - ALT_GR_MODIFIER]; + ALT_GR_MODIFIER, +]; // The set of HID usages that represent modifier keys that toggle state (as opposed to remain active while pressed). const TOGGLE_MODIFIERS: &[u16] = &[NUM_LOCK_MODIFIER, CAPS_LOCK_MODIFIER, SCROLL_LOCK_MODIFIER]; @@ -41,34 +40,42 @@ const CTRL_MODIFIERS: &[u16] = &[LEFT_CONTROL_MODIFIER, RIGHT_CONTROL_MODIFIER]; const SHIFT_MODIFIERS: &[u16] = &[LEFT_SHIFT_MODIFIER, RIGHT_SHIFT_MODIFIER]; const ALT_MODIFIERS: &[u16] = &[LEFT_ALT_MODIFIER, RIGHT_ALT_MODIFIER]; -// Defines whether a key stroke represents a key being pressed (KeyDown) or released (KeyUp) +/// Defines whether a key stroke represents a key being pressed (KeyDown) or released (KeyUp) #[derive(Debug, PartialEq, Eq)] pub(crate) enum KeyAction { - // Key is being pressed - KeyUp, - // Key is being released + /// Key is being pressed KeyDown, + /// Key is being released + KeyUp, } -// A wrapper for the KeyData type that allows definition of the Ord trait and additional registration matching logic. +/// A wrapper for the KeyData type that allows definition of the Ord trait and additional registration matching logic. #[derive(Debug, Clone)] pub(crate) struct OrdKeyData(pub protocols::simple_text_input_ex::KeyData); +impl Deref for OrdKeyData { + type Target = protocols::simple_text_input_ex::KeyData; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + impl Ord for OrdKeyData { fn cmp(&self, other: &Self) -> core::cmp::Ordering { - let e = self.0.key.unicode_char.cmp(&other.0.key.unicode_char); + let e = self.key.unicode_char.cmp(&other.key.unicode_char); if !e.is_eq() { return e; } - let e = self.0.key.scan_code.cmp(&other.0.key.scan_code); + let e = self.key.scan_code.cmp(&other.key.scan_code); if !e.is_eq() { return e; } - let e = self.0.key_state.key_shift_state.cmp(&other.0.key_state.key_shift_state); + let e = self.key_state.key_shift_state.cmp(&other.key_state.key_shift_state); if !e.is_eq() { return e; } - self.0.key_state.key_toggle_state.cmp(&other.0.key_state.key_toggle_state) + self.key_state.key_toggle_state.cmp(&other.key_state.key_toggle_state) } } @@ -91,14 +98,14 @@ impl OrdKeyData { // allows for some degree of wildcard matching. Refer to UEFI spec 2.10 section 12.2.5. pub(crate) fn matches_registered_key(&self, registration: &Self) -> bool { // assign names here for brevity below. - let self_char = self.0.key.unicode_char; - let self_scan = self.0.key.scan_code; - let self_shift = self.0.key_state.key_shift_state; - let self_toggle = self.0.key_state.key_toggle_state; - let register_char = registration.0.key.unicode_char; - let register_scan = registration.0.key.scan_code; - let register_shift = registration.0.key_state.key_shift_state; - let register_toggle = registration.0.key_state.key_toggle_state; + let self_char = self.key.unicode_char; + let self_scan = self.key.scan_code; + let self_shift = self.key_state.key_shift_state; + let self_toggle = self.key_state.key_toggle_state; + let register_char = registration.key.unicode_char; + let register_scan = registration.key.scan_code; + let register_shift = registration.key_state.key_shift_state; + let register_toggle = registration.key_state.key_toggle_state; //char and scan must match (per the reference implementation in the EDK2 C code). if !(register_char == self_char && register_scan == self_scan) { @@ -136,7 +143,7 @@ impl KeyQueue { if extended_reset { self.active_modifiers.clear(); } else { - let active_leds = self.get_active_led_modifiers(); + let active_leds = self.active_led_modifiers(); self.active_modifiers.retain(|x| active_leds.contains(x)); } self.active_ns_key = None; @@ -159,7 +166,7 @@ impl KeyQueue { // Check if it is a dependent key of a currently active "non-spacing" (ns) key. // Non-spacing key handling is described in UEFI spec 2.10 section 33.2.4.3. - let mut current_descriptor: Option = None; + let mut current_descriptor = None; if let Some(ref ns_key) = self.active_ns_key { for descriptor in &ns_key.dependent_keys { if descriptor.key == efi_key { @@ -176,38 +183,33 @@ impl KeyQueue { if current_descriptor.is_none() { for key in &active_layout.keys { match key { - HiiKey::Key(descriptor) => { - if descriptor.key == efi_key { - current_descriptor = Some(*descriptor); - break; - } + HiiKey::Key(descriptor) if descriptor.key == efi_key => { + current_descriptor = Some(*descriptor); + break; } - HiiKey::NsKey(ns_descriptor) => { - if ns_descriptor.descriptor.key == efi_key { - // if it is an ns_key, set it as the active ns key, and no further processing is needed. - self.active_ns_key = Some(ns_descriptor.clone()); - return; - } + HiiKey::NsKey(ns_descriptor) if ns_descriptor.descriptor.key == efi_key => { + // if it is an ns_key, set it as the active ns key, and no further processing is needed. + self.active_ns_key = Some(ns_descriptor.clone()); + return; } + _ => continue, } } } - if current_descriptor.is_none() { + let Some(current_descriptor) = current_descriptor else { return; //could not find descriptor, nothing to do. - } - - let current_descriptor = current_descriptor.unwrap(); + }; //handle modifiers that are active as long as they are pressed if KEYBOARD_MODIFIERS.contains(¤t_descriptor.modifier) { match action { - KeyAction::KeyUp => { - self.active_modifiers.remove(¤t_descriptor.modifier); - } KeyAction::KeyDown => { self.active_modifiers.insert(current_descriptor.modifier); } + KeyAction::KeyUp => { + self.active_modifiers.remove(¤t_descriptor.modifier); + } } } @@ -401,16 +403,16 @@ impl KeyQueue { self.partial_key_support_active = (toggle_state & KEY_STATE_EXPOSED) != 0; } - fn get_active_led_modifiers(&self) -> Vec { + fn active_led_modifiers(&self) -> Vec { self.active_modifiers.iter().cloned().filter(|x| modifier_to_led_usage(*x).is_some()).collect() } // Returns a vector of HID usages corresponding to the active LEDs based on the active modifier state. - pub(crate) fn get_active_leds(&self) -> Vec { + pub(crate) fn active_leds(&self) -> Vec { self.active_modifiers.iter().cloned().filter_map(modifier_to_led_usage).collect() } // Returns the current keyboard layout that the KeyQueue is using. - pub(crate) fn get_layout(&self) -> Option { + pub(crate) fn layout(&self) -> Option { self.layout.clone() } diff --git a/HidPkg/UefiHidDxeV2/src/keyboard/simple_text_in.rs b/HidPkg/UefiHidDxeV2/src/keyboard/simple_text_in.rs index 2b8c7e9170..94ce4acc7d 100644 --- a/HidPkg/UefiHidDxeV2/src/keyboard/simple_text_in.rs +++ b/HidPkg/UefiHidDxeV2/src/keyboard/simple_text_in.rs @@ -9,7 +9,7 @@ //! use alloc::boxed::Box; -use core::ffi::c_void; +use core::{ffi::c_void, ptr}; use r_efi::{efi, protocols}; use rust_advanced_logger_dxe::{debugln, DEBUG_ERROR}; @@ -51,7 +51,7 @@ impl SimpleTextInFfi { simple_text_in: protocols::simple_text_input::Protocol { reset: Self::simple_text_in_reset, read_key_stroke: Self::simple_text_in_read_key_stroke, - wait_for_key: core::ptr::null_mut(), + wait_for_key: ptr::null_mut(), }, boot_services, keyboard_handler: keyboard_handler as *mut KeyboardHidHandler, @@ -60,13 +60,13 @@ impl SimpleTextInFfi { let simple_text_in_ptr = Box::into_raw(Box::new(simple_text_in_ctx)); //create event for wait_for_key - let mut wait_for_key_event: efi::Event = core::ptr::null_mut(); + let mut wait_for_key_event: efi::Event = ptr::null_mut(); let status = boot_services.create_event( efi::EVT_NOTIFY_WAIT, efi::TPL_NOTIFY, Some(Self::simple_text_in_wait_for_key), simple_text_in_ptr as *mut c_void, - core::ptr::addr_of_mut!(wait_for_key_event), + ptr::addr_of_mut!(wait_for_key_event), ); if status.is_error() { drop(unsafe { Box::from_raw(simple_text_in_ptr) }); @@ -78,7 +78,7 @@ impl SimpleTextInFfi { //install the simple_text_in protocol let mut controller = controller; let status = boot_services.install_protocol_interface( - core::ptr::addr_of_mut!(controller), + ptr::addr_of_mut!(controller), &protocols::simple_text_input::PROTOCOL_GUID as *const efi::Guid as *mut efi::Guid, efi::NATIVE_INTERFACE, simple_text_in_ptr as *mut c_void, @@ -101,11 +101,11 @@ impl SimpleTextInFfi { ) -> Result<(), efi::Status> { //Controller is set - that means initialize() was called, and there is potential state exposed thru FFI that needs //to be cleaned up. - let mut simple_text_in_ptr: *mut SimpleTextInFfi = core::ptr::null_mut(); + let mut simple_text_in_ptr: *mut SimpleTextInFfi = ptr::null_mut(); let status = boot_services.open_protocol( controller, &protocols::simple_text_input::PROTOCOL_GUID as *const efi::Guid as *mut efi::Guid, - core::ptr::addr_of_mut!(simple_text_in_ptr) as *mut *mut c_void, + ptr::addr_of_mut!(simple_text_in_ptr) as *mut *mut c_void, agent, controller, efi::OPEN_PROTOCOL_GET_PROTOCOL, @@ -130,7 +130,7 @@ impl SimpleTextInFfi { debugln!(DEBUG_ERROR, "Failed to uninstall simple_text_in interface, status: {:x?}", status); unsafe { - (*simple_text_in_ptr).keyboard_handler = core::ptr::null_mut(); + (*simple_text_in_ptr).keyboard_handler = ptr::null_mut(); } //return without tearing down the context. return Err(status); @@ -146,7 +146,7 @@ impl SimpleTextInFfi { //and return error based on observing keyboard_handler is null. debugln!(DEBUG_ERROR, "Failed to close simple_text_in_ptr.wait_for_key event, status: {:x?}", status); unsafe { - (*simple_text_in_ptr).keyboard_handler = core::ptr::null_mut(); + (*simple_text_in_ptr).keyboard_handler = ptr::null_mut(); } return Err(status); } @@ -170,8 +170,8 @@ impl SimpleTextInFfi { let keyboard_handler = unsafe { context.keyboard_handler.as_mut() }; if let Some(keyboard_handler) = keyboard_handler { //reset requires an instance of hid_io to allow for LED updating, so use UefiHidIoFactory to build one. - if let Some(controller) = keyboard_handler.get_controller() { - let hid_io = UefiHidIoFactory::new(context.boot_services, keyboard_handler.get_agent()) + if let Some(controller) = keyboard_handler.controller() { + let hid_io = UefiHidIoFactory::new(context.boot_services, keyboard_handler.agent()) .new_hid_io(controller, false); if let Ok(hid_io) = hid_io { if let Err(err) = keyboard_handler.reset(hid_io.as_ref(), extended_verification.into()) { @@ -268,7 +268,8 @@ mod test { use core::{ ffi::c_void, mem::MaybeUninit, - sync::atomic::{AtomicBool, AtomicPtr}, + ptr, + sync::atomic::{AtomicBool, AtomicPtr, Ordering}, }; use crate::{ @@ -339,13 +340,13 @@ mod test { #[test] fn uninstall_should_uninstall_simple_text_in_interface() { - static CONTEXT_PTR: AtomicPtr = AtomicPtr::new(core::ptr::null_mut()); + static CONTEXT_PTR: AtomicPtr = AtomicPtr::new(ptr::null_mut()); let boot_services = create_fake_static_boot_service(); // used in install boot_services.expect_create_event().returning(|_, _, _, _, _| efi::Status::SUCCESS); boot_services.expect_install_protocol_interface().returning(|_, _, _, interface| { - CONTEXT_PTR.store(interface, core::sync::atomic::Ordering::SeqCst); + CONTEXT_PTR.store(interface, Ordering::SeqCst); efi::Status::SUCCESS }); @@ -353,14 +354,14 @@ mod test { boot_services.expect_open_protocol().returning(|_, protocol, interface, _, _, _| { unsafe { assert_eq!(protocol.read(), protocols::simple_text_input::PROTOCOL_GUID); - interface.write(CONTEXT_PTR.load(core::sync::atomic::Ordering::SeqCst)); + interface.write(CONTEXT_PTR.load(Ordering::SeqCst)); } efi::Status::SUCCESS }); boot_services.expect_uninstall_protocol_interface().returning(|_, protocol, interface| { unsafe { assert_eq!(protocol.read(), protocols::simple_text_input::PROTOCOL_GUID); - assert_eq!(interface, CONTEXT_PTR.load(core::sync::atomic::Ordering::SeqCst)); + assert_eq!(interface, CONTEXT_PTR.load(Ordering::SeqCst)); } efi::Status::SUCCESS }); @@ -369,20 +370,20 @@ mod test { let mut keyboard_handler = KeyboardHidHandler::new(boot_services, 1 as efi::Handle); SimpleTextInFfi::install(boot_services, 2 as efi::Handle, &mut keyboard_handler).unwrap(); - assert_ne!(CONTEXT_PTR.load(core::sync::atomic::Ordering::SeqCst), core::ptr::null_mut()); + assert_ne!(CONTEXT_PTR.load(Ordering::SeqCst), ptr::null_mut()); SimpleTextInFfi::uninstall(boot_services, 1 as efi::Handle, 2 as efi::Handle).unwrap(); } #[test] fn reset_should_invoke_reset() { - static CONTEXT_PTR: AtomicPtr = AtomicPtr::new(core::ptr::null_mut()); + static CONTEXT_PTR: AtomicPtr = AtomicPtr::new(ptr::null_mut()); let boot_services = create_fake_static_boot_service(); // used in install boot_services.expect_create_event().returning(|_, _, _, _, _| efi::Status::SUCCESS); boot_services.expect_install_protocol_interface().returning(|_, _, _, interface| { - CONTEXT_PTR.store(interface, core::sync::atomic::Ordering::SeqCst); + CONTEXT_PTR.store(interface, Ordering::SeqCst); efi::Status::SUCCESS }); @@ -422,7 +423,7 @@ mod test { boot_services.expect_uninstall_protocol_interface().returning(|_, protocol, interface| { unsafe { assert_eq!(protocol.read(), protocols::simple_text_input::PROTOCOL_GUID); - assert_eq!(interface, CONTEXT_PTR.load(core::sync::atomic::Ordering::SeqCst)); + assert_eq!(interface, CONTEXT_PTR.load(Ordering::SeqCst)); } efi::Status::SUCCESS }); @@ -432,10 +433,9 @@ mod test { keyboard_handler.set_controller(Some(2 as efi::Handle)); SimpleTextInFfi::install(boot_services, 2 as efi::Handle, &mut keyboard_handler).unwrap(); - assert_ne!(CONTEXT_PTR.load(core::sync::atomic::Ordering::SeqCst), core::ptr::null_mut()); + assert_ne!(CONTEXT_PTR.load(Ordering::SeqCst), ptr::null_mut()); - let this = - CONTEXT_PTR.load(core::sync::atomic::Ordering::SeqCst) as *mut protocols::simple_text_input::Protocol; + let this = CONTEXT_PTR.load(Ordering::SeqCst) as *mut protocols::simple_text_input::Protocol; SimpleTextInFfi::simple_text_in_reset(this, efi::Boolean::FALSE); // avoid keyboard drop uninstall flows @@ -444,13 +444,13 @@ mod test { #[test] fn read_key_stroke_should_read_keystrokes() { - static CONTEXT_PTR: AtomicPtr = AtomicPtr::new(core::ptr::null_mut()); + static CONTEXT_PTR: AtomicPtr = AtomicPtr::new(ptr::null_mut()); let boot_services = create_fake_static_boot_service(); // used in install boot_services.expect_create_event().returning(|_, _, _, _, _| efi::Status::SUCCESS); boot_services.expect_install_protocol_interface().returning(|_, _, _, interface| { - CONTEXT_PTR.store(interface, core::sync::atomic::Ordering::SeqCst); + CONTEXT_PTR.store(interface, Ordering::SeqCst); efi::Status::SUCCESS }); @@ -475,7 +475,7 @@ mod test { keyboard_handler.initialize(2 as efi::Handle, &hid_io).unwrap(); SimpleTextInFfi::install(boot_services, 2 as efi::Handle, &mut keyboard_handler).unwrap(); - assert_ne!(CONTEXT_PTR.load(core::sync::atomic::Ordering::SeqCst), core::ptr::null_mut()); + assert_ne!(CONTEXT_PTR.load(Ordering::SeqCst), ptr::null_mut()); let report: &[u8] = &[0x00, 0x00, 0x04, 0x05, 0x06, 0x00, 0x00, 0x00]; keyboard_handler.receive_report(report, &hid_io); @@ -484,8 +484,7 @@ mod test { keyboard_handler.receive_report(report, &hid_io); //read with simple_text_in - let this = - CONTEXT_PTR.load(core::sync::atomic::Ordering::SeqCst) as *mut protocols::simple_text_input::Protocol; + let this = CONTEXT_PTR.load(Ordering::SeqCst) as *mut protocols::simple_text_input::Protocol; let mut input_key: protocols::simple_text_input::InputKey = Default::default(); let status = SimpleTextInFfi::simple_text_in_read_key_stroke( @@ -582,7 +581,7 @@ mod test { #[test] fn wait_for_key_should_wait_for_key() { - static CONTEXT_PTR: AtomicPtr = AtomicPtr::new(core::ptr::null_mut()); + static CONTEXT_PTR: AtomicPtr = AtomicPtr::new(ptr::null_mut()); static RECEIVED_EVENT: AtomicBool = AtomicBool::new(false); let boot_services = create_fake_static_boot_service(); @@ -592,7 +591,7 @@ mod test { boot_services.expect_locate_protocol().returning(|_, _, _| efi::Status::NOT_FOUND); boot_services.expect_install_protocol_interface().returning(|_, protocol, _, interface| { if unsafe { *protocol } == protocols::simple_text_input::PROTOCOL_GUID { - CONTEXT_PTR.store(interface, core::sync::atomic::Ordering::SeqCst); + CONTEXT_PTR.store(interface, Ordering::SeqCst); } efi::Status::SUCCESS }); @@ -601,7 +600,7 @@ mod test { boot_services.expect_restore_tpl().returning(|_| ()); boot_services.expect_signal_event().returning(|_| { - RECEIVED_EVENT.store(true, core::sync::atomic::Ordering::SeqCst); + RECEIVED_EVENT.store(true, Ordering::SeqCst); efi::Status::SUCCESS }); @@ -616,12 +615,9 @@ mod test { keyboard_handler.set_controller(Some(2 as efi::Handle)); keyboard_handler.initialize(2 as efi::Handle, &hid_io).unwrap(); - RECEIVED_EVENT.store(false, core::sync::atomic::Ordering::SeqCst); - SimpleTextInFfi::simple_text_in_wait_for_key( - 3 as efi::Event, - CONTEXT_PTR.load(core::sync::atomic::Ordering::SeqCst), - ); - assert!(!RECEIVED_EVENT.load(core::sync::atomic::Ordering::SeqCst)); + RECEIVED_EVENT.store(false, Ordering::SeqCst); + SimpleTextInFfi::simple_text_in_wait_for_key(3 as efi::Event, CONTEXT_PTR.load(Ordering::SeqCst)); + assert!(!RECEIVED_EVENT.load(Ordering::SeqCst)); // press the a key let report: &[u8] = &[0x00, 0x00, 0x04, 0x00, 0x00, 0x00, 0x00, 0x00]; @@ -631,11 +627,8 @@ mod test { let report: &[u8] = &[0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]; keyboard_handler.receive_report(report, &hid_io); - SimpleTextInFfi::simple_text_in_wait_for_key( - 3 as efi::Event, - CONTEXT_PTR.load(core::sync::atomic::Ordering::SeqCst), - ); - assert!(RECEIVED_EVENT.load(core::sync::atomic::Ordering::SeqCst)); + SimpleTextInFfi::simple_text_in_wait_for_key(3 as efi::Event, CONTEXT_PTR.load(Ordering::SeqCst)); + assert!(RECEIVED_EVENT.load(Ordering::SeqCst)); // avoid keyboard drop uninstall flows keyboard_handler.set_controller(None); diff --git a/HidPkg/UefiHidDxeV2/src/keyboard/simple_text_in_ex.rs b/HidPkg/UefiHidDxeV2/src/keyboard/simple_text_in_ex.rs index ea71486418..62ecdfe300 100644 --- a/HidPkg/UefiHidDxeV2/src/keyboard/simple_text_in_ex.rs +++ b/HidPkg/UefiHidDxeV2/src/keyboard/simple_text_in_ex.rs @@ -8,7 +8,7 @@ //! SPDX-License-Identifier: BSD-2-Clause-Patent //! use alloc::{boxed::Box, vec::Vec}; -use core::ffi::c_void; +use core::{ffi::c_void, ptr}; use r_efi::{efi, protocols}; use rust_advanced_logger_dxe::{debugln, DEBUG_ERROR}; @@ -51,26 +51,26 @@ impl SimpleTextInExFfi { simple_text_in_ex: protocols::simple_text_input_ex::Protocol { reset: Self::simple_text_in_ex_reset, read_key_stroke_ex: Self::simple_text_in_ex_read_key_stroke, - wait_for_key_ex: core::ptr::null_mut(), + wait_for_key_ex: ptr::null_mut(), set_state: Self::simple_text_in_ex_set_state, register_key_notify: Self::simple_text_in_ex_register_key_notify, unregister_key_notify: Self::simple_text_in_ex_unregister_key_notify, }, boot_services, - key_notify_event: core::ptr::null_mut(), + key_notify_event: ptr::null_mut(), keyboard_handler: keyboard_handler as *mut KeyboardHidHandler, }; let simple_text_in_ex_ptr = Box::into_raw(Box::new(simple_text_in_ex_ctx)); //create event for wait_for_key - let mut wait_for_key_event: efi::Event = core::ptr::null_mut(); + let mut wait_for_key_event: efi::Event = ptr::null_mut(); let status = boot_services.create_event( efi::EVT_NOTIFY_WAIT, efi::TPL_NOTIFY, Some(Self::simple_text_in_ex_wait_for_key), simple_text_in_ex_ptr as *mut c_void, - core::ptr::addr_of_mut!(wait_for_key_event), + ptr::addr_of_mut!(wait_for_key_event), ); if status.is_error() { drop(unsafe { Box::from_raw(simple_text_in_ex_ptr) }); @@ -82,13 +82,13 @@ impl SimpleTextInExFfi { //Key notifies are required to dispatch at TPL_CALLBACK per UEFI spec 2.10 section 12.2.5. The keyboard handler //interfaces run at TPL_NOTIFY and issue a boot_services.signal_event() on this event to pend key notifies to be //serviced at TPL_CALLBACK. - let mut key_notify_event: efi::Event = core::ptr::null_mut(); + let mut key_notify_event: efi::Event = ptr::null_mut(); let status = boot_services.create_event( efi::EVT_NOTIFY_SIGNAL, efi::TPL_CALLBACK, Some(Self::process_key_notifies), simple_text_in_ex_ptr as *mut c_void, - core::ptr::addr_of_mut!(key_notify_event), + ptr::addr_of_mut!(key_notify_event), ); if status.is_error() { let _ = boot_services.close_event(wait_for_key_event); @@ -100,7 +100,7 @@ impl SimpleTextInExFfi { //install the simple_text_in_ex protocol let mut controller = controller; let status = boot_services.install_protocol_interface( - core::ptr::addr_of_mut!(controller), + ptr::addr_of_mut!(controller), &protocols::simple_text_input_ex::PROTOCOL_GUID as *const efi::Guid as *mut efi::Guid, efi::NATIVE_INTERFACE, simple_text_in_ex_ptr as *mut c_void, @@ -124,11 +124,11 @@ impl SimpleTextInExFfi { ) -> Result<(), efi::Status> { //Controller is set - that means initialize() was called, and there is potential state exposed thru FFI that needs //to be cleaned up. - let mut simple_text_in_ex_ptr: *mut SimpleTextInExFfi = core::ptr::null_mut(); + let mut simple_text_in_ex_ptr: *mut SimpleTextInExFfi = ptr::null_mut(); let status = boot_services.open_protocol( controller, &protocols::simple_text_input_ex::PROTOCOL_GUID as *const efi::Guid as *mut efi::Guid, - core::ptr::addr_of_mut!(simple_text_in_ex_ptr) as *mut *mut c_void, + ptr::addr_of_mut!(simple_text_in_ex_ptr) as *mut *mut c_void, agent, controller, efi::OPEN_PROTOCOL_GET_PROTOCOL, @@ -153,7 +153,7 @@ impl SimpleTextInExFfi { debugln!(DEBUG_ERROR, "Failed to uninstall simple_text_in interface, status: {:x?}", status); unsafe { - (*simple_text_in_ex_ptr).keyboard_handler = core::ptr::null_mut(); + (*simple_text_in_ex_ptr).keyboard_handler = ptr::null_mut(); } //return without tearing down the context. return Err(status); @@ -169,7 +169,7 @@ impl SimpleTextInExFfi { //and return error based on observing keyboard_handler is null. debugln!(DEBUG_ERROR, "Failed to close simple_text_in_ptr.wait_for_key event, status: {:x?}", status); unsafe { - (*simple_text_in_ex_ptr).keyboard_handler = core::ptr::null_mut(); + (*simple_text_in_ex_ptr).keyboard_handler = ptr::null_mut(); } return Err(status); } @@ -184,7 +184,7 @@ impl SimpleTextInExFfi { //and return error based on observing keyboard_handler is null. debugln!(DEBUG_ERROR, "Failed to close key_notify_event event, status: {:x?}", status); unsafe { - (*simple_text_in_ex_ptr).keyboard_handler = core::ptr::null_mut(); + (*simple_text_in_ex_ptr).keyboard_handler = ptr::null_mut(); } return Err(status); } @@ -204,24 +204,19 @@ impl SimpleTextInExFfi { } let context = unsafe { (this as *mut SimpleTextInExFfi).as_mut() }.expect("bad pointer"); let old_tpl = context.boot_services.raise_tpl(efi::TPL_NOTIFY); - let mut status = efi::Status::DEVICE_ERROR; - '_reset_processing: { - let keyboard_handler = unsafe { context.keyboard_handler.as_mut() }; - if let Some(keyboard_handler) = keyboard_handler { - //reset requires an instance of hid_io to allow for LED updating, so use UefiHidIoFactory to build one. - if let Some(controller) = keyboard_handler.get_controller() { - let hid_io = UefiHidIoFactory::new(context.boot_services, keyboard_handler.get_agent()) - .new_hid_io(controller, false); - if let Ok(hid_io) = hid_io { - if let Err(err) = keyboard_handler.reset(hid_io.as_ref(), extended_verification.into()) { - status = err; - } else { - status = efi::Status::SUCCESS; - } - } - } - } - } + let status = 'reset_processing: { + let Some(keyboard_handler) = (unsafe { context.keyboard_handler.as_mut() }) else { + break 'reset_processing efi::Status::DEVICE_ERROR; + }; + let Some(controller) = keyboard_handler.controller() else { + break 'reset_processing efi::Status::DEVICE_ERROR; + }; + UefiHidIoFactory::new(context.boot_services, keyboard_handler.agent()) + .new_hid_io(controller, false) + .and_then(|hid_io| keyboard_handler.reset(hid_io.as_ref(), extended_verification.into())) + .err() + .unwrap_or(efi::Status::SUCCESS) + }; context.boot_services.restore_tpl(old_tpl); status } @@ -235,26 +230,24 @@ impl SimpleTextInExFfi { return efi::Status::INVALID_PARAMETER; } let context = unsafe { (this as *mut SimpleTextInExFfi).as_mut() }.expect("bad pointer"); - let mut status = efi::Status::SUCCESS; let old_tpl = context.boot_services.raise_tpl(efi::TPL_NOTIFY); - 'read_key_stroke: { + let status = 'read_key_stroke: { let keyboard_handler = unsafe { context.keyboard_handler.as_mut() }; - if let Some(keyboard_handler) = keyboard_handler { - if let Some(key) = keyboard_handler.pop_key() { - unsafe { key_data.write(key) } - } else { - let key = protocols::simple_text_input_ex::KeyData { - key_state: keyboard_handler.get_key_state(), - ..Default::default() - }; - unsafe { key_data.write(key) }; - status = efi::Status::NOT_READY - } + let Some(keyboard_handler) = keyboard_handler else { + break 'read_key_stroke efi::Status::DEVICE_ERROR; + }; + if let Some(key) = keyboard_handler.pop_key() { + unsafe { ptr::write(key_data, key) } + efi::Status::SUCCESS } else { - status = efi::Status::DEVICE_ERROR; - break 'read_key_stroke; + let key = protocols::simple_text_input_ex::KeyData { + key_state: keyboard_handler.get_key_state(), + ..Default::default() + }; + unsafe { ptr::write(key_data, key) }; + efi::Status::NOT_READY } - } + }; context.boot_services.restore_tpl(old_tpl); status } @@ -268,24 +261,22 @@ impl SimpleTextInExFfi { return efi::Status::INVALID_PARAMETER; } let context = unsafe { (this as *mut SimpleTextInExFfi).as_mut() }.expect("bad pointer"); - let mut status = efi::Status::DEVICE_ERROR; let old_tpl = context.boot_services.raise_tpl(efi::TPL_NOTIFY); - '_set_state_processing: { - if let Some(keyboard_handler) = unsafe { context.keyboard_handler.as_mut() } { - if let Some(controller) = keyboard_handler.get_controller() { - let hid_io = UefiHidIoFactory::new(context.boot_services, keyboard_handler.get_agent()) - .new_hid_io(controller, false); - if let Ok(hid_io) = hid_io { - status = efi::Status::SUCCESS; - keyboard_handler.set_key_toggle_state(unsafe { key_toggle_state.read() }); - let result = keyboard_handler.update_leds(hid_io.as_ref()); - if let Err(result) = result { - status = result; - } - } - } - } - } + let status = 'set_state_processing: { + let Some(keyboard_handler) = (unsafe { context.keyboard_handler.as_mut() }) else { + break 'set_state_processing efi::Status::DEVICE_ERROR; + }; + let Some(controller) = keyboard_handler.controller() else { + break 'set_state_processing efi::Status::DEVICE_ERROR; + }; + let Ok(hid_io) = + UefiHidIoFactory::new(context.boot_services, keyboard_handler.agent()).new_hid_io(controller, false) + else { + break 'set_state_processing efi::Status::DEVICE_ERROR; + }; + keyboard_handler.set_key_toggle_state(unsafe { key_toggle_state.read() }); + keyboard_handler.update_leds(hid_io.as_ref()).err().unwrap_or(efi::Status::SUCCESS) + }; context.boot_services.restore_tpl(old_tpl); status } @@ -307,17 +298,16 @@ impl SimpleTextInExFfi { let context = unsafe { (this as *mut SimpleTextInExFfi).as_mut() }.expect("bad pointer"); let old_tpl = context.boot_services.raise_tpl(efi::TPL_NOTIFY); - let status; - { + let status = { if let Some(keyboard_handler) = unsafe { context.keyboard_handler.as_mut() } { let key_data = unsafe { key_data_ptr.read() }; let handle = keyboard_handler.insert_key_notify_callback(key_data, key_notification_function); unsafe { notify_handle.write(handle as *mut c_void) }; - status = efi::Status::SUCCESS; + efi::Status::SUCCESS } else { - status = efi::Status::DEVICE_ERROR; + efi::Status::DEVICE_ERROR } - } + }; context.boot_services.restore_tpl(old_tpl); status } @@ -330,18 +320,16 @@ impl SimpleTextInExFfi { if this.is_null() { return efi::Status::INVALID_PARAMETER; } - let status; let context = unsafe { (this as *mut SimpleTextInExFfi).as_mut() }.expect("bad pointer"); let old_tpl = context.boot_services.raise_tpl(efi::TPL_NOTIFY); - if let Some(keyboard_handler) = unsafe { context.keyboard_handler.as_mut() } { - if let Err(err) = keyboard_handler.remove_key_notify_callback(notification_handle as usize) { - status = err; - } else { - status = efi::Status::SUCCESS; - } + let status = if let Some(keyboard_handler) = unsafe { context.keyboard_handler.as_mut() } { + keyboard_handler + .remove_key_notify_callback(notification_handle as usize) + .err() + .unwrap_or(efi::Status::SUCCESS) } else { - status = efi::Status::DEVICE_ERROR; - } + efi::Status::DEVICE_ERROR + }; context.boot_services.restore_tpl(old_tpl); status } @@ -355,18 +343,16 @@ impl SimpleTextInExFfi { let context = unsafe { (context as *mut SimpleTextInExFfi).as_mut() }.expect("bad pointer"); let old_tpl = context.boot_services.raise_tpl(efi::TPL_NOTIFY); - { - if let Some(keyboard_handler) = unsafe { context.keyboard_handler.as_mut() } { - while let Some(key_data) = keyboard_handler.peek_key() { - if key_data.key.unicode_char == 0 && key_data.key.scan_code == 0 { - // consume (and ignore) the partial stroke. - let _ = keyboard_handler.pop_key(); - continue; - } else { - // valid keystroke - context.boot_services.signal_event(event); - break; - } + if let Some(keyboard_handler) = unsafe { context.keyboard_handler.as_mut() } { + while let Some(key_data) = keyboard_handler.peek_key() { + if key_data.key.unicode_char == 0 && key_data.key.scan_code == 0 { + // consume (and ignore) the partial stroke. + let _ = keyboard_handler.pop_key(); + continue; + } else { + // valid keystroke + context.boot_services.signal_event(event); + break; } } } @@ -376,29 +362,30 @@ impl SimpleTextInExFfi { // Event callback function for handling registered key notifications. Iterates over the queue of keys to be notified, // and invokes the registered callback function for each of those keys. extern "efiapi" fn process_key_notifies(_event: efi::Event, context: *mut c_void) { - if let Some(context) = unsafe { (context as *mut SimpleTextInExFfi).as_mut() } { - loop { - let mut pending_key = None; - let mut pending_callbacks = Vec::new(); - - let old_tpl = context.boot_services.raise_tpl(efi::TPL_NOTIFY); - if let Some(keyboard_handler) = unsafe { context.keyboard_handler.as_mut() } { - (pending_key, pending_callbacks) = keyboard_handler.get_pending_callbacks(); - } else { - debugln!(DEBUG_ERROR, "process_key_notifies event called without a valid keyboard_handler"); - } - context.boot_services.restore_tpl(old_tpl); + let Some(context) = (unsafe { (context as *mut SimpleTextInExFfi).as_mut() }) else { + return; + }; + loop { + let mut pending_key = None; + let mut pending_callbacks = Vec::new(); - //dispatch notifies (if any) at the TPL this event callback was invoked at. - if let Some(mut pending_key) = pending_key { - let key_ptr = &mut pending_key as *mut protocols::simple_text_input_ex::KeyData; - for callback in pending_callbacks { - let _ = callback(key_ptr); - } - } else { - // no pending notifies to process - break; + let old_tpl = context.boot_services.raise_tpl(efi::TPL_NOTIFY); + if let Some(keyboard_handler) = unsafe { context.keyboard_handler.as_mut() } { + (pending_key, pending_callbacks) = keyboard_handler.pending_callbacks(); + } else { + debugln!(DEBUG_ERROR, "process_key_notifies event called without a valid keyboard_handler"); + } + context.boot_services.restore_tpl(old_tpl); + + //dispatch notifies (if any) at the TPL this event callback was invoked at. + if let Some(mut pending_key) = pending_key { + let key_ptr = &mut pending_key as *mut protocols::simple_text_input_ex::KeyData; + for callback in pending_callbacks { + let _ = callback(key_ptr); } + } else { + // no pending notifies to process + break; } } } @@ -409,7 +396,8 @@ mod test { use core::{ ffi::c_void, mem::MaybeUninit, - sync::atomic::{AtomicBool, AtomicPtr}, + ptr, + sync::atomic::{AtomicBool, AtomicPtr, Ordering}, }; use r_efi::{efi, protocols}; @@ -481,13 +469,13 @@ mod test { #[test] fn uninstall_should_uninstall_simple_text_in_interface() { - static CONTEXT_PTR: AtomicPtr = AtomicPtr::new(core::ptr::null_mut()); + static CONTEXT_PTR: AtomicPtr = AtomicPtr::new(ptr::null_mut()); let boot_services = create_fake_static_boot_service(); // used in install boot_services.expect_create_event().returning(|_, _, _, _, _| efi::Status::SUCCESS); boot_services.expect_install_protocol_interface().returning(|_, _, _, interface| { - CONTEXT_PTR.store(interface, core::sync::atomic::Ordering::SeqCst); + CONTEXT_PTR.store(interface, Ordering::SeqCst); efi::Status::SUCCESS }); @@ -495,14 +483,14 @@ mod test { boot_services.expect_open_protocol().returning(|_, protocol, interface, _, _, _| { unsafe { assert_eq!(protocol.read(), protocols::simple_text_input_ex::PROTOCOL_GUID); - interface.write(CONTEXT_PTR.load(core::sync::atomic::Ordering::SeqCst)); + interface.write(CONTEXT_PTR.load(Ordering::SeqCst)); } efi::Status::SUCCESS }); boot_services.expect_uninstall_protocol_interface().returning(|_, protocol, interface| { unsafe { assert_eq!(protocol.read(), protocols::simple_text_input_ex::PROTOCOL_GUID); - assert_eq!(interface, CONTEXT_PTR.load(core::sync::atomic::Ordering::SeqCst)); + assert_eq!(interface, CONTEXT_PTR.load(Ordering::SeqCst)); } efi::Status::SUCCESS }); @@ -511,20 +499,20 @@ mod test { let mut keyboard_handler = KeyboardHidHandler::new(boot_services, 1 as efi::Handle); SimpleTextInExFfi::install(boot_services, 2 as efi::Handle, &mut keyboard_handler).unwrap(); - assert_ne!(CONTEXT_PTR.load(core::sync::atomic::Ordering::SeqCst), core::ptr::null_mut()); + assert_ne!(CONTEXT_PTR.load(Ordering::SeqCst), ptr::null_mut()); SimpleTextInExFfi::uninstall(boot_services, 1 as efi::Handle, 2 as efi::Handle).unwrap(); } #[test] fn reset_should_invoke_reset() { - static CONTEXT_PTR: AtomicPtr = AtomicPtr::new(core::ptr::null_mut()); + static CONTEXT_PTR: AtomicPtr = AtomicPtr::new(ptr::null_mut()); let boot_services = create_fake_static_boot_service(); // used in install boot_services.expect_create_event().returning(|_, _, _, _, _| efi::Status::SUCCESS); boot_services.expect_install_protocol_interface().returning(|_, _, _, interface| { - CONTEXT_PTR.store(interface, core::sync::atomic::Ordering::SeqCst); + CONTEXT_PTR.store(interface, Ordering::SeqCst); efi::Status::SUCCESS }); @@ -564,7 +552,7 @@ mod test { boot_services.expect_uninstall_protocol_interface().returning(|_, protocol, interface| { unsafe { assert_eq!(protocol.read(), protocols::simple_text_input_ex::PROTOCOL_GUID); - assert_eq!(interface, CONTEXT_PTR.load(core::sync::atomic::Ordering::SeqCst)); + assert_eq!(interface, CONTEXT_PTR.load(Ordering::SeqCst)); } efi::Status::SUCCESS }); @@ -574,10 +562,9 @@ mod test { keyboard_handler.set_controller(Some(2 as efi::Handle)); SimpleTextInExFfi::install(boot_services, 2 as efi::Handle, &mut keyboard_handler).unwrap(); - assert_ne!(CONTEXT_PTR.load(core::sync::atomic::Ordering::SeqCst), core::ptr::null_mut()); + assert_ne!(CONTEXT_PTR.load(Ordering::SeqCst), ptr::null_mut()); - let this = - CONTEXT_PTR.load(core::sync::atomic::Ordering::SeqCst) as *mut protocols::simple_text_input_ex::Protocol; + let this = CONTEXT_PTR.load(Ordering::SeqCst) as *mut protocols::simple_text_input_ex::Protocol; SimpleTextInExFfi::simple_text_in_ex_reset(this, efi::Boolean::FALSE); // avoid keyboard drop uninstall flows @@ -586,13 +573,13 @@ mod test { #[test] fn read_key_stroke_should_read_keystrokes() { - static CONTEXT_PTR: AtomicPtr = AtomicPtr::new(core::ptr::null_mut()); + static CONTEXT_PTR: AtomicPtr = AtomicPtr::new(ptr::null_mut()); let boot_services = create_fake_static_boot_service(); // used in install boot_services.expect_create_event().returning(|_, _, _, _, _| efi::Status::SUCCESS); boot_services.expect_install_protocol_interface().returning(|_, _, _, interface| { - CONTEXT_PTR.store(interface, core::sync::atomic::Ordering::SeqCst); + CONTEXT_PTR.store(interface, Ordering::SeqCst); efi::Status::SUCCESS }); @@ -617,7 +604,7 @@ mod test { keyboard_handler.initialize(2 as efi::Handle, &hid_io).unwrap(); SimpleTextInExFfi::install(boot_services, 2 as efi::Handle, &mut keyboard_handler).unwrap(); - assert_ne!(CONTEXT_PTR.load(core::sync::atomic::Ordering::SeqCst), core::ptr::null_mut()); + assert_ne!(CONTEXT_PTR.load(Ordering::SeqCst), ptr::null_mut()); let report: &[u8] = &[0x00, 0x00, 0x04, 0x05, 0x06, 0x00, 0x00, 0x00]; keyboard_handler.receive_report(report, &hid_io); @@ -626,8 +613,7 @@ mod test { keyboard_handler.receive_report(report, &hid_io); //read with simple_text_in - let this = - CONTEXT_PTR.load(core::sync::atomic::Ordering::SeqCst) as *mut protocols::simple_text_input_ex::Protocol; + let this = CONTEXT_PTR.load(Ordering::SeqCst) as *mut protocols::simple_text_input_ex::Protocol; let mut input_key: protocols::simple_text_input_ex::KeyData = Default::default(); let status = SimpleTextInExFfi::simple_text_in_ex_read_key_stroke( @@ -740,13 +726,13 @@ mod test { #[test] fn set_state_should_set_state() { - static CONTEXT_PTR: AtomicPtr = AtomicPtr::new(core::ptr::null_mut()); + static CONTEXT_PTR: AtomicPtr = AtomicPtr::new(ptr::null_mut()); let boot_services = create_fake_static_boot_service(); // used in install boot_services.expect_create_event().returning(|_, _, _, _, _| efi::Status::SUCCESS); boot_services.expect_install_protocol_interface().returning(|_, _, _, interface| { - CONTEXT_PTR.store(interface, core::sync::atomic::Ordering::SeqCst); + CONTEXT_PTR.store(interface, Ordering::SeqCst); efi::Status::SUCCESS }); boot_services.expect_locate_protocol().returning(|_, _, _| efi::Status::NOT_FOUND); @@ -797,14 +783,13 @@ mod test { keyboard_handler.initialize(2 as efi::Handle, &hid_io).unwrap(); SimpleTextInExFfi::install(boot_services, 2 as efi::Handle, &mut keyboard_handler).unwrap(); - assert_ne!(CONTEXT_PTR.load(core::sync::atomic::Ordering::SeqCst), core::ptr::null_mut()); + assert_ne!(CONTEXT_PTR.load(Ordering::SeqCst), ptr::null_mut()); - let this = - CONTEXT_PTR.load(core::sync::atomic::Ordering::SeqCst) as *mut protocols::simple_text_input_ex::Protocol; + let this = CONTEXT_PTR.load(Ordering::SeqCst) as *mut protocols::simple_text_input_ex::Protocol; let mut key_toggle_state: protocols::simple_text_input_ex::KeyToggleState = protocols::simple_text_input_ex::KEY_STATE_EXPOSED; - let status = SimpleTextInExFfi::simple_text_in_ex_set_state(this, core::ptr::addr_of_mut!(key_toggle_state)); + let status = SimpleTextInExFfi::simple_text_in_ex_set_state(this, ptr::addr_of_mut!(key_toggle_state)); assert_eq!(status, efi::Status::SUCCESS); assert_eq!( keyboard_handler.get_key_state().key_toggle_state, @@ -813,7 +798,7 @@ mod test { key_toggle_state = protocols::simple_text_input_ex::CAPS_LOCK_ACTIVE | protocols::simple_text_input_ex::NUM_LOCK_ACTIVE; - let status = SimpleTextInExFfi::simple_text_in_ex_set_state(this, core::ptr::addr_of_mut!(key_toggle_state)); + let status = SimpleTextInExFfi::simple_text_in_ex_set_state(this, ptr::addr_of_mut!(key_toggle_state)); assert_eq!(status, efi::Status::SUCCESS); assert_eq!( keyboard_handler.get_key_state().key_toggle_state, @@ -824,7 +809,7 @@ mod test { #[test] fn register_key_notify_should_register_key() { const NOTIFY_EVENT: efi::Event = 1 as efi::Event; - static CONTEXT_PTR: AtomicPtr = AtomicPtr::new(core::ptr::null_mut()); + static CONTEXT_PTR: AtomicPtr = AtomicPtr::new(ptr::null_mut()); static KEY_NOTIFIED: AtomicBool = AtomicBool::new(false); static KEY2_NOTIFIED: AtomicBool = AtomicBool::new(false); @@ -833,7 +818,7 @@ mod test { // used in install boot_services.expect_create_event().returning(|_, _, _, _, _| efi::Status::SUCCESS); boot_services.expect_install_protocol_interface().returning(|_, _, _, interface| { - CONTEXT_PTR.store(interface, core::sync::atomic::Ordering::SeqCst); + CONTEXT_PTR.store(interface, Ordering::SeqCst); efi::Status::SUCCESS }); boot_services.expect_locate_protocol().returning(|_, _, _| efi::Status::NOT_FOUND); @@ -846,7 +831,7 @@ mod test { boot_services.expect_create_event_ex().returning(|_, _, _, _, _, _| efi::Status::SUCCESS); boot_services.expect_signal_event().returning(|event| { if event == NOTIFY_EVENT { - SimpleTextInExFfi::process_key_notifies(event, CONTEXT_PTR.load(core::sync::atomic::Ordering::SeqCst)); + SimpleTextInExFfi::process_key_notifies(event, CONTEXT_PTR.load(Ordering::SeqCst)); } efi::Status::SUCCESS }); @@ -864,14 +849,14 @@ mod test { keyboard_handler.set_notify_event(NOTIFY_EVENT); SimpleTextInExFfi::install(boot_services, 2 as efi::Handle, &mut keyboard_handler).unwrap(); - assert_ne!(CONTEXT_PTR.load(core::sync::atomic::Ordering::SeqCst), core::ptr::null_mut()); + assert_ne!(CONTEXT_PTR.load(Ordering::SeqCst), ptr::null_mut()); extern "efiapi" fn key_notify_callback_a( key_data: *mut protocols::simple_text_input_ex::KeyData, ) -> efi::Status { let key = unsafe { key_data.read() }; assert_eq!(key.key.unicode_char, 'a' as u16); - KEY_NOTIFIED.store(true, core::sync::atomic::Ordering::SeqCst); + KEY_NOTIFIED.store(true, Ordering::SeqCst); efi::Status::SUCCESS } @@ -880,43 +865,42 @@ mod test { ) -> efi::Status { let key = unsafe { key_data.read() }; assert!((key.key.unicode_char == 'a' as u16) || (key.key.unicode_char == 'b' as u16)); - KEY2_NOTIFIED.store(true, core::sync::atomic::Ordering::SeqCst); + KEY2_NOTIFIED.store(true, Ordering::SeqCst); efi::Status::SUCCESS } - let this = - CONTEXT_PTR.load(core::sync::atomic::Ordering::SeqCst) as *mut protocols::simple_text_input_ex::Protocol; + let this = CONTEXT_PTR.load(Ordering::SeqCst) as *mut protocols::simple_text_input_ex::Protocol; let mut key_data: protocols::simple_text_input_ex::KeyData = Default::default(); key_data.key.unicode_char = 'a' as u16; - let mut notify_handle = core::ptr::null_mut(); + let mut notify_handle = ptr::null_mut(); let status = SimpleTextInExFfi::simple_text_in_ex_register_key_notify( this, - core::ptr::addr_of_mut!(key_data), + ptr::addr_of_mut!(key_data), key_notify_callback_a, - core::ptr::addr_of_mut!(notify_handle), + ptr::addr_of_mut!(notify_handle), ); assert_eq!(status, efi::Status::SUCCESS); assert_eq!(notify_handle as usize, 1); - notify_handle = core::ptr::null_mut(); + notify_handle = ptr::null_mut(); let status = SimpleTextInExFfi::simple_text_in_ex_register_key_notify( this, - core::ptr::addr_of_mut!(key_data), + ptr::addr_of_mut!(key_data), key_notify_callback_a_and_b, - core::ptr::addr_of_mut!(notify_handle), + ptr::addr_of_mut!(notify_handle), ); assert_eq!(status, efi::Status::SUCCESS); assert_eq!(notify_handle as usize, 2); - notify_handle = core::ptr::null_mut(); + notify_handle = ptr::null_mut(); key_data.key.unicode_char = 'b' as u16; let status = SimpleTextInExFfi::simple_text_in_ex_register_key_notify( this, - core::ptr::addr_of_mut!(key_data), + ptr::addr_of_mut!(key_data), key_notify_callback_a_and_b, - core::ptr::addr_of_mut!(notify_handle), + ptr::addr_of_mut!(notify_handle), ); assert_eq!(status, efi::Status::SUCCESS); assert_eq!(notify_handle as usize, 3); @@ -929,11 +913,11 @@ mod test { let report: &[u8] = &[0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]; keyboard_handler.receive_report(report, &hid_io); - assert!(!KEY_NOTIFIED.load(core::sync::atomic::Ordering::SeqCst)); - assert!(KEY2_NOTIFIED.load(core::sync::atomic::Ordering::SeqCst)); + assert!(!KEY_NOTIFIED.load(Ordering::SeqCst)); + assert!(KEY2_NOTIFIED.load(Ordering::SeqCst)); - KEY_NOTIFIED.store(false, core::sync::atomic::Ordering::SeqCst); - KEY2_NOTIFIED.store(false, core::sync::atomic::Ordering::SeqCst); + KEY_NOTIFIED.store(false, Ordering::SeqCst); + KEY2_NOTIFIED.store(false, Ordering::SeqCst); //send 'a' let report: &[u8] = &[0x00, 0x00, 0x04, 0x00, 0x00, 0x00, 0x00, 0x00]; @@ -943,11 +927,11 @@ mod test { let report: &[u8] = &[0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]; keyboard_handler.receive_report(report, &hid_io); - assert!(KEY_NOTIFIED.load(core::sync::atomic::Ordering::SeqCst)); - assert!(KEY2_NOTIFIED.load(core::sync::atomic::Ordering::SeqCst)); + assert!(KEY_NOTIFIED.load(Ordering::SeqCst)); + assert!(KEY2_NOTIFIED.load(Ordering::SeqCst)); - KEY_NOTIFIED.store(false, core::sync::atomic::Ordering::SeqCst); - KEY2_NOTIFIED.store(false, core::sync::atomic::Ordering::SeqCst); + KEY_NOTIFIED.store(false, Ordering::SeqCst); + KEY2_NOTIFIED.store(false, Ordering::SeqCst); //remove the 'a'-only callback let status = SimpleTextInExFfi::simple_text_in_ex_unregister_key_notify(this, 1 as *mut c_void); @@ -961,13 +945,13 @@ mod test { let report: &[u8] = &[0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]; keyboard_handler.receive_report(report, &hid_io); - assert!(!KEY_NOTIFIED.load(core::sync::atomic::Ordering::SeqCst)); - assert!(KEY2_NOTIFIED.load(core::sync::atomic::Ordering::SeqCst)); + assert!(!KEY_NOTIFIED.load(Ordering::SeqCst)); + assert!(KEY2_NOTIFIED.load(Ordering::SeqCst)); } #[test] fn wait_for_key_should_wait_for_key() { - static CONTEXT_PTR: AtomicPtr = AtomicPtr::new(core::ptr::null_mut()); + static CONTEXT_PTR: AtomicPtr = AtomicPtr::new(ptr::null_mut()); static RECEIVED_EVENT: AtomicBool = AtomicBool::new(false); let boot_services = create_fake_static_boot_service(); @@ -976,7 +960,7 @@ mod test { boot_services.expect_create_event().returning(|_, _, _, _, _| efi::Status::SUCCESS); boot_services.expect_install_protocol_interface().returning(|_, protocol, _, interface| { if unsafe { *protocol } == protocols::simple_text_input_ex::PROTOCOL_GUID { - CONTEXT_PTR.store(interface, core::sync::atomic::Ordering::SeqCst); + CONTEXT_PTR.store(interface, Ordering::SeqCst); } efi::Status::SUCCESS }); @@ -987,7 +971,7 @@ mod test { boot_services.expect_restore_tpl().returning(|_| ()); boot_services.expect_signal_event().returning(|_| { - RECEIVED_EVENT.store(true, core::sync::atomic::Ordering::SeqCst); + RECEIVED_EVENT.store(true, Ordering::SeqCst); efi::Status::SUCCESS }); @@ -1002,12 +986,9 @@ mod test { keyboard_handler.set_controller(Some(2 as efi::Handle)); keyboard_handler.initialize(2 as efi::Handle, &hid_io).unwrap(); - RECEIVED_EVENT.store(false, core::sync::atomic::Ordering::SeqCst); - SimpleTextInExFfi::simple_text_in_ex_wait_for_key( - 3 as efi::Event, - CONTEXT_PTR.load(core::sync::atomic::Ordering::SeqCst), - ); - assert!(!RECEIVED_EVENT.load(core::sync::atomic::Ordering::SeqCst)); + RECEIVED_EVENT.store(false, Ordering::SeqCst); + SimpleTextInExFfi::simple_text_in_ex_wait_for_key(3 as efi::Event, CONTEXT_PTR.load(Ordering::SeqCst)); + assert!(!RECEIVED_EVENT.load(Ordering::SeqCst)); // press the a key let report: &[u8] = &[0x00, 0x00, 0x04, 0x00, 0x00, 0x00, 0x00, 0x00]; @@ -1017,11 +998,8 @@ mod test { let report: &[u8] = &[0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]; keyboard_handler.receive_report(report, &hid_io); - SimpleTextInExFfi::simple_text_in_ex_wait_for_key( - 3 as efi::Event, - CONTEXT_PTR.load(core::sync::atomic::Ordering::SeqCst), - ); - assert!(RECEIVED_EVENT.load(core::sync::atomic::Ordering::SeqCst)); + SimpleTextInExFfi::simple_text_in_ex_wait_for_key(3 as efi::Event, CONTEXT_PTR.load(Ordering::SeqCst)); + assert!(RECEIVED_EVENT.load(Ordering::SeqCst)); // avoid keyboard drop uninstall flows keyboard_handler.set_controller(None); diff --git a/HidPkg/UefiHidDxeV2/src/lib.rs b/HidPkg/UefiHidDxeV2/src/lib.rs index 5dca990737..ad50e51d38 100644 --- a/HidPkg/UefiHidDxeV2/src/lib.rs +++ b/HidPkg/UefiHidDxeV2/src/lib.rs @@ -30,12 +30,14 @@ pub mod hid_io; pub mod keyboard; pub mod pointer; -use boot_services::StandardUefiBootServices; -use core::sync::atomic::AtomicPtr; +use core::{ptr, sync::atomic::AtomicPtr}; + use r_efi::efi; +use boot_services::StandardUefiBootServices; + /// Global instance of UEFI Boot Services. pub static BOOT_SERVICES: StandardUefiBootServices = StandardUefiBootServices::new(); /// Global instance of UEFI Runtime Services. -pub static RUNTIME_SERVICES: AtomicPtr = AtomicPtr::new(core::ptr::null_mut()); +pub static RUNTIME_SERVICES: AtomicPtr = AtomicPtr::new(ptr::null_mut()); diff --git a/HidPkg/UefiHidDxeV2/src/main.rs b/HidPkg/UefiHidDxeV2/src/main.rs index c373fe8791..1044933717 100644 --- a/HidPkg/UefiHidDxeV2/src/main.rs +++ b/HidPkg/UefiHidDxeV2/src/main.rs @@ -15,9 +15,8 @@ #[cfg(target_os = "uefi")] mod uefi_entry { extern crate alloc; - use core::{panic::PanicInfo, sync::atomic::Ordering}; - use alloc::{boxed::Box, vec::Vec}; + use core::{panic::PanicInfo, sync::atomic::Ordering}; use r_efi::{efi, system}; diff --git a/HidPkg/UefiHidDxeV2/src/pointer/mod.rs b/HidPkg/UefiHidDxeV2/src/pointer.rs similarity index 97% rename from HidPkg/UefiHidDxeV2/src/pointer/mod.rs rename to HidPkg/UefiHidDxeV2/src/pointer.rs index 78e7a07afd..479f7c17d3 100644 --- a/HidPkg/UefiHidDxeV2/src/pointer/mod.rs +++ b/HidPkg/UefiHidDxeV2/src/pointer.rs @@ -15,21 +15,21 @@ use alloc::{ collections::{BTreeMap, BTreeSet}, vec::Vec, }; + +use r_efi::{efi, protocols}; + use hidparser::{ report_data_types::{ReportId, Usage}, ReportDescriptor, ReportField, VariableField, }; -use r_efi::{efi, protocols}; - use rust_advanced_logger_dxe::{debugln, function, DEBUG_ERROR, DEBUG_VERBOSE}; +use self::absolute_pointer::PointerContext; use crate::{ boot_services::UefiBootServices, hid_io::{HidIo, HidReportReceiver}, }; -use self::absolute_pointer::PointerContext; - // Usages supported by this module. const GENERIC_DESKTOP_X: u32 = 0x00010030; const GENERIC_DESKTOP_Y: u32 = 0x00010031; @@ -147,7 +147,6 @@ impl PointerHidHandler { self.input_reports.insert(report_data.report_id, report_data); } } - if !self.input_reports.is_empty() { Ok(()) } else { diff --git a/HidPkg/UefiHidDxeV2/src/pointer/absolute_pointer.rs b/HidPkg/UefiHidDxeV2/src/pointer/absolute_pointer.rs index 5c00c6fb19..6b42ff72c1 100644 --- a/HidPkg/UefiHidDxeV2/src/pointer/absolute_pointer.rs +++ b/HidPkg/UefiHidDxeV2/src/pointer/absolute_pointer.rs @@ -8,15 +8,15 @@ //! SPDX-License-Identifier: BSD-2-Clause-Patent //! use alloc::boxed::Box; -use core::ffi::c_void; +use core::{ffi::c_void, ptr}; -use hidparser::report_data_types::Usage; use r_efi::{efi, protocols}; -use rust_advanced_logger_dxe::{debugln, DEBUG_ERROR, DEBUG_INFO, DEBUG_WARN}; -use crate::boot_services::UefiBootServices; +use hidparser::report_data_types::Usage; +use rust_advanced_logger_dxe::{debugln, DEBUG_ERROR, DEBUG_INFO, DEBUG_WARN}; use super::{PointerHidHandler, BUTTON_MAX, BUTTON_MIN, DIGITIZER_SWITCH_MAX, DIGITIZER_SWITCH_MIN}; +use crate::boot_services::UefiBootServices; // FFI context // Safety: a pointer to PointerHidHandler is included in the context so that it can be reclaimed in the absolute_pointer @@ -57,7 +57,7 @@ impl PointerContext { reset: Self::absolute_pointer_reset, get_state: Self::absolute_pointer_get_state, mode: Box::into_raw(Box::new(Self::initialize_mode(pointer_handler))), - wait_for_input: core::ptr::null_mut(), + wait_for_input: ptr::null_mut(), }, boot_services, pointer_handler: pointer_handler as *mut PointerHidHandler, @@ -66,13 +66,13 @@ impl PointerContext { let absolute_pointer_ptr = Box::into_raw(Box::new(pointer_ctx)); // create event for wait_for_input. - let mut wait_for_pointer_input_event: efi::Event = core::ptr::null_mut(); + let mut wait_for_pointer_input_event: efi::Event = ptr::null_mut(); let status = boot_services.create_event( efi::EVT_NOTIFY_WAIT, efi::TPL_NOTIFY, Some(Self::wait_for_pointer), absolute_pointer_ptr as *mut c_void, - core::ptr::addr_of_mut!(wait_for_pointer_input_event), + ptr::addr_of_mut!(wait_for_pointer_input_event), ); if status.is_error() { drop(unsafe { Box::from_raw(absolute_pointer_ptr) }); @@ -84,7 +84,7 @@ impl PointerContext { // install the absolute_pointer protocol. let mut controller = controller; let status = boot_services.install_protocol_interface( - core::ptr::addr_of_mut!(controller), + ptr::addr_of_mut!(controller), &protocols::absolute_pointer::PROTOCOL_GUID as *const efi::Guid as *mut efi::Guid, efi::NATIVE_INTERFACE, absolute_pointer_ptr as *mut c_void, @@ -148,11 +148,11 @@ impl PointerContext { agent: efi::Handle, controller: efi::Handle, ) -> Result<(), efi::Status> { - let mut absolute_pointer_ptr: *mut PointerContext = core::ptr::null_mut(); + let mut absolute_pointer_ptr: *mut PointerContext = ptr::null_mut(); let status = boot_services.open_protocol( controller, &protocols::absolute_pointer::PROTOCOL_GUID as *const efi::Guid as *mut efi::Guid, - core::ptr::addr_of_mut!(absolute_pointer_ptr) as *mut *mut c_void, + ptr::addr_of_mut!(absolute_pointer_ptr) as *mut *mut c_void, agent, controller, efi::OPEN_PROTOCOL_GET_PROTOCOL, @@ -177,7 +177,7 @@ impl PointerContext { debugln!(DEBUG_ERROR, "Failed to uninstall absolute_pointer interface, status: {:x?}", status); unsafe { - (*absolute_pointer_ptr).pointer_handler = core::ptr::null_mut(); + (*absolute_pointer_ptr).pointer_handler = ptr::null_mut(); } //return without tearing down the context. return Err(status); @@ -193,7 +193,7 @@ impl PointerContext { //and return error based on observing pointer_handler is null. debugln!(DEBUG_ERROR, "Failed to close absolute_pointer.wait_for_input event, status: {:x?}", status); unsafe { - (*absolute_pointer_ptr).pointer_handler = core::ptr::null_mut(); + (*absolute_pointer_ptr).pointer_handler = ptr::null_mut(); } return Err(status); } @@ -290,18 +290,14 @@ impl PointerContext { #[cfg(test)] mod test { - - use core::ffi::c_void; - - use r_efi::{efi, protocols}; - use super::*; - use crate::{ boot_services::MockUefiBootServices, hid_io::{HidReportReceiver, MockHidIo}, pointer::CENTER, }; + use core::ffi::c_void; + use r_efi::{efi, protocols}; static MOUSE_REPORT_DESCRIPTOR: &[u8] = &[ 0x05, 0x01, // USAGE_PAGE (Generic Desktop) @@ -349,14 +345,14 @@ mod test { const CONTROLLER_HANDLE: efi::Handle = 0x02 as efi::Handle; const POINTER_EVENT: efi::Event = 0x03 as efi::Event; - static mut ABS_PTR_INTERFACE: *mut c_void = core::ptr::null_mut(); - static mut EVENT_CONTEXT: *mut c_void = core::ptr::null_mut(); + static mut ABS_PTR_INTERFACE: *mut c_void = ptr::null_mut(); + static mut EVENT_CONTEXT: *mut c_void = ptr::null_mut(); static mut EVENT_SIGNALED: bool = false; // expected on PointerHidHandler::initialize(). boot_services.expect_create_event().returning(|_, _, wait_for_ptr, context, event_ptr| { assert!(wait_for_ptr == Some(PointerContext::wait_for_pointer)); - assert_ne!(context, core::ptr::null_mut()); + assert_ne!(context, ptr::null_mut()); unsafe { EVENT_CONTEXT = context; event_ptr.write(POINTER_EVENT); @@ -428,13 +424,13 @@ mod test { const CONTROLLER_HANDLE: efi::Handle = 0x02 as efi::Handle; const EVENT_HANDLE: efi::Handle = 0x03 as efi::Handle; - static mut ABS_PTR_INTERFACE: *mut c_void = core::ptr::null_mut(); - static mut EVENT_CONTEXT: *mut c_void = core::ptr::null_mut(); + static mut ABS_PTR_INTERFACE: *mut c_void = ptr::null_mut(); + static mut EVENT_CONTEXT: *mut c_void = ptr::null_mut(); // expected on PointerHidHandler::initialize(). boot_services.expect_create_event().returning(|_, _, wait_for_ptr, context, event_ptr| { assert!(wait_for_ptr == Some(PointerContext::wait_for_pointer)); - assert_ne!(context, core::ptr::null_mut()); + assert_ne!(context, ptr::null_mut()); unsafe { EVENT_CONTEXT = context; event_ptr.write(EVENT_HANDLE); @@ -513,13 +509,13 @@ mod test { const CONTROLLER_HANDLE: efi::Handle = 0x02 as efi::Handle; const EVENT_HANDLE: efi::Handle = 0x03 as efi::Handle; - static mut ABS_PTR_INTERFACE: *mut c_void = core::ptr::null_mut(); - static mut EVENT_CONTEXT: *mut c_void = core::ptr::null_mut(); + static mut ABS_PTR_INTERFACE: *mut c_void = ptr::null_mut(); + static mut EVENT_CONTEXT: *mut c_void = ptr::null_mut(); // expected on PointerHidHandler::initialize(). boot_services.expect_create_event().returning(|_, _, wait_for_ptr, context, event_ptr| { assert!(wait_for_ptr == Some(PointerContext::wait_for_pointer)); - assert_ne!(context, core::ptr::null_mut()); + assert_ne!(context, ptr::null_mut()); unsafe { EVENT_CONTEXT = context; event_ptr.write(EVENT_HANDLE); From c052f426b79192cf3fded227af306a0ddf88f5c9 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Wed, 31 Jul 2024 08:24:14 -0700 Subject: [PATCH 02/16] pip: bump regex from 2024.5.15 to 2024.7.24 (#534) Bumps [regex](https://github.com/mrabarnett/mrab-regex) from 2024.5.15 to 2024.7.24. Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- pip-requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pip-requirements.txt b/pip-requirements.txt index 54057b4847..b89d9d4efb 100644 --- a/pip-requirements.txt +++ b/pip-requirements.txt @@ -15,5 +15,5 @@ edk2-pytool-library==0.21.8 edk2-pytool-extensions==0.27.10 antlr4-python3-runtime==4.13.1 -regex==2024.5.15 +regex==2024.7.24 pygount==1.8.0 From 6385ad045f017c305a38ba0a41a30fd181d8d3fe Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Wed, 31 Jul 2024 10:55:25 -0700 Subject: [PATCH 03/16] pip: bump edk2-pytool-library from 0.21.8 to 0.21.9 (#535) Bumps [edk2-pytool-library](https://github.com/tianocore/edk2-pytool-library) from 0.21.8 to 0.21.9. Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- pip-requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pip-requirements.txt b/pip-requirements.txt index b89d9d4efb..7f370736ca 100644 --- a/pip-requirements.txt +++ b/pip-requirements.txt @@ -12,7 +12,7 @@ # https://www.python.org/dev/peps/pep-0440/#version-specifiers ## -edk2-pytool-library==0.21.8 +edk2-pytool-library==0.21.9 edk2-pytool-extensions==0.27.10 antlr4-python3-runtime==4.13.1 regex==2024.7.24 From f0c8e103815e12d6f9fade58ec0ee9467be0df55 Mon Sep 17 00:00:00 2001 From: liqiqiii <132628835+liqiqiii@users.noreply.github.com> Date: Fri, 2 Aug 2024 00:47:03 -0700 Subject: [PATCH 04/16] Add debug level prefix for advanced logger memory message entries - Driver implementation (#525) ## Description Add debug level prefix for advanced logger memory message entries - Driver implementation This change added the existing metadata - debug level into the final advanced logger memory entries through AdvancedFileLogger. This is a followup PR for https://github.com/microsoft/mu_plus/commit/243bd187e4aa6ac68a3eb323d07554576d39ad47. After this PR checked in, we can easily track the DEBUG_ERRORs through advanced logger files on UEFI that included this driver. Added an extra space in the decodeuefilog.py to improve readability of the log. For the for loop in the code, ran with perf_trace enabled, and influence is very low and can be ignored. Updated the prefix of python script to match the design here. Use [ERROR] instead of [DEBUG_ERROR], which could save overall log file sizes and memory buffer usage. - [x] Impacts functionality? - **Functionality** - Does the change ultimately impact how firmware functions? - Examples: Add a new library, publish a new PPI, update an algorithm, ... - [ ] Impacts security? - **Security** - Does the change have a direct security impact on an application, flow, or firmware? - Examples: Crypto algorithm change, buffer overflow fix, parameter validation improvement, ... - [ ] Breaking change? - **Breaking change** - Will anyone consuming this change experience a break in build or boot behavior? - Examples: Add a new library class, move a module to a different repo, call a function in a new library class in a pre-existing module, ... - [ ] Includes tests? - **Tests** - Does the change include any explicit test code? - Examples: Unit tests, integration tests, robot tests, ... - [ ] Includes documentation? - **Documentation** - Does the change contain explicit documentation additions outside direct code modifications (and comments)? - Examples: Update readme file, add feature readme file, link to documentation on an a separate Web page, ... ## How This Was Tested Tested with two platforms that uses AdvancedFileLogger and can see debug level prefixes in the logs in the EFI Partition and USB UefiLogs folder. [MM_CORE] [ERROR] Image - MmSupervisorCore.pdb ## Integration Instructions N/A --- AdvLoggerPkg/AdvLoggerPkg.ci.yaml | 3 +- .../DecodeUefiLog/DecodeUefiLog.py | 54 ++++--- .../AdvancedLoggerAccessLib.c | 150 ++++++++++++++---- 3 files changed, 152 insertions(+), 55 deletions(-) diff --git a/AdvLoggerPkg/AdvLoggerPkg.ci.yaml b/AdvLoggerPkg/AdvLoggerPkg.ci.yaml index 644530283c..0ab8fac76b 100644 --- a/AdvLoggerPkg/AdvLoggerPkg.ci.yaml +++ b/AdvLoggerPkg/AdvLoggerPkg.ci.yaml @@ -88,7 +88,8 @@ LOGTELEMETRY, DEBUGAGENT, POSTMEM, - MMARM + MMARM, + BLKIO ] }, diff --git a/AdvLoggerPkg/Application/DecodeUefiLog/DecodeUefiLog.py b/AdvLoggerPkg/Application/DecodeUefiLog/DecodeUefiLog.py index 10e0492cdf..cdc5e7d6f3 100644 --- a/AdvLoggerPkg/Application/DecodeUefiLog/DecodeUefiLog.py +++ b/AdvLoggerPkg/Application/DecodeUefiLog/DecodeUefiLog.py @@ -260,9 +260,9 @@ class AdvLogParser (): ADVANCED_LOGGER_PHASE_SMM = 9 ADVANCED_LOGGER_PHASE_TFA = 10 ADVANCED_LOGGER_PHASE_CNT = 11 - PHASE_STRING_LIST = ["[UNSPECIFIED] ", "[SEC] ", "[PEI] ", "[PEI64] ", - "[DXE] ", "[RUNTIME] ", "[MM_CORE] ", "[MM] ", - "[SMM_CORE] ", "[SMM] ", "[TFA] "] + PHASE_STRING_LIST = ["[UNSPECIFIED]", "[SEC]", "[PEI]", "[PEI64]", + "[DXE]", "[RUNTIME]", "[MM_CORE]", "[MM]", + "[SMM_CORE]", "[SMM]", "[TFA]"] # Debug levels from MU_BASECORE\MdePkg\Include\Library\DebugLib.h # // @@ -293,27 +293,27 @@ class AdvLogParser (): # #define DEBUG_ERROR 0x80000000 // Error debug_levels_dict = { - 0x00000001: "[DEBUG_INIT]", - 0x00000002: "[DEBUG_WARN]", - 0x00000004: "[DEBUG_LOAD]", - 0x00000008: "[DEBUG_FS]", - 0x00000010: "[DEBUG_POOL]", - 0x00000020: "[DEBUG_PAGE]", - 0x00000040: "[DEBUG_INFO]", - 0x00000080: "[DEBUG_DISPATCH]", - 0x00000100: "[DEBUG_VARIABLE]", - 0x00000200: "[DEBUG_SMI]", - 0x00000400: "[DEBUG_BM]", - 0x00001000: "[DEBUG_BLKIO]", - 0x00004000: "[DEBUG_NET]", - 0x00010000: "[DEBUG_UNDI]", - 0x00020000: "[DEBUG_LOADFILE]", - 0x00080000: "[DEBUG_EVENT]", - 0x00100000: "[DEBUG_GCD]", - 0x00200000: "[DEBUG_CACHE]", - 0x00400000: "[DEBUG_VERBOSE]", - 0x00800000: "[DEBUG_MANAGEABILITY]", - 0x80000000: "[DEBUG_ERROR]" + 0x00000001: "[INIT]", + 0x00000002: "[WARN]", + 0x00000004: "[LOAD]", + 0x00000008: "[FS]", + 0x00000010: "[POOL]", + 0x00000020: "[PAGE]", + 0x00000040: "[INFO]", + 0x00000080: "[DISPATCH]", + 0x00000100: "[VARIABLE]", + 0x00000200: "[SMI]", + 0x00000400: "[BM]", + 0x00001000: "[BLKIO]", + 0x00004000: "[NET]", + 0x00010000: "[UNDI]", + 0x00020000: "[LOADFILE]", + 0x00080000: "[EVENT]", + 0x00100000: "[GCD]", + 0x00200000: "[CACHE]", + 0x00400000: "[VERBOSE]", + 0x00800000: "[MANAGEABILITY]", + 0x80000000: "[ERROR]" } # @@ -713,7 +713,8 @@ def _GetPhaseString(self, Phase): elif Phase <= self.ADVANCED_LOGGER_PHASE_UNSPECIFIED: PhaseString = "" else: - PhaseString = self.PHASE_STRING_LIST[Phase] + # Add an extra space for readability + PhaseString = self.PHASE_STRING_LIST[Phase] + ' ' return PhaseString # @@ -721,7 +722,8 @@ def _GetPhaseString(self, Phase): # def _GetDebugLevelString(self, DebugLevel): if DebugLevel in list(self.debug_levels_dict.keys()): - DebugLevelString = self.debug_levels_dict[DebugLevel] + # Add an extra space for readability + DebugLevelString = self.debug_levels_dict[DebugLevel] + ' ' else: DebugLevelString = "" return DebugLevelString diff --git a/AdvLoggerPkg/Library/AdvancedLoggerAccessLib/AdvancedLoggerAccessLib.c b/AdvLoggerPkg/Library/AdvancedLoggerAccessLib/AdvancedLoggerAccessLib.c index 893e32256b..b4845011d9 100644 --- a/AdvLoggerPkg/Library/AdvancedLoggerAccessLib/AdvancedLoggerAccessLib.c +++ b/AdvLoggerPkg/Library/AdvancedLoggerAccessLib/AdvancedLoggerAccessLib.c @@ -27,23 +27,55 @@ STATIC EFI_PHYSICAL_ADDRESS mLowAddress = 0; STATIC EFI_PHYSICAL_ADDRESS mHighAddress = 0; STATIC UINT16 mMaxMessageSize = ADVANCED_LOGGER_MAX_MESSAGE_SIZE; CONST CHAR8 *AdvMsgEntryPrefix[ADVANCED_LOGGER_PHASE_CNT] = { - "[UNSPECIFIED] ", - "[SEC] ", - "[PEI] ", - "[PEI64] ", - "[DXE] ", - "[RUNTIME] ", - "[MM_CORE] ", - "[MM] ", - "[SMM_CORE] ", - "[SMM] ", - "[TFA] ", + "[UNSPECIFIED]", + "[SEC]", + "[PEI]", + "[PEI64]", + "[DXE]", + "[RUNTIME]", + "[MM_CORE]", + "[MM]", + "[SMM_CORE]", + "[SMM]", + "[TFA]", }; -#define ADV_TIME_STAMP_FORMAT "%2.2d:%2.2d:%2.2d.%3.3d : " -#define ADV_TIME_STAMP_RESULT "hh:mm:ss:ttt : " -#define ADV_PHASE_ERR_FORMAT "[%04X] " -#define ADV_PHASE_MAX_SIZE 32 +// Define a structure to hold debug level information +typedef struct { + CONST CHAR8 *Name; + UINT32 Value; +} DEBUG_LEVEL; + +// Create an array of DebugLevel structures +DEBUG_LEVEL DebugLevels[] = { + { "[INIT]", 0x00000001 }, + { "[WARN]", 0x00000002 }, + { "[LOAD]", 0x00000004 }, + { "[FS]", 0x00000008 }, + { "[POOL]", 0x00000010 }, + { "[PAGE]", 0x00000020 }, + { "[INFO]", 0x00000040 }, + { "[DISPATCH]", 0x00000080 }, + { "[VARIABLE]", 0x00000100 }, + { "[SMI]", 0x00000200 }, + { "[BM]", 0x00000400 }, + { "[BLKIO]", 0x00001000 }, + { "[NET]", 0x00004000 }, + { "[UNDI]", 0x00010000 }, + { "[LOADFILE]", 0x00020000 }, + { "[EVENT]", 0x00080000 }, + { "[GCD]", 0x00100000 }, + { "[CACHE]", 0x00200000 }, + { "[VERBOSE]", 0x00400000 }, + { "[MANAGEABILITY]", 0x00800000 }, + { "[ERROR]", 0x80000000 } +}; + +#define ADV_LOG_TIME_STAMP_FORMAT "%2.2d:%2.2d:%2.2d.%3.3d : " +#define ADV_LOG_TIME_STAMP_RESULT "hh:mm:ss:ttt : " +#define ADV_LOG_PHASE_ERR_FORMAT "[%04X] " +#define ADV_LOG_PHASE_MAX_SIZE 32 +#define ADV_LOG_DEBUG_LEVEL_MAX_SIZE 32 /** @@ -88,21 +120,19 @@ FormatTimeStamp ( TimeStampLen = AsciiSPrint ( MessageBuffer, MessageBufferSize, - ADV_TIME_STAMP_FORMAT, + ADV_LOG_TIME_STAMP_FORMAT, Hours, Minutes, Seconds, Milliseconds ); - ASSERT (TimeStampLen == AsciiStrLen (ADV_TIME_STAMP_RESULT)); + ASSERT (TimeStampLen == AsciiStrLen (ADV_LOG_TIME_STAMP_RESULT)); return (UINT16)TimeStampLen; } /** - FormatPhasePrefix - Adds a phase indicator to the message being returned. If phase is recognized and specified, returns the phase prefix in from the AdvMsgEntryPrefix, otherwise raw phase value is returned. @@ -128,14 +158,64 @@ FormatPhasePrefix ( } else if (Phase < ADVANCED_LOGGER_PHASE_CNT) { // Normal message we recognize PhaseStringLen = AsciiSPrint (MessageBuffer, MessageBufferSize, AdvMsgEntryPrefix[Phase]); + // Verify string length and add an extra space for readability + if (PhaseStringLen < MessageBufferSize - 1) { + MessageBuffer[PhaseStringLen] = ' '; + MessageBuffer[PhaseStringLen + 1] = '\0'; + PhaseStringLen++; + } } else { // Unrecognized phase, just print the raw value - PhaseStringLen = AsciiSPrint (MessageBuffer, MessageBufferSize, ADV_PHASE_ERR_FORMAT, Phase); + PhaseStringLen = AsciiSPrint (MessageBuffer, MessageBufferSize, ADV_LOG_PHASE_ERR_FORMAT, Phase); } return (UINT16)PhaseStringLen; } +/** + Adds a debug level indicator to the message being returned. If debug level is recognized and specified, + returns the debug_level prefix in from the AdvMsgEntryPrefix, otherwise raw debug level value is returned. + + @param MessageBuffer + @param MessageBufferSize + @param DebugLevel + + @retval Number of characters printed +*/ +STATIC +UINT16 +FormatDebugLevelPrefix ( + IN CHAR8 *MessageBuffer, + IN UINTN MessageBufferSize, + IN UINT32 DebugLevel + ) +{ + if ((MessageBuffer == NULL) || (MessageBufferSize == 0)) { + return 0; + } + + UINTN DebugLevelStringLen; + UINTN Index; + + // Print the debug flags + for (Index = 0; Index < ARRAY_SIZE (DebugLevels); Index++) { + if ((DebugLevel & DebugLevels[Index].Value) == DebugLevels[Index].Value) { + DebugLevelStringLen = AsciiSPrint (MessageBuffer, MessageBufferSize, DebugLevels[Index].Name); + // Verify string length and add an extra space for readability + if (DebugLevelStringLen < MessageBufferSize - 1) { + MessageBuffer[DebugLevelStringLen] = ' '; + MessageBuffer[DebugLevelStringLen + 1] = '\0'; + DebugLevelStringLen++; + } + + return (UINT16)DebugLevelStringLen; + } + } + + // If this is not a known debug level, just don't print it out and return 0 + return 0; +} + /** Get Next Message Block. @@ -286,8 +366,11 @@ AdvancedLoggerAccessLibGetNextFormattedLine ( UINT16 TargetLen; UINT16 PhaseStringLen; UINT16 CurrPhaseStringLen; - CHAR8 TimeStampString[] = { ADV_TIME_STAMP_RESULT }; - CHAR8 PhaseString[ADV_PHASE_MAX_SIZE] = { 0 }; + UINT16 DebugLevelStringLen; + UINT16 CurrDebugLevelStringLen; + CHAR8 TimeStampString[] = { ADV_LOG_TIME_STAMP_RESULT }; + CHAR8 PhaseString[ADV_LOG_PHASE_MAX_SIZE] = { 0 }; + CHAR8 DebugLevelString[ADV_LOG_DEBUG_LEVEL_MAX_SIZE] = { 0 }; if (LineEntry == NULL) { return EFI_INVALID_PARAMETER; @@ -298,7 +381,7 @@ AdvancedLoggerAccessLibGetNextFormattedLine ( // reuse the previous LineBuffer // if (LineEntry->Message == NULL) { - LineBuffer = AllocatePool (mMaxMessageSize + sizeof (TimeStampString) + ADV_PHASE_MAX_SIZE); + LineBuffer = AllocatePool (mMaxMessageSize + sizeof (TimeStampString) + ADV_LOG_PHASE_MAX_SIZE); if (LineBuffer == NULL) { return EFI_OUT_OF_RESOURCES; } @@ -314,15 +397,18 @@ AdvancedLoggerAccessLibGetNextFormattedLine ( // GetNextLine. // In case this is a restart of the same Message, initialize the time stamp and prefix. - PhaseStringLen = 0; + PhaseStringLen = 0; + DebugLevelStringLen = 0; if (LineEntry->BlockEntry.Message != NULL) { FormatTimeStamp (TimeStampString, sizeof (TimeStampString), LineEntry->BlockEntry.TimeStamp); CopyMem (LineBuffer, TimeStampString, sizeof (TimeStampString) - sizeof (CHAR8)); PhaseStringLen = FormatPhasePrefix (PhaseString, sizeof (PhaseString), LineEntry->BlockEntry.Phase); CopyMem (LineBuffer + sizeof (TimeStampString) - sizeof (CHAR8), PhaseString, PhaseStringLen); + DebugLevelStringLen = FormatDebugLevelPrefix (DebugLevelString, sizeof (DebugLevelString), LineEntry->BlockEntry.DebugLevel); + CopyMem (LineBuffer + sizeof (TimeStampString) + PhaseStringLen - sizeof (CHAR8), DebugLevelString, DebugLevelStringLen); } - TargetPtr = &LineBuffer[sizeof (TimeStampString) - sizeof (CHAR8) + PhaseStringLen]; + TargetPtr = &LineBuffer[sizeof (TimeStampString) - sizeof (CHAR8) + PhaseStringLen + DebugLevelStringLen]; TargetLen = 0; Status = EFI_SUCCESS; @@ -380,17 +466,25 @@ AdvancedLoggerAccessLibGetNextFormattedLine ( CopyMem (LineBuffer, TimeStampString, sizeof (TimeStampString) - sizeof (CHAR8)); CurrPhaseStringLen = FormatPhasePrefix (PhaseString, sizeof (PhaseString), LineEntry->BlockEntry.Phase); if (PhaseStringLen != CurrPhaseStringLen) { - // Adjust the TargetPtr to point to the end of the PhaseString + // Update the PhaseStringLen PhaseStringLen = CurrPhaseStringLen; - TargetPtr = &LineBuffer[sizeof (TimeStampString) - sizeof (CHAR8) + PhaseStringLen]; } CopyMem (LineBuffer + sizeof (TimeStampString) - sizeof (CHAR8), PhaseString, PhaseStringLen); + + CurrDebugLevelStringLen = FormatDebugLevelPrefix (DebugLevelString, sizeof (DebugLevelString), LineEntry->BlockEntry.DebugLevel); + if (DebugLevelStringLen != CurrDebugLevelStringLen) { + // Adjust the TargetPtr to point to the end of the DebugLevelString + DebugLevelStringLen = CurrDebugLevelStringLen; + TargetPtr = &LineBuffer[sizeof (TimeStampString) - sizeof (CHAR8) + PhaseStringLen + DebugLevelStringLen]; + } + + CopyMem (LineBuffer + sizeof (TimeStampString) - sizeof (CHAR8) + PhaseStringLen, DebugLevelString, DebugLevelStringLen); } } while (!EFI_ERROR (Status)); if (!EFI_ERROR (Status)) { - LineEntry->MessageLen = TargetLen + sizeof (TimeStampString) - sizeof (CHAR8) + PhaseStringLen; + LineEntry->MessageLen = TargetLen + sizeof (TimeStampString) - sizeof (CHAR8) + PhaseStringLen + DebugLevelStringLen; LineEntry->TimeStamp = LineEntry->BlockEntry.TimeStamp; LineEntry->DebugLevel = LineEntry->BlockEntry.DebugLevel; LineEntry->Phase = LineEntry->BlockEntry.Phase; From e7489e905fed26cce25317d5b87071dd595637de Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Tue, 6 Aug 2024 09:36:39 -0700 Subject: [PATCH 05/16] Rust Dependency: Update r-efi requirement from 4.3.0 to 5.0.0 (#538) Updates the requirements on [r-efi](https://github.com/r-efi/r-efi) to permit the latest version. Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index 0e5184cda6..ea5bc470eb 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -21,7 +21,7 @@ HiiKeyboardLayout = {path = "HidPkg/Crates/HiiKeyboardLayout"} memoffset = "0.9.0" num-traits = { version = "0.2", default-features = false} num-derive = { version = "0.4", default-features = false} -r-efi = "4.3.0" +r-efi = "5.0.0" rustversion = "1.0.14" spin = "0.9.8" scroll = { version = "0.12", default-features = false, features = ["derive"]} From 87399cb0d406be47446207f75dbeeb2bdfde3a0e Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Wed, 7 Aug 2024 08:04:55 -0700 Subject: [PATCH 06/16] pip: bump antlr4-python3-runtime from 4.13.1 to 4.13.2 (#542) Bumps [antlr4-python3-runtime](http://www.antlr.org) from 4.13.1 to 4.13.2. Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- pip-requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pip-requirements.txt b/pip-requirements.txt index 7f370736ca..dea67571dd 100644 --- a/pip-requirements.txt +++ b/pip-requirements.txt @@ -14,6 +14,6 @@ edk2-pytool-library==0.21.9 edk2-pytool-extensions==0.27.10 -antlr4-python3-runtime==4.13.1 +antlr4-python3-runtime==4.13.2 regex==2024.7.24 pygount==1.8.0 From b9c33598778a6111e253527f744de76ece1468f4 Mon Sep 17 00:00:00 2001 From: Aaron <105021049+apop5@users.noreply.github.com> Date: Wed, 7 Aug 2024 16:43:59 -0700 Subject: [PATCH 07/16] Switch to use edk2-pytool-library UefiVariableSupportLib (#500) ## Description There are multiple copies of VariableSupportLib floating across repos, mostly only supporting Windows. Functionality has been consolidated into edk2-pytool-library version 0.21.7. Support for Linux has been added. Switch MfciPolicy.py, DecodeUefiLog.py and UefiVarAudit.py to use consolidated version from edk2-pytool-library. Removed local copies of VariableSupportLib.py - [x] Impacts functionality? - **Functionality** - Does the change ultimately impact how firmware functions? - Examples: Add a new library, publish a new PPI, update an algorithm, ... - [ ] Impacts security? - **Security** - Does the change have a direct security impact on an application, flow, or firmware? - Examples: Crypto algorithm change, buffer overflow fix, parameter validation improvement, ... - [ ] Breaking change? - **Breaking change** - Will anyone consuming this change experience a break in build or boot behavior? - Examples: Add a new library class, move a module to a different repo, call a function in a new library class in a pre-existing module, ... - [ ] Includes tests? - **Tests** - Does the change include any explicit test code? - Examples: Unit tests, integration tests, robot tests, ... - [ ] Includes documentation? - **Documentation** - Does the change contain explicit documentation additions outside direct code modifications (and comments)? - Examples: Update readme file, add feature readme file, link to documentation on an a separate Web page, ... ## How This Was Tested ## Integration Instructions N/A --- .../DecodeUefiLog/DecodeUefiLog.py | 9 +- .../DecodeUefiLog/UefiVariablesSupportLib.py | 114 ------------ MfciPkg/Application/MfciPolicy/MfciPolicy.py | 20 +-- .../UefiVariablesSupportLib.py | 169 ------------------ .../UefiVarLockAudit/Windows/UefiVarAudit.py | 21 +-- .../Windows/UefiVariablesSupportLib.py | 92 ---------- 6 files changed, 21 insertions(+), 404 deletions(-) delete mode 100644 AdvLoggerPkg/Application/DecodeUefiLog/UefiVariablesSupportLib.py delete mode 100644 MfciPkg/Application/MfciPolicy/UefiVariableSupport/UefiVariablesSupportLib.py delete mode 100644 UefiTestingPkg/AuditTests/UefiVarLockAudit/Windows/UefiVariablesSupportLib.py diff --git a/AdvLoggerPkg/Application/DecodeUefiLog/DecodeUefiLog.py b/AdvLoggerPkg/Application/DecodeUefiLog/DecodeUefiLog.py index cdc5e7d6f3..19840779e8 100644 --- a/AdvLoggerPkg/Application/DecodeUefiLog/DecodeUefiLog.py +++ b/AdvLoggerPkg/Application/DecodeUefiLog/DecodeUefiLog.py @@ -12,8 +12,7 @@ import copy from win32com.shell import shell - -from UefiVariablesSupportLib import UefiVariable +from edk2toollib.os.uefivariablesupport import UefiVariable class AdvLogParser (): @@ -1003,7 +1002,7 @@ def ReadLogFromUefiInterface(): while rc == 0: VariableName = 'V'+str(Index) - (rc, var, errorstring) = UefiVar.GetUefiVar(VariableName, 'a021bf2b-34ed-4a98-859c-420ef94f3e94') + (rc, var) = UefiVar.GetUefiVar(VariableName, 'a021bf2b-34ed-4a98-859c-420ef94f3e94') if (rc == 0): Index += 1 InFile.write(var) @@ -1060,7 +1059,7 @@ def main(): CountOfLines = len(lines) print(f"{CountOfLines} lines written to {options.OutFilePath}") - except Exception as ex: + except Exception: print("Error processing log output.") traceback.print_exc() @@ -1072,7 +1071,7 @@ def main(): RawFile.close() print("RawFile complete") - except Exception as ex: + except Exception: print("Error processing raw file output.") traceback.print_exc() diff --git a/AdvLoggerPkg/Application/DecodeUefiLog/UefiVariablesSupportLib.py b/AdvLoggerPkg/Application/DecodeUefiLog/UefiVariablesSupportLib.py deleted file mode 100644 index ba63fa458c..0000000000 --- a/AdvLoggerPkg/Application/DecodeUefiLog/UefiVariablesSupportLib.py +++ /dev/null @@ -1,114 +0,0 @@ -# @file -# -# Python lib to support Reading and writing UEFI variables from windows -# -# Modified from original source to not produce error messages for Variable NOT FOUND -# -# Copyright (c), Microsoft Corporation -# SPDX-License-Identifier: BSD-2-Clause-Patent - -import os, sys -from ctypes import * -import logging -import pywintypes -import win32api, win32process, win32security, win32file -import winerror - -kernel32 = windll.kernel32 -EFI_VAR_MAX_BUFFER_SIZE = 1024*1024 - -class UefiVariable(object): - - def __init__(self): - # enable required SeSystemEnvironmentPrivilege privilege - privilege = win32security.LookupPrivilegeValue( None, 'SeSystemEnvironmentPrivilege' ) - token = win32security.OpenProcessToken( win32process.GetCurrentProcess(), win32security.TOKEN_READ|win32security.TOKEN_ADJUST_PRIVILEGES ) - win32security.AdjustTokenPrivileges( token, False, [(privilege, win32security.SE_PRIVILEGE_ENABLED)] ) - win32api.CloseHandle( token ) - - # import firmware variable API - try: - self._GetFirmwareEnvironmentVariable = kernel32.GetFirmwareEnvironmentVariableW - self._GetFirmwareEnvironmentVariable.restype = c_int - self._GetFirmwareEnvironmentVariable.argtypes = [c_wchar_p, c_wchar_p, c_void_p, c_int] - self._SetFirmwareEnvironmentVariable = kernel32.SetFirmwareEnvironmentVariableW - self._SetFirmwareEnvironmentVariable.restype = c_int - self._SetFirmwareEnvironmentVariable.argtypes = [c_wchar_p, c_wchar_p, c_void_p, c_int] - self._SetFirmwareEnvironmentVariableEx = kernel32.SetFirmwareEnvironmentVariableExW - self._SetFirmwareEnvironmentVariableEx.restype = c_int - self._SetFirmwareEnvironmentVariableEx.argtypes = [c_wchar_p, c_wchar_p, c_void_p, c_int, c_int] - except AttributeError as msg: - logging.warn( "G[S]etFirmwareEnvironmentVariableW function doesn't seem to exist" ) - pass - - # - # Helper function to create buffer for var read/write - # - def CreateBuffer(self, init, size=None): - """CreateBuffer(aString) -> character array - CreateBuffer(anInteger) -> character array - CreateBuffer(aString, anInteger) -> character array - """ - if isinstance(init, str): - if size is None: - size = len(init)+1 - buftype = c_char * size - buf = buftype() - buf.value = init - return buf - elif isinstance(init, int): - buftype = c_char * init - buf = buftype() - return buf - raise TypeError(init) - - # - #Function to get variable - # return a tuple of error code and variable data as string - # - def GetUefiVar(self, name, guid ): - err = 0 #success - efi_var = create_string_buffer( EFI_VAR_MAX_BUFFER_SIZE ) - if self._GetFirmwareEnvironmentVariable is not None: - logging.info("calling GetFirmwareEnvironmentVariable( name='%s', GUID='%s' ).." % (name, "{%s}" % guid) ) - length = self._GetFirmwareEnvironmentVariable( name, "{%s}" % guid, efi_var, EFI_VAR_MAX_BUFFER_SIZE ) - if (0 == length) or (efi_var is None): - err = kernel32.GetLastError() - # - # Don't produce an error message for NOT_FOUND - # - if err != 203: # 203 is NOT FOUND - logging.error( 'GetFirmwareEnvironmentVariable[Ex] failed (GetLastError = 0x%x)' % err) - logging.error(WinError()) - return (err, None, WinError(err)) - return (err, efi_var[:length], None) - # - #Function to set variable - # return a tuple of boolean status, errorcode, errorstring (None if not error) - # - def SetUefiVar(self, name, guid, var=None, attrs=None): - var_len = 0 - err = 0 - errorstring = None - if var is None: - var = bytes(0) - else: - var_len = len(var) - success = 0 # Fail - if(attrs == None): - if self._SetFirmwareEnvironmentVariable is not None: - logging.info("Calling SetFirmwareEnvironmentVariable (name='%s', Guid='%s')..." % (name, "{%s}" % guid, )) - success = self._SetFirmwareEnvironmentVariable(name, "{%s}" % guid, var, var_len) - else: - attrs = int(attrs) - if self._SetFirmwareEnvironmentVariableEx is not None: - logging.info(" calling SetFirmwareEnvironmentVariableEx( name='%s', GUID='%s', length=0x%X, attributes=0x%X ).." % (name, "{%s}" % guid, var_len, attrs) ) - success = self._SetFirmwareEnvironmentVariableEx( name, "{%s}" % guid, var, var_len, attrs ) - - if 0 == success: - err = kernel32.GetLastError() - logging.error('SetFirmwareEnvironmentVariable failed (GetLastError = 0x%x)' % err ) - logging.error(WinError()) - errorstring = WinError(err) - return (success,err, errorstring) - diff --git a/MfciPkg/Application/MfciPolicy/MfciPolicy.py b/MfciPkg/Application/MfciPolicy/MfciPolicy.py index 97de7979fb..ac6470eddc 100644 --- a/MfciPkg/Application/MfciPolicy/MfciPolicy.py +++ b/MfciPkg/Application/MfciPolicy/MfciPolicy.py @@ -11,7 +11,7 @@ import logging import argparse import ctypes -from UefiVariableSupport.UefiVariablesSupportLib import UefiVariable +from edk2toollib.os.uefivariablesupport import UefiVariable MFCI_VENDOR_GUID = "EBA1A9D2-BF4D-4736-B680-B36AFB4DD65B" CURRENT_POLICY_BLOB = "CurrentMfciPolicyBlob" @@ -77,7 +77,7 @@ def get_system_info(): UefiVar = UefiVariable() for Variable in MFCI_POLICY_INFO_VARIABLES: - (errorcode, data, errorstring) = UefiVar.GetUefiVar(Variable, MFCI_VENDOR_GUID) + (errorcode, data) = UefiVar.GetUefiVar(Variable, MFCI_VENDOR_GUID) if errorcode != 0: logging.critical(f"Failed to get policy variable {Variable}") else: @@ -91,7 +91,7 @@ def get_system_info(): def get_current_mfci_policy(): UefiVar = UefiVariable() - (errorcode, data, errorstring) = UefiVar.GetUefiVar(CURRENT_MFCI_POLICY, MFCI_VENDOR_GUID) + (errorcode, data) = UefiVar.GetUefiVar(CURRENT_MFCI_POLICY, MFCI_VENDOR_GUID) if errorcode == 0: result = hex(int.from_bytes(data, byteorder="little", signed=False)) logging.info(f" Current MFCI Policy is {result}") @@ -104,17 +104,17 @@ def get_current_mfci_policy(): def delete_current_mfci_policy(): UefiVar = UefiVariable() - (errorcode, data, errorstring) = UefiVar.SetUefiVar(CURRENT_POLICY_BLOB, MFCI_VENDOR_GUID, None, 3) + (errorcode, data) = UefiVar.SetUefiVar(CURRENT_POLICY_BLOB, MFCI_VENDOR_GUID, None, 3) if errorcode == 0: - logging.info(f"Failed to Delete {CURRENT_POLICY_BLOB}\n {errorcode}{errorstring}") + logging.info(f"Failed to Delete {CURRENT_POLICY_BLOB}\n {errorcode}") print(f"Failed to delete {CURRENT_POLICY_BLOB}") else: logging.info(f"{CURRENT_POLICY_BLOB} was deleted") print(f"{CURRENT_POLICY_BLOB} was deleted") - (errorcode, data, errorstring) = UefiVar.SetUefiVar(NEXT_MFCI_POLICY_BLOB, MFCI_VENDOR_GUID, None, 3) + (errorcode, data) = UefiVar.SetUefiVar(NEXT_MFCI_POLICY_BLOB, MFCI_VENDOR_GUID, None, 3) if errorcode == 0: - logging.info(f"Failed to Delete {NEXT_MFCI_POLICY_BLOB}\n {errorcode}{errorstring}") + logging.info(f"Failed to Delete {NEXT_MFCI_POLICY_BLOB}\n {errorcode}") print(f"Failed to delete {NEXT_MFCI_POLICY_BLOB}") else: logging.info(f"{NEXT_MFCI_POLICY_BLOB} was deleted") @@ -128,9 +128,9 @@ def set_next_mfci_policy(policy): var = file.read() UefiVar = UefiVariable() - (errorcode, data, errorstring) = UefiVar.SetUefiVar(NEXT_MFCI_POLICY_BLOB, MFCI_VENDOR_GUID, var, 3) + (errorcode, data) = UefiVar.SetUefiVar(NEXT_MFCI_POLICY_BLOB, MFCI_VENDOR_GUID, var, 3) if errorcode == 0: - logging.info("Next Policy failed: {errorstring}") + logging.info("Next Policy failed: {errorcode}") else: logging.info("Next Policy was set") print(f"{NEXT_MFCI_POLICY_BLOB} was set") @@ -139,7 +139,7 @@ def set_next_mfci_policy(policy): def get_next_mfci_policy(): UefiVar = UefiVariable() - (errorcode, data, errorstring) = UefiVar.GetUefiVar(NEXT_MFCI_POLICY_BLOB, MFCI_VENDOR_GUID) + (errorcode, data) = UefiVar.GetUefiVar(NEXT_MFCI_POLICY_BLOB, MFCI_VENDOR_GUID) if errorcode != 0: logging.info("No Next Mfci Policy Set") print(f"No variable {NEXT_MFCI_POLICY_BLOB} found") diff --git a/MfciPkg/Application/MfciPolicy/UefiVariableSupport/UefiVariablesSupportLib.py b/MfciPkg/Application/MfciPolicy/UefiVariableSupport/UefiVariablesSupportLib.py deleted file mode 100644 index 0197efb844..0000000000 --- a/MfciPkg/Application/MfciPolicy/UefiVariableSupport/UefiVariablesSupportLib.py +++ /dev/null @@ -1,169 +0,0 @@ -# @file -# -# Python lib to support Reading and writing UEFI variables from windows -# -# Copyright (c) Microsoft Corporation. -# SPDX-License-Identifier: BSD-2-Clause-Patent - -from ctypes import ( - windll, - c_wchar_p, - c_void_p, - c_int, - c_char, - create_string_buffer, - WinError, -) -import logging -from win32 import win32api -from win32 import win32process -from win32 import win32security - -kernel32 = windll.kernel32 -EFI_VAR_MAX_BUFFER_SIZE = 1024 * 1024 - - -class UefiVariable(object): - def __init__(self): - # enable required SeSystemEnvironmentPrivilege privilege - privilege = win32security.LookupPrivilegeValue( - None, "SeSystemEnvironmentPrivilege" - ) - token = win32security.OpenProcessToken( - win32process.GetCurrentProcess(), - win32security.TOKEN_READ | win32security.TOKEN_ADJUST_PRIVILEGES, - ) - win32security.AdjustTokenPrivileges( - token, False, [(privilege, win32security.SE_PRIVILEGE_ENABLED)] - ) - win32api.CloseHandle(token) - - # import firmware variable API - try: - self._GetFirmwareEnvironmentVariable = ( - kernel32.GetFirmwareEnvironmentVariableW - ) - self._GetFirmwareEnvironmentVariable.restype = c_int - self._GetFirmwareEnvironmentVariable.argtypes = [ - c_wchar_p, - c_wchar_p, - c_void_p, - c_int, - ] - self._SetFirmwareEnvironmentVariable = ( - kernel32.SetFirmwareEnvironmentVariableW - ) - self._SetFirmwareEnvironmentVariable.restype = c_int - self._SetFirmwareEnvironmentVariable.argtypes = [ - c_wchar_p, - c_wchar_p, - c_void_p, - c_int, - ] - self._SetFirmwareEnvironmentVariableEx = ( - kernel32.SetFirmwareEnvironmentVariableExW - ) - self._SetFirmwareEnvironmentVariableEx.restype = c_int - self._SetFirmwareEnvironmentVariableEx.argtypes = [ - c_wchar_p, - c_wchar_p, - c_void_p, - c_int, - c_int, - ] - except Exception: - logging.warn( - "G[S]etFirmwareEnvironmentVariableW function doesn't seem to exist" - ) - pass - - # - # Helper function to create buffer for var read/write - # - def CreateBuffer(self, init, size=None): - """CreateBuffer(aString) -> character array - CreateBuffer(anInteger) -> character array - CreateBuffer(aString, anInteger) -> character array - """ - if isinstance(init, str): - if size is None: - size = len(init) + 1 - buftype = c_char * size - buf = buftype() - buf.value = init - return buf - elif isinstance(init, int): - buftype = c_char * init - buf = buftype() - return buf - raise TypeError(init) - - # - # Function to get variable - # return a tuple of error code and variable data as string - # - def GetUefiVar(self, name, guid): - # success - err = 0 - efi_var = create_string_buffer(EFI_VAR_MAX_BUFFER_SIZE) - if self._GetFirmwareEnvironmentVariable is not None: - logging.info( - "calling GetFirmwareEnvironmentVariable( name='%s', GUID='%s' ).." - % (name, "{%s}" % guid) - ) - length = self._GetFirmwareEnvironmentVariable( - name, "{%s}" % guid, efi_var, EFI_VAR_MAX_BUFFER_SIZE - ) - if (0 == length) or (efi_var is None): - err = kernel32.GetLastError() - logging.error( - "GetFirmwareEnvironmentVariable[Ex] failed (GetLastError = 0x%x)" % err - ) - logging.error(WinError()) - return (err, None, WinError(err)) - return (err, efi_var[:length], None) - - # - # Function to set variable - # return a tuple of boolean status, error_code, error_string (None if not error) - # - def SetUefiVar(self, name, guid, var=None, attrs=None): - var_len = 0 - err = 0 - error_string = None - if var is None: - var = bytes(0) - else: - var_len = len(var) - success = 0 # Fail - if attrs is None: - if self._SetFirmwareEnvironmentVariable is not None: - logging.info( - "Calling SetFirmwareEnvironmentVariable (name='%s', Guid='%s')..." - % ( - name, - "{%s}" % guid, - ) - ) - success = self._SetFirmwareEnvironmentVariable( - name, "{%s}" % guid, var, var_len - ) - else: - attrs = int(attrs) - if self._SetFirmwareEnvironmentVariableEx is not None: - logging.info( - "Calling SetFirmwareEnvironmentVariableEx( name='%s', GUID='%s', length=0x%X, attributes=0x%X ).." - % (name, "{%s}" % guid, var_len, attrs) - ) - success = self._SetFirmwareEnvironmentVariableEx( - name, "{%s}" % guid, var, var_len, attrs - ) - - if 0 == success: - err = kernel32.GetLastError() - logging.error( - "SetFirmwareEnvironmentVariable failed (GetLastError = 0x%x)" % err - ) - logging.error(WinError()) - error_string = WinError(err) - return (success, err, error_string) diff --git a/UefiTestingPkg/AuditTests/UefiVarLockAudit/Windows/UefiVarAudit.py b/UefiTestingPkg/AuditTests/UefiVarLockAudit/Windows/UefiVarAudit.py index 2507142c93..fb38bd776e 100644 --- a/UefiTestingPkg/AuditTests/UefiVarLockAudit/Windows/UefiVarAudit.py +++ b/UefiTestingPkg/AuditTests/UefiVarLockAudit/Windows/UefiVarAudit.py @@ -8,17 +8,14 @@ ## -import os, sys +import os +import sys import argparse import logging import datetime -import struct -import hashlib -import shutil -import time import xml.etree.ElementTree as ET from xml.etree.ElementTree import Element -from UefiVariablesSupportLib import UefiVariable +from edk2toollib.os.uefivariablesupport import UefiVariable # #main script function @@ -72,13 +69,13 @@ def main(): for var in XmlRoot.findall("Variable"): name = var.get("Name") guid = var.get("Guid") - (ReadStatus, Data, ReadErrorString) = Uefi.GetUefiVar(name, guid) - (WriteSuccess, ErrorCode, WriteErrorString)= Uefi.SetUefiVar(name, guid) + (ReadStatus, Data) = Uefi.GetUefiVar(name, guid) + (WriteSuccess, ErrorCode)= Uefi.SetUefiVar(name, guid) if(WriteSuccess != 0): logging.info("Must Restore Var %s:%s" % (name, guid)) - (RestoreSuccess, RestoreEC, RestoreErrorString) = Uefi.SetUefiVar(name, guid, Data) + (RestoreSuccess, RestoreEC) = Uefi.SetUefiVar(name, guid, Data) if (RestoreSuccess == 0): - logging.critical("Restoring failed for Var %s:%s 0x%X ErrorCode: 0x%X %s" % (name, guid, RestoreSuccess, RestoreEC, RestoreErrorString)) + logging.critical("Restoring failed for Var %s:%s 0x%X ErrorCode: 0x%X %s" % (name, guid, RestoreSuccess, RestoreEC)) #append # #0x0 Success @@ -87,11 +84,7 @@ def main(): rs = Element("ReadStatus") ws = Element("WriteStatus") rs.text = "0x%lX" % (ReadStatus) - if(ReadErrorString is not None): - rs.text = rs.text + " %s" % ReadErrorString ws.text = "0x%lX" % ErrorCode - if(WriteErrorString is not None): - ws.text = ws.text + " %s" % WriteErrorString ele.append(rs) ele.append(ws) var.append(ele) diff --git a/UefiTestingPkg/AuditTests/UefiVarLockAudit/Windows/UefiVariablesSupportLib.py b/UefiTestingPkg/AuditTests/UefiVarLockAudit/Windows/UefiVariablesSupportLib.py deleted file mode 100644 index 9398cbc195..0000000000 --- a/UefiTestingPkg/AuditTests/UefiVarLockAudit/Windows/UefiVariablesSupportLib.py +++ /dev/null @@ -1,92 +0,0 @@ -## -## Python lib to support Reading and writing UEFI variables from windows -## -# -# -# Copyright (C) Microsoft Corporation. All rights reserved. -# SPDX-License-Identifier: BSD-2-Clause-Patent -## - -import os, sys -from ctypes import * -import logging -import pywintypes -import win32api, win32process, win32security, win32file -import winerror - -kernel32 = windll.kernel32 -EFI_VAR_MAX_BUFFER_SIZE = 1024*1024 - -class UefiVariable(object): - - def __init__(self): - # enable required SeSystemEnvironmentPrivilege privilege - privilege = win32security.LookupPrivilegeValue( None, 'SeSystemEnvironmentPrivilege' ) - token = win32security.OpenProcessToken( win32process.GetCurrentProcess(), win32security.TOKEN_READ|win32security.TOKEN_ADJUST_PRIVILEGES ) - win32security.AdjustTokenPrivileges( token, False, [(privilege, win32security.SE_PRIVILEGE_ENABLED)] ) - win32api.CloseHandle( token ) - - # get windows firmware variable API - try: - self._GetFirmwareEnvironmentVariable = kernel32.GetFirmwareEnvironmentVariableW - self._GetFirmwareEnvironmentVariable.restype = c_int - self._GetFirmwareEnvironmentVariable.argtypes = [c_wchar_p, c_wchar_p, c_void_p, c_int] - self._SetFirmwareEnvironmentVariable = kernel32.SetFirmwareEnvironmentVariableW - self._SetFirmwareEnvironmentVariable.restype = c_int - self._SetFirmwareEnvironmentVariable.argtypes = [c_wchar_p, c_wchar_p, c_void_p, c_int] - self._SetFirmwareEnvironmentVariableEx = kernel32.SetFirmwareEnvironmentVariableExW - self._SetFirmwareEnvironmentVariableEx.restype = c_int - self._SetFirmwareEnvironmentVariableEx.argtypes = [c_wchar_p, c_wchar_p, c_void_p, c_int, c_int] - except AttributeError: - logging.warn( "Some get/set functions don't't seem to exist" ) - - # - #Function to get variable - # return a tuple of error code and variable data as string - # - def GetUefiVar(self, name, guid ): - err = 0 #success - efi_var = create_string_buffer( EFI_VAR_MAX_BUFFER_SIZE ) - if self._GetFirmwareEnvironmentVariable is not None: - logging.info("calling GetFirmwareEnvironmentVariable( name='%s', GUID='%s' ).." % (name, "{%s}" % guid) ) - length = self._GetFirmwareEnvironmentVariable( name, "{%s}" % guid, efi_var, EFI_VAR_MAX_BUFFER_SIZE ) - if (0 == length) or (efi_var is None): - err = kernel32.GetLastError() - logging.error( 'GetFirmwareEnvironmentVariable[Ex] failed (GetLastError = 0x%x)' % err) - logging.error(WinError()) - return (err, None, WinError(err)) - return (err, efi_var[:length], None) - # - #Function to set variable - # return a tuple of boolean status, errorcode, errorstring (None if not error) - # - def SetUefiVar(self, name, guid, var=None, attrs=None): - var_len = 0 - err = 0 - errorstring = None - if var is None: - var = bytes(0) - else: - var_len = len(var) - if(attrs == None): - if self._SetFirmwareEnvironmentVariable is not None: - logging.info("Calling SetFirmwareEnvironmentVariable (name='%s', Guid='%s')..." % (name, "{%s}" % guid, )) - success = self._SetFirmwareEnvironmentVariable(name, "{%s}" % guid, var, var_len) - else: - if self._SetFirmwareEnvironmentVariableEx is not None: - logging.info(" calling SetFirmwareEnvironmentVariableEx( name='%s', GUID='%s', length=0x%X, attributes=0x%X ).." % (name, "{%s}" % guid, var_len, attrs) ) - success = self._SetFirmwareEnvironmentVariableEx( name, "{%s}" % guid, var, var_len, attrs ) - - if 0 == success: - err = kernel32.GetLastError() - logging.error('SetFirmwareEnvironmentVariable failed (GetLastError = 0x%x)' % err ) - logging.error(WinError()) - errorstring = WinError(err) - return (success,err, errorstring) - - - -#Test code -#UefiVar = UefiVariable() -#(errorcode, data, errorstring) = UefiVar.GetUefiVar('PK', '8BE4DF61-93CA-11D2-AA0D-00E098032B8C') -#(status, errorcode, errorstring) = UefiVar.SetUefiVar('PK','8BE4DF61-93CA-11D2-AA0D-00E098032B8C',None, None) \ No newline at end of file From 0b3804b898772033c8f44f7a170b5134a5671dd0 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Wed, 7 Aug 2024 20:22:08 -0700 Subject: [PATCH 08/16] pip: bump edk2-pytool-library from 0.21.9 to 0.21.10 (#543) Bumps [edk2-pytool-library](https://github.com/tianocore/edk2-pytool-library) from 0.21.9 to 0.21.10. Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- pip-requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pip-requirements.txt b/pip-requirements.txt index dea67571dd..d0204ba083 100644 --- a/pip-requirements.txt +++ b/pip-requirements.txt @@ -12,7 +12,7 @@ # https://www.python.org/dev/peps/pep-0440/#version-specifiers ## -edk2-pytool-library==0.21.9 +edk2-pytool-library==0.21.10 edk2-pytool-extensions==0.27.10 antlr4-python3-runtime==4.13.2 regex==2024.7.24 From 64cbc066b23861d283c7f0d219a87242807a65ee Mon Sep 17 00:00:00 2001 From: John Schock Date: Wed, 7 Aug 2024 12:27:24 -0700 Subject: [PATCH 09/16] Fix a debug string format specifier. (#544) ## Description Fix an issue where the wrong format specifier was used for an ASCII string. - [ ] Impacts functionality? - [ ] Impacts security? - [ ] Breaking change? - [ ] Includes tests? - [ ] Includes documentation? ## How This Was Tested Observed that ID_NOT_FOUND string was properly formatted. ## Integration Instructions N/A --- .../MfciDeviceIdSupportLibSmbios/MfciDeviceIdSupportLibSmbios.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MfciPkg/Library/MfciDeviceIdSupportLibSmbios/MfciDeviceIdSupportLibSmbios.c b/MfciPkg/Library/MfciDeviceIdSupportLibSmbios/MfciDeviceIdSupportLibSmbios.c index cff1ce67b1..ee4e4dfe4b 100644 --- a/MfciPkg/Library/MfciDeviceIdSupportLibSmbios/MfciDeviceIdSupportLibSmbios.c +++ b/MfciPkg/Library/MfciDeviceIdSupportLibSmbios/MfciDeviceIdSupportLibSmbios.c @@ -72,7 +72,7 @@ GetOptionalStringByIndex ( // Meet the end of strings set but Index is non-zero, or // found an empty string, or Index passed in was 0 // - DEBUG ((DEBUG_ERROR, "SMBIOS string not found, returning \"%s\"\n", ID_NOT_FOUND)); + DEBUG ((DEBUG_ERROR, "SMBIOS string not found, returning \"%a\"\n", ID_NOT_FOUND)); StrSize = sizeof (ID_NOT_FOUND); WhichStr = ID_NOT_FOUND; } else { From 80dac5e1eba7f87a79042d4ad76274ec8ff27248 Mon Sep 17 00:00:00 2001 From: "Project Mu UEFI Bot [bot]" <45776386+uefibot@users.noreply.github.com> Date: Fri, 9 Aug 2024 11:56:17 -0400 Subject: [PATCH 10/16] Repo File Sync: synced file(s) with microsoft/mu_devops (#546) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit synced local file(s) with [microsoft/mu_devops](https://github.com/microsoft/mu_devops). 🤖: View the [Repo File Sync Configuration File](https://github.com/microsoft/mu_devops/blob/main/.sync/Files.yml) to see how files are synced. --- This PR was created automatically by the [repo-file-sync-action](https://github.com/BetaHuhn/repo-file-sync-action) workflow run [#10316616855](https://github.com/microsoft/mu_devops/actions/runs/10316616855) Signed-off-by: Project Mu UEFI Bot --- .cargo/config.toml | 17 ++++++++++ .github/pull_request_template.md | 29 ++--------------- CONTRIBUTING.md | 53 +++++++++++++++++++++++++++++++- 3 files changed, 72 insertions(+), 27 deletions(-) create mode 100644 .cargo/config.toml diff --git a/.cargo/config.toml b/.cargo/config.toml new file mode 100644 index 0000000000..48bd897d22 --- /dev/null +++ b/.cargo/config.toml @@ -0,0 +1,17 @@ +[target.x86_64-unknown-uefi] +rustflags = [ + "-C", "link-arg=/base:0x0", + "-C", "link-arg=/subsystem:efi_boot_service_driver", +] + +[target.i686-unknown-uefi] +rustflags = [ + "-C", "link-arg=/base:0x0", + "-C", "link-arg=/subsystem:efi_boot_service_driver", +] + +[target.aarch64-unknown-uefi] +rustflags = [ + "-C", "link-arg=/base:0x0", + "-C", "link-arg=/subsystem:efi_boot_service_driver", +] diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 0f216b6d50..60a2b7af1b 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -1,41 +1,18 @@ -# Preface - -Please ensure you have read the [contribution docs](https://github.com/microsoft/mu/blob/master/CONTRIBUTING.md) prior -to submitting the pull request. In particular, -[pull request guidelines](https://github.com/microsoft/mu/blob/master/CONTRIBUTING.md#pull-request-best-practices). - ## Description -<_Please include a description of the change and why this change was made._> +<_Include a description of the change and why this change was made._> -For each item, place an "x" in between `[` and `]` if true. Example: `[x]`. -_(you can also check items in the GitHub UI)_ +For details on how to complete to complete these options and their meaning refer to [CONTRIBUTING.md](https://github.com/microsoft/mu/blob/HEAD/CONTRIBUTING.md). - [ ] Impacts functionality? - - **Functionality** - Does the change ultimately impact how firmware functions? - - Examples: Add a new library, publish a new PPI, update an algorithm, ... - [ ] Impacts security? - - **Security** - Does the change have a direct security impact on an application, - flow, or firmware? - - Examples: Crypto algorithm change, buffer overflow fix, parameter - validation improvement, ... - [ ] Breaking change? - - **Breaking change** - Will anyone consuming this change experience a break - in build or boot behavior? - - Examples: Add a new library class, move a module to a different repo, call - a function in a new library class in a pre-existing module, ... - [ ] Includes tests? - - **Tests** - Does the change include any explicit test code? - - Examples: Unit tests, integration tests, robot tests, ... - [ ] Includes documentation? - - **Documentation** - Does the change contain explicit documentation additions - outside direct code modifications (and comments)? - - Examples: Update readme file, add feature readme file, link to documentation - on an a separate Web page, ... ## How This Was Tested -<_Please describe the test(s) that were run to verify the changes._> +<_Describe the test(s) that were run to verify the changes._> ## Integration Instructions diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index c6ff888b87..dc0f5763ec 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -21,7 +21,7 @@ submitted in the issues section. ## Security Vulnerabilities Please review the repos `Security Policy` but in general every Project Mu repo has `Private vulnerability reporting` -enabled. Please use the security tab to report a potential issue. +enabled. Please use the security tab to report a potential issue. ### Identify Where to Report @@ -63,6 +63,57 @@ configuration files. To aid maintainers in reviewing your code, we suggest adher * If the contribution logically be broken up into separate pull requests that independently build and function successfully, do use multiple pull requests. +#### Pull Request Description Checkboxes + +Project Mu pull requests autopopulate a PR description from a template in most repositories. You should: + +1. **Replace** this text with an actual descrption: + + ```txt + <_Include a description of the change and why this change was made._> + ``` + +2. **Remove** this line of instructions so the PR description shows cleanly in release notes: + + `"For details on how to complete to complete these options and their meaning refer to [CONTRIBUTING.md](https://github.com/microsoft/mu/blob/HEAD/CONTRIBUTING.md)."` + +3. For each checkbox in the PR description, **place an "x"** in between `[` and `]` if true. Example: `[x]`. + _(you can also check items in the GitHub UI)_ + + * **[] Impacts functionality?** + * **Functionality** - Does the change ultimately impact how firmware functions? + * Examples: Add a new library, publish a new PPI, update an algorithm, ... + * **[] Impacts security?** + * **Security** - Does the change have a direct security impact on an application, + flow, or firmware? + * Examples: Crypto algorithm change, buffer overflow fix, parameter + validation improvement, ... + * **[] Breaking change?** + * **Breaking change** - Will anyone consuming this change experience a break + in build or boot behavior? + * Examples: Add a new library class, move a module to a different repo, call + a function in a new library class in a pre-existing module, ... + * [] **Includes tests?** + * **Tests** - Does the change include any explicit test code? + * Examples: Unit tests, integration tests, robot tests, ... + * [] **Includes documentation?** + * **Documentation** - Does the change contain explicit documentation additions + outside direct code modifications (and comments)? + * Examples: Update readme file, add feature readme file, link to documentation + on an a separate Web page, ... + +4. **Replace** this text as instructed: + + ```txt + <_Describe the test(s) that were run to verify the changes._> + ``` + +5. **Replace** this text as instructed: + + ```txt + <_Describe how these changes should be integrated. Use N/A if nothing is required._> + ``` + #### Code Categories To keep code digestible, you may consider breaking large pull requests into three categories of commits within the pull From bc5f1f007ae9a24d7d011b14b4bb88520f506d38 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Wed, 14 Aug 2024 08:14:27 -0700 Subject: [PATCH 11/16] pip: bump edk2-pytool-extensions from 0.27.10 to 0.27.11 (#550) Bumps [edk2-pytool-extensions](https://github.com/tianocore/edk2-pytool-extensions) from 0.27.10 to 0.27.11. Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- pip-requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pip-requirements.txt b/pip-requirements.txt index d0204ba083..83f0e5ec1a 100644 --- a/pip-requirements.txt +++ b/pip-requirements.txt @@ -13,7 +13,7 @@ ## edk2-pytool-library==0.21.10 -edk2-pytool-extensions==0.27.10 +edk2-pytool-extensions==0.27.11 antlr4-python3-runtime==4.13.2 regex==2024.7.24 pygount==1.8.0 From fa263f16c1c7f7a025afc3ed6b6e994886a17f39 Mon Sep 17 00:00:00 2001 From: julorenz117 <60625130+julorenz117@users.noreply.github.com> Date: Wed, 21 Aug 2024 19:09:53 -0700 Subject: [PATCH 12/16] MsGraphicsPkg: Correct positioning of trash can icon in Load Option's list box (#552) ## Description Fixes #554 - Adjusted CellTrashcanBounds.Left to be CellBounds->Right - TrashcanHitAreaWidth to ensure the trash can icon is displayed to the right of the list box. - Updated width parameter in SWM_RECT_INIT2 to use TrashcanHitAreaWidth instead of CheckBoxHitAreaWidth for correct dimensions. This resolves the issue of the trash can icon overlapping with the ListBox's deletable item's checkbox thus ensuring its related operations work correctly: activating/deactivating the Load Option or deleting it. ## How This Was Tested Verified that a Load Option allowed to be deleted, such as 'Windows Boot Manager', can now be deleted by pressing the trash icon in its proper position or activated via its check-box. ## Integration Instructions N/A Co-authored-by: Michael Kubacki --- MsGraphicsPkg/Library/SimpleUIToolKit/ListBox.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/MsGraphicsPkg/Library/SimpleUIToolKit/ListBox.c b/MsGraphicsPkg/Library/SimpleUIToolKit/ListBox.c index fcc42d1a73..2468a7f142 100644 --- a/MsGraphicsPkg/Library/SimpleUIToolKit/ListBox.c +++ b/MsGraphicsPkg/Library/SimpleUIToolKit/ListBox.c @@ -982,9 +982,9 @@ Ctor ( SWM_RECT_INIT2 ( this->m_pCells[Index].CellTrashcanBounds, - CellBounds->Left, + CellBounds->Right - TrashcanHitAreaWidth, CellBounds->Top, - CheckBoxHitAreaWidth, + TrashcanHitAreaWidth, SWM_RECT_HEIGHT (*CellBounds) ); } From 5d44a00663355cf42ef24259d430cc505598c411 Mon Sep 17 00:00:00 2001 From: Raymond-MS Date: Wed, 28 Aug 2024 10:49:03 -0700 Subject: [PATCH 13/16] Removed reference to PcdAdvancedHdwLoggerDisable in the README (#541) ## Description Removed reference to PcdAdvHdwLoggerDisable in the ReadMe MarkDown document. In commit 38339016109e88a3f4ebaf8a7728b68bde0ca95f, PcdAdvancedSerialLoggerDisable in AdvLoggerPkg.dec was changed to PcdAdvancedLoggerHdwPortDisable but in the ReadMe was changed to PcdAdvancedHdwLoggerDisable. This PCD was later removed from the .DEC in commit 8770182b7b18d55497763c4771f0804bc1e6d783 but wasn't removed from the ReadMe due to the naming difference. - [ ] Impacts functionality? - **Functionality** - Does the change ultimately impact how firmware functions? - Examples: Add a new library, publish a new PPI, update an algorithm, ... - [ ] Impacts security? - **Security** - Does the change have a direct security impact on an application, flow, or firmware? - Examples: Crypto algorithm change, buffer overflow fix, parameter validation improvement, ... - [ ] Breaking change? - **Breaking change** - Will anyone consuming this change experience a break in build or boot behavior? - Examples: Add a new library class, move a module to a different repo, call a function in a new library class in a pre-existing module, ... - [ ] Includes tests? - **Tests** - Does the change include any explicit test code? - Examples: Unit tests, integration tests, robot tests, ... - [x] Includes documentation? - **Documentation** - Does the change contain explicit documentation additions outside direct code modifications (and comments)? - Examples: Update readme file, add feature readme file, link to documentation on an a separate Web page, ... ## How This Was Tested Ran CI for X64 Architecture. All Test Passed. ## Integration Instructions N/A --- AdvLoggerPkg/Docs/ReadMe.md | 1 - 1 file changed, 1 deletion(-) diff --git a/AdvLoggerPkg/Docs/ReadMe.md b/AdvLoggerPkg/Docs/ReadMe.md index 0d67369642..0cdd30ff3f 100644 --- a/AdvLoggerPkg/Docs/ReadMe.md +++ b/AdvLoggerPkg/Docs/ReadMe.md @@ -28,7 +28,6 @@ PCD's used by Advanced Logger |PcdAdvancedLoggerPeiInRAM | For systems that have memory at PeiCore entry. The full in memory log buffer if PcdAdvancedLoggerPages is allocated in the Pei Core constructor and PcdAdvancedLoggerPreMemPages is ignored.| |PcdAdvancedLoggerFixedInRAM | For systems that have a fixed memory buffer prior to UEFI. The full in memory log buffer is assumed.| |PcdAdvancedHdwLoggerDebugPrintErrorLevel | The standard debug flags filter which log messages are produced. This PCD allow a subset of log messages to be forwarded to the Hdw Port Lib.| -|PcdAdvancedHdwLoggerDisable | Specifies when to disable writing to the Hdw Port.| |PcdAdvancedLoggerPreMemPages | Amount of temporary RAM used for the debug log.| |PcdAdvancedLoggerPages | Amount of system RAM used for the debug log| |PcdAdvancedLoggerLocator | When enabled, the AdvLogger creates a variable "AdvLoggerLocator" with the address of the LoggerInfo buffer| From e135172922f507010b5db5cd1d0a7e0b613efdb8 Mon Sep 17 00:00:00 2001 From: Raymond-MS Date: Wed, 28 Aug 2024 11:20:02 -0700 Subject: [PATCH 14/16] Removed All References to PcdAdvancedLoggerPeiInRam (#540) ## Description The PCD PcdAdvancedLoggerPeiInRam does not work and is being removed. For Intel systems, the PEI phase uses cache-as-ram and determines that there is no way to allocate memory. This PCD was supposed to signal to the logger that it could allocate memory, however, it would error as memory allocation was not allowed in that phase. - [ ] Impacts functionality? - **Functionality** - Does the change ultimately impact how firmware functions? - Examples: Add a new library, publish a new PPI, update an algorithm, ... - [ ] Impacts security? - **Security** - Does the change have a direct security impact on an application, flow, or firmware? - Examples: Crypto algorithm change, buffer overflow fix, parameter validation improvement, ... - [x] Breaking change? - **Breaking change** - Will anyone consuming this change experience a break in build or boot behavior? - Examples: Add a new library class, move a module to a different repo, call a function in a new library class in a pre-existing module, ... - [ ] Includes tests? - **Tests** - Does the change include any explicit test code? - Examples: Unit tests, integration tests, robot tests, ... - [x] Includes documentation? - **Documentation** - Does the change contain explicit documentation additions outside direct code modifications (and comments)? - Examples: Update readme file, add feature readme file, link to documentation on an a separate Web page, ... ## How This Was Tested Ran the CI build for all packages in mu_plus for the X64 architecture. All tests passing. ## Integration Instructions Users of PcdAdvancedLoggerPeiInRam will need to remove that PCD from DSCs. It is not expected there are users as the PCD does not work, although platforms may have specifically set it to FALSE. --- AdvLoggerPkg/AdvLoggerPkg.dec | 5 ---- AdvLoggerPkg/Docs/ReadMe.md | 3 +-- .../BaseArm/AdvancedLoggerLib.c | 1 - .../MmCoreArm/AdvancedLoggerLib.c | 1 - .../PeiCore/AdvancedLoggerLib.c | 24 +++---------------- .../PeiCore/AdvancedLoggerLib.inf | 1 - 6 files changed, 4 insertions(+), 31 deletions(-) diff --git a/AdvLoggerPkg/AdvLoggerPkg.dec b/AdvLoggerPkg/AdvLoggerPkg.dec index f4bfd2589f..3c325a170e 100644 --- a/AdvLoggerPkg/AdvLoggerPkg.dec +++ b/AdvLoggerPkg/AdvLoggerPkg.dec @@ -85,11 +85,6 @@ # gAdvLoggerPkgTokenSpaceGuid.PcdAdvancedLoggerFixedInRAM|FALSE|BOOLEAN|0x00010189 - ## PcdAdvancedLoggerPeiInRAM - Tells the PEI Advanced Logger that this PEI can allocate memory - # and there is no need for a temporary memory buffer - # - gAdvLoggerPkgTokenSpaceGuid.PcdAdvancedLoggerPeiInRAM|FALSE|BOOLEAN|0x00010185 - ## PcdAdvancedLoggerLocator - Tells the Advanced Logger to publish a variable with the logger info block address # gAdvLoggerPkgTokenSpaceGuid.PcdAdvancedLoggerLocator|FALSE|BOOLEAN|0x00010186 diff --git a/AdvLoggerPkg/Docs/ReadMe.md b/AdvLoggerPkg/Docs/ReadMe.md index 0cdd30ff3f..cd0c23cbae 100644 --- a/AdvLoggerPkg/Docs/ReadMe.md +++ b/AdvLoggerPkg/Docs/ReadMe.md @@ -14,7 +14,7 @@ The following configurations are supported: | --- | --- | | DXE Only | Uses DxeCore, DxeRuntime, and Dxe AdvancedLoggerLib libraries for logging from start of DXE CORE through Exit Boot Services. Accepts the PEI Advanced Logger Hob if one is generated. Produces the AdvancedLogger protocol. | | DXE+SMM | Requires DXE modules above, and adds the Smm AdvancedLoggerLib library. Collects SMM generated messages in the in memory log. | -| PEI | Uses PeiCore and Pei AdvancedLoggerLib libraries. Creates the Advanced Logger Hob if PcdAdvancedLoggerPeiInRAM is set. | +| PEI | Uses PeiCore and Pei AdvancedLoggerLib libraries. | | SEC | Uses the Sec Advanced Logger Library. SEC requires a fixed load address, so it piggy backs on the Temporary RAM PCD information. Produces a Fixed Address temporary RAM log. When memory is added, the Sec Advanced Logger library converts the Temporary RAM logging information to the PEI Advanced Logger Hob. | | PEI64 | Uses Pei64 Advanced Logger Library. Requires the SEC fixed address temporary log information in order to log Pei64 bit DEBUG messages. | | MM | Standalone MM - Loads during PEI phase. | @@ -25,7 +25,6 @@ PCD's used by Advanced Logger | PCD | Function of the PCD| | --- | --- | |PcdAdvancedLoggerForceEnable | The default operation is to check if a Logs directory is present in the root of the filesystem. If the UefiLogs directory is present, logging is enabled. When PcdAdvancedLoggerForceEnable is TRUE, and the device is not a USB device, a UefiLogs directory will be created and logging is enabled. When logging is enabled, the proper log files will be created if not already preset.| -|PcdAdvancedLoggerPeiInRAM | For systems that have memory at PeiCore entry. The full in memory log buffer if PcdAdvancedLoggerPages is allocated in the Pei Core constructor and PcdAdvancedLoggerPreMemPages is ignored.| |PcdAdvancedLoggerFixedInRAM | For systems that have a fixed memory buffer prior to UEFI. The full in memory log buffer is assumed.| |PcdAdvancedHdwLoggerDebugPrintErrorLevel | The standard debug flags filter which log messages are produced. This PCD allow a subset of log messages to be forwarded to the Hdw Port Lib.| |PcdAdvancedLoggerPreMemPages | Amount of temporary RAM used for the debug log.| diff --git a/AdvLoggerPkg/Library/AdvancedLoggerLib/BaseArm/AdvancedLoggerLib.c b/AdvLoggerPkg/Library/AdvancedLoggerLib/BaseArm/AdvancedLoggerLib.c index 48dfdc2c4e..f64affe5d7 100644 --- a/AdvLoggerPkg/Library/AdvancedLoggerLib/BaseArm/AdvancedLoggerLib.c +++ b/AdvLoggerPkg/Library/AdvancedLoggerLib/BaseArm/AdvancedLoggerLib.c @@ -76,7 +76,6 @@ ValidateInfoBlock ( The following PCD settings are assumed: - PcdAdvancedLoggerPeiInRAM -- TRUE PcdAdvancedLoggerBase -- NOT NULL and pointer to memory to be used PcdAdvancedLoggerPages -- > 64KB of pages PcdAdvancedLoggerCarBase -- NOT USED, leave at default diff --git a/AdvLoggerPkg/Library/AdvancedLoggerLib/MmCoreArm/AdvancedLoggerLib.c b/AdvLoggerPkg/Library/AdvancedLoggerLib/MmCoreArm/AdvancedLoggerLib.c index 583ed85528..0bd4f4c511 100644 --- a/AdvLoggerPkg/Library/AdvancedLoggerLib/MmCoreArm/AdvancedLoggerLib.c +++ b/AdvLoggerPkg/Library/AdvancedLoggerLib/MmCoreArm/AdvancedLoggerLib.c @@ -31,7 +31,6 @@ The following PCD settings are assumed: - PcdAdvancedLoggerPeiInRAM -- TRUE PcdAdvancedLoggerBase -- NOT NULL and pointer to memory to be used PcdAdvancedLoggerPages -- > 64KB of pages PcdAdvancedLoggerCarBase -- NOT USED, leave at default diff --git a/AdvLoggerPkg/Library/AdvancedLoggerLib/PeiCore/AdvancedLoggerLib.c b/AdvLoggerPkg/Library/AdvancedLoggerLib/PeiCore/AdvancedLoggerLib.c index a97c0de4f4..b76006b83f 100644 --- a/AdvLoggerPkg/Library/AdvancedLoggerLib/PeiCore/AdvancedLoggerLib.c +++ b/AdvLoggerPkg/Library/AdvancedLoggerLib/PeiCore/AdvancedLoggerLib.c @@ -453,7 +453,6 @@ AdvancedLoggerGetLoggerInfo ( UINTN Pages; CONST EFI_PEI_SERVICES **PeiServices; EFI_STATUS Status; - EFI_MEMORY_TYPE Type; ADVANCED_LOGGER_MESSAGE_ENTRY_V2 *LogEntry; // Try to do the minimum work at the start of this function as this @@ -553,19 +552,12 @@ AdvancedLoggerGetLoggerInfo ( // Memory Discovered Ppi. At that time, the full in memory log buffer is allocated. // - if (FeaturePcdGet (PcdAdvancedLoggerPeiInRAM)) { - Pages = FixedPcdGet32 (PcdAdvancedLoggerPages); - Type = EfiRuntimeServicesData; - } else { - Pages = FixedPcdGet32 (PcdAdvancedLoggerPreMemPages); - // This is to avoid the interim buffer being allocated to consume 64KB on ARM64 platforms. - Type = EfiBootServicesData; - } + Pages = FixedPcdGet32 (PcdAdvancedLoggerPreMemPages); BufferSize = EFI_PAGES_TO_SIZE (Pages); Status = PeiServicesAllocatePages ( - Type, + EfiBootServicesData, Pages, &NewLoggerInfo ); @@ -623,17 +615,7 @@ AdvancedLoggerGetLoggerInfo ( Status = PeiServicesInstallPpi (mAdvancedLoggerPpiList); ASSERT_EFI_ERROR (Status); - if (FeaturePcdGet (PcdAdvancedLoggerPeiInRAM)) { - LoggerInfo->InPermanentRAM = TRUE; - Status = MmUnblockMemoryRequest (NewLoggerInfo, Pages); - if (EFI_ERROR (Status)) { - if (Status != EFI_UNSUPPORTED) { - DEBUG ((DEBUG_ERROR, "%a: Unable to notify StandaloneMM. Code=%r\n", __FUNCTION__, Status)); - } - } else { - DEBUG ((DEBUG_INFO, "%a: StandaloneMM Hob data published\n", __FUNCTION__)); - } - } else if (FeaturePcdGet (PcdAdvancedLoggerFixedInRAM)) { + if (FeaturePcdGet (PcdAdvancedLoggerFixedInRAM)) { DEBUG ((DEBUG_INFO, "%a: Standalone MM Hob of fixed data published\n", __FUNCTION__)); } else { PeiServicesNotifyPpi (mMemoryDiscoveredNotifyList); diff --git a/AdvLoggerPkg/Library/AdvancedLoggerLib/PeiCore/AdvancedLoggerLib.inf b/AdvLoggerPkg/Library/AdvancedLoggerLib/PeiCore/AdvancedLoggerLib.inf index 5348657e4f..f766a7469d 100644 --- a/AdvLoggerPkg/Library/AdvancedLoggerLib/PeiCore/AdvancedLoggerLib.inf +++ b/AdvLoggerPkg/Library/AdvancedLoggerLib/PeiCore/AdvancedLoggerLib.inf @@ -54,7 +54,6 @@ gEfiPeiMemoryDiscoveredPpiGuid ## CONSUMES [FeaturePcd] - gAdvLoggerPkgTokenSpaceGuid.PcdAdvancedLoggerPeiInRAM ## CONSUMES gAdvLoggerPkgTokenSpaceGuid.PcdAdvancedLoggerFixedInRAM ## CONSUMES gAdvLoggerPkgTokenSpaceGuid.PcdAdvancedLoggerAutoWrapEnable From 602574c2a62dbf8f01eadaca54332f924e5082d3 Mon Sep 17 00:00:00 2001 From: Oliver Smith-Denny Date: Mon, 29 Jul 2024 10:44:56 -0700 Subject: [PATCH 15/16] DxePagingAuditTestApp: Remove MemoryOutsideEfiMemoryMapIsInaccessible Test MemoryOutsideEfiMemoryMapIsInaccessible was attempting to test that memory outside the EFI_MEMORY_MAP was marked EFI_MEMORY_RP or unmapped, however this is not a valid test as we expect there to be ranges outside of the EFI_MEMORY_MAP, such as GCD non-existent memory and non-runtime MMIO ranges. This patch removes the test. --- .../AuditTests/PagingAudit/README.md | 2 - .../UEFI/Dxe/App/DxePagingAuditTestApp.c | 107 ------------------ 2 files changed, 109 deletions(-) diff --git a/UefiTestingPkg/AuditTests/PagingAudit/README.md b/UefiTestingPkg/AuditTests/PagingAudit/README.md index 98c9e0f467..853a6aa928 100644 --- a/UefiTestingPkg/AuditTests/PagingAudit/README.md +++ b/UefiTestingPkg/AuditTests/PagingAudit/README.md @@ -66,8 +66,6 @@ is installed. code are EFI_MEMORY_RO and sections containing data are EFI_MEMORY_XP. - **BspStackIsXpAndHasGuardPage:** Checks that the stack is EFI_MEMORY_XP and has an EFI_MEMORY_RP page at the base to catch overflow. -- **MemoryOutsideEfiMemoryMapIsInaccessible:** Checks that memory ranges not in -the EFI memory map EFI_MEMORY_RP or is not mapped. #### Mode 2: Paging Audit Collection Tool diff --git a/UefiTestingPkg/AuditTests/PagingAudit/UEFI/Dxe/App/DxePagingAuditTestApp.c b/UefiTestingPkg/AuditTests/PagingAudit/UEFI/Dxe/App/DxePagingAuditTestApp.c index fe68ad7dd5..b6b1d5e2be 100644 --- a/UefiTestingPkg/AuditTests/PagingAudit/UEFI/Dxe/App/DxePagingAuditTestApp.c +++ b/UefiTestingPkg/AuditTests/PagingAudit/UEFI/Dxe/App/DxePagingAuditTestApp.c @@ -1348,112 +1348,6 @@ BspStackIsXpAndHasGuardPage ( return UNIT_TEST_PASSED; } -/** - Checks that memory ranges not in the EFI - memory map will cause a CPU fault if accessed. - - @param[in] Context Unit test context - - @retval UNIT_TEST_PASSED The unit test passed - @retval other The unit test failed -**/ -STATIC -UNIT_TEST_STATUS -EFIAPI -MemoryOutsideEfiMemoryMapIsInaccessible ( - IN UNIT_TEST_CONTEXT Context - ) -{ - UINT64 StartOfAddressSpace; - UINT64 EndOfAddressSpace; - EFI_MEMORY_DESCRIPTOR *EndOfEfiMemoryMap; - EFI_MEMORY_DESCRIPTOR *CurrentEfiMemoryMapEntry; - BOOLEAN TestFailure; - EFI_PHYSICAL_ADDRESS LastMemoryMapEntryEnd; - EFI_STATUS Status; - - DEBUG ((DEBUG_INFO, "%a Enter...\n", __FUNCTION__)); - - UT_ASSERT_NOT_EFI_ERROR (ValidatePageTableMapSize ()); - UT_ASSERT_NOT_EFI_ERROR (ValidateEfiMemoryMapSize ()); - UT_ASSERT_NOT_EFI_ERROR (PopulateMemorySpaceMap ()); - UT_ASSERT_NOT_NULL (mMemorySpaceMap); - UT_ASSERT_NOT_EFI_ERROR (PopulateEfiMemoryMap ()); - UT_ASSERT_NOT_EFI_ERROR (PopulatePageTableMap ()); - - StartOfAddressSpace = mMemorySpaceMap[0].BaseAddress; - EndOfAddressSpace = mMemorySpaceMap[mMemorySpaceMapCount - 1].BaseAddress + - mMemorySpaceMap[mMemorySpaceMapCount - 1].Length; - TestFailure = FALSE; - EndOfEfiMemoryMap = (EFI_MEMORY_DESCRIPTOR *)(((UINT8 *)mEfiMemoryMap + mEfiMemoryMapSize)); - CurrentEfiMemoryMapEntry = mEfiMemoryMap; - - if (CurrentEfiMemoryMapEntry->PhysicalStart > StartOfAddressSpace) { - Status = ValidateRegionAttributes ( - &mMap, - StartOfAddressSpace, - CurrentEfiMemoryMapEntry->PhysicalStart - StartOfAddressSpace, - EFI_MEMORY_RP, - TRUE, - TRUE, - TRUE - ); - - // Inaccessible could mean EFI_MEMORY_RP or completely unmapped in page table - if (EFI_ERROR (Status) && (Status != EFI_NO_MAPPING)) { - TestFailure = TRUE; - } - } - - LastMemoryMapEntryEnd = CurrentEfiMemoryMapEntry->PhysicalStart + - (CurrentEfiMemoryMapEntry->NumberOfPages * EFI_PAGE_SIZE); - CurrentEfiMemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (CurrentEfiMemoryMapEntry, mEfiMemoryMapDescriptorSize); - - while ((UINTN)CurrentEfiMemoryMapEntry < (UINTN)EndOfEfiMemoryMap) { - if (CurrentEfiMemoryMapEntry->PhysicalStart > LastMemoryMapEntryEnd) { - Status = ValidateRegionAttributes ( - &mMap, - LastMemoryMapEntryEnd, - CurrentEfiMemoryMapEntry->PhysicalStart - LastMemoryMapEntryEnd, - EFI_MEMORY_RP, - TRUE, - TRUE, - TRUE - ); - - // Inaccessible could mean EFI_MEMORY_RP or completely unmapped in page table - if (EFI_ERROR (Status) && (Status != EFI_NO_MAPPING)) { - TestFailure = TRUE; - } - } - - LastMemoryMapEntryEnd = CurrentEfiMemoryMapEntry->PhysicalStart + - (CurrentEfiMemoryMapEntry->NumberOfPages * EFI_PAGE_SIZE); - CurrentEfiMemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (CurrentEfiMemoryMapEntry, mEfiMemoryMapDescriptorSize); - } - - if (LastMemoryMapEntryEnd < EndOfAddressSpace) { - Status = ValidateRegionAttributes ( - &mMap, - LastMemoryMapEntryEnd, - EndOfAddressSpace - LastMemoryMapEntryEnd, - EFI_MEMORY_RP, - TRUE, - TRUE, - TRUE - ); - - // Inaccessible could mean EFI_MEMORY_RP or completely unmapped in page table - if (EFI_ERROR (Status) && (Status != EFI_NO_MAPPING)) { - TestFailure = TRUE; - } - } - - UT_ASSERT_FALSE (TestFailure); - - return UNIT_TEST_PASSED; -} - /** Entry Point of the shell app. @@ -1547,7 +1441,6 @@ DxePagingAuditTestAppEntryPoint ( AddTestCase (Misc, "MMIO Regions are EFI_MEMORY_XP", "Security.Misc.MmioIsXp", MmioIsXp, NULL, GeneralTestCleanup, NULL); AddTestCase (Misc, "Image code sections are EFI_MEMORY_RO and and data sections are EFI_MEMORY_XP", "Security.Misc.ImageCodeSectionsRoDataSectionsXp", ImageCodeSectionsRoDataSectionsXp, NULL, GeneralTestCleanup, NULL); AddTestCase (Misc, "BSP stack is EFI_MEMORY_XP and has EFI_MEMORY_RP guard page", "Security.Misc.BspStackIsXpAndHasGuardPage", BspStackIsXpAndHasGuardPage, NULL, GeneralTestCleanup, NULL); - AddTestCase (Misc, "Memory outside of the EFI Memory Map is inaccessible", "Security.Misc.MemoryOutsideEfiMemoryMapIsInaccessible", MemoryOutsideEfiMemoryMapIsInaccessible, NULL, GeneralTestCleanup, NULL); // // Execute the tests. From 4cd4ed03d95829ab2e6cfeedb8fb82e12ca458c8 Mon Sep 17 00:00:00 2001 From: "YiTa Wu (AMI US Holdings Inc)" Date: Tue, 10 Sep 2024 14:19:46 +0800 Subject: [PATCH 16/16] Add MockDeviceBootManagerLib --- MsCorePkg/MsCorePkg.dec | 1 + .../Library/MockDeviceBootManagerLib.h | 66 +++++++++++++++++++ .../MockDeviceBootManagerLib.cpp | 17 +++++ .../MockDeviceBootManagerLib.inf | 34 ++++++++++ MsCorePkg/UnitTests/MsCorePkgHostTest.dsc | 2 + 5 files changed, 120 insertions(+) create mode 100644 MsCorePkg/UnitTests/Mock/Include/GoogleTest/Library/MockDeviceBootManagerLib.h create mode 100644 MsCorePkg/UnitTests/Mock/Library/MockDeviceBootManagerLib/MockDeviceBootManagerLib.cpp create mode 100644 MsCorePkg/UnitTests/Mock/Library/MockDeviceBootManagerLib/MockDeviceBootManagerLib.inf diff --git a/MsCorePkg/MsCorePkg.dec b/MsCorePkg/MsCorePkg.dec index 074be6d792..295700f41f 100644 --- a/MsCorePkg/MsCorePkg.dec +++ b/MsCorePkg/MsCorePkg.dec @@ -18,6 +18,7 @@ [Includes] Include + UnitTests/Mock/Include [LibraryClasses] diff --git a/MsCorePkg/UnitTests/Mock/Include/GoogleTest/Library/MockDeviceBootManagerLib.h b/MsCorePkg/UnitTests/Mock/Include/GoogleTest/Library/MockDeviceBootManagerLib.h new file mode 100644 index 0000000000..e5fc5b7022 --- /dev/null +++ b/MsCorePkg/UnitTests/Mock/Include/GoogleTest/Library/MockDeviceBootManagerLib.h @@ -0,0 +1,66 @@ +/** @file MockDeviceBootManagerLib.h + Google Test mocks for DeviceBootManagerLib. + + Copyright (c) Microsoft Corporation. + Your use of this software is governed by the terms of the Microsoft agreement under which you obtained the software. +**/ + +#ifndef MOCK_DEVICE_BOOT_MANAGER_LIB_H_ +#define MOCK_DEVICE_BOOT_MANAGER_LIB_H_ + +#include +#include + +extern "C" { + #include + #include +} + +struct MockDeviceBootManagerLib { + MOCK_INTERFACE_DECLARATION (MockDeviceBootManagerLib); + + MOCK_FUNCTION_DECLARATION ( + EFI_STATUS, + DeviceBootManagerPriorityBoot, + (EFI_BOOT_MANAGER_LOAD_OPTION *BootOption) + ); + + MOCK_FUNCTION_DECLARATION ( + VOID, + DeviceBootManagerUnableToBoot, + () + ); + + MOCK_FUNCTION_DECLARATION ( + VOID, + DeviceBootManagerProcessBootCompletion, + (EFI_BOOT_MANAGER_LOAD_OPTION *BootOption) + ); + + MOCK_FUNCTION_DECLARATION ( + EFI_DEVICE_PATH_PROTOCOL **, + DeviceBootManagerOnDemandConInConnect, + () + ); + + MOCK_FUNCTION_DECLARATION ( + VOID, + DeviceBootManagerBdsEntry, + () + ); + + MOCK_FUNCTION_DECLARATION ( + EFI_HANDLE, + DeviceBootManagerBeforeConsole, + (EFI_DEVICE_PATH_PROTOCOL **DevicePath, + BDS_CONSOLE_CONNECT_ENTRY **PlatformConsoles) + ); + + MOCK_FUNCTION_DECLARATION ( + EFI_DEVICE_PATH_PROTOCOL **, + DeviceBootManagerAfterConsole, + () + ); +}; + +#endif diff --git a/MsCorePkg/UnitTests/Mock/Library/MockDeviceBootManagerLib/MockDeviceBootManagerLib.cpp b/MsCorePkg/UnitTests/Mock/Library/MockDeviceBootManagerLib/MockDeviceBootManagerLib.cpp new file mode 100644 index 0000000000..ea0e33bb43 --- /dev/null +++ b/MsCorePkg/UnitTests/Mock/Library/MockDeviceBootManagerLib/MockDeviceBootManagerLib.cpp @@ -0,0 +1,17 @@ +/** @file MockDeviceBootManagerLib.cpp + Google Test mocks for DeviceBootManagerLib. + + Copyright (c) Microsoft Corporation. + Your use of this software is governed by the terms of the Microsoft agreement under which you obtained the software. +**/ + +#include + +MOCK_INTERFACE_DEFINITION (MockDeviceBootManagerLib); +MOCK_FUNCTION_DEFINITION (MockDeviceBootManagerLib, DeviceBootManagerPriorityBoot, 1, EFIAPI); +MOCK_FUNCTION_DEFINITION (MockDeviceBootManagerLib, DeviceBootManagerUnableToBoot, 0, EFIAPI); +MOCK_FUNCTION_DEFINITION (MockDeviceBootManagerLib, DeviceBootManagerProcessBootCompletion, 1, EFIAPI); +MOCK_FUNCTION_DEFINITION (MockDeviceBootManagerLib, DeviceBootManagerOnDemandConInConnect, 0, EFIAPI); +MOCK_FUNCTION_DEFINITION (MockDeviceBootManagerLib, DeviceBootManagerBdsEntry, 0, EFIAPI); +MOCK_FUNCTION_DEFINITION (MockDeviceBootManagerLib, DeviceBootManagerBeforeConsole, 2, EFIAPI); +MOCK_FUNCTION_DEFINITION (MockDeviceBootManagerLib, DeviceBootManagerAfterConsole, 0, EFIAPI); diff --git a/MsCorePkg/UnitTests/Mock/Library/MockDeviceBootManagerLib/MockDeviceBootManagerLib.inf b/MsCorePkg/UnitTests/Mock/Library/MockDeviceBootManagerLib/MockDeviceBootManagerLib.inf new file mode 100644 index 0000000000..1ca29a3efe --- /dev/null +++ b/MsCorePkg/UnitTests/Mock/Library/MockDeviceBootManagerLib/MockDeviceBootManagerLib.inf @@ -0,0 +1,34 @@ +## @file MockDeviceBootManagerLib.inf +# Google Test mocks for DeviceBootManagerLib. +# +# Copyright (c) Microsoft Corporation. +# Your use of this software is governed by the terms of the Microsoft agreement under which you obtained the software. +## + +[Defines] + INF_VERSION = 0x00010005 + BASE_NAME = MockDeviceBootManagerLib + FILE_GUID = A19D9B27-21EB-4DCF-A084-0326C2EB0E58 + MODULE_TYPE = HOST_APPLICATION + VERSION_STRING = 1.0 + LIBRARY_CLASS = DeviceBootManagerLib + +# +# The following information is for reference only and not required by the build tools. +# +# VALID_ARCHITECTURES = IA32 X64 +# + +[Sources] + MockDeviceBootManagerLib.cpp + +[Packages] + MdePkg/MdePkg.dec + MdeModulePkg/MdeModulePkg.dec + UnitTestFrameworkPkg/UnitTestFrameworkPkg.dec + MsCorePkg/MsCorePkg.dec +[LibraryClasses] + GoogleTestLib + +[BuildOptions] + MSFT:*_*_*_CC_FLAGS = /EHsc diff --git a/MsCorePkg/UnitTests/MsCorePkgHostTest.dsc b/MsCorePkg/UnitTests/MsCorePkgHostTest.dsc index 037e4d86c1..af365fe8d6 100644 --- a/MsCorePkg/UnitTests/MsCorePkgHostTest.dsc +++ b/MsCorePkg/UnitTests/MsCorePkgHostTest.dsc @@ -46,5 +46,7 @@ [Components] MsCorePkg/MacAddressEmulationDxe/Test/MacAddressEmulationDxeHostTest.inf + MsCorePkg/UnitTests/Mock/Library/MockDeviceBootManagerLib/MockDeviceBootManagerLib.inf + [BuildOptions] *_*_*_CC_FLAGS = -D DISABLE_NEW_DEPRECATED_INTERFACES