Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Relax report length requirements #455

Merged
merged 5 commits into from
Apr 18, 2024
Merged

Relax report length requirements #455

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0f37883
Relax report length checks to allow some non-conformant devices to fu…
joschock Mar 27, 2024
dae7bcb
Update unit tests for report length relaxation
joschock Mar 29, 2024
5764c41
Call layout update routine directly instead of using signal_event to …
joschock Mar 28, 2024
b6ed83f
Update unit tests for layout update behavior change
joschock Mar 29, 2024
d35ebcb
Merge branch 'release/202311' into relax_report_length_requirements
joschock Apr 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 64 additions & 45 deletions HidPkg/UefiHidDxeV2/src/keyboard/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ use crate::{
keyboard::key_queue::OrdKeyData,
};

use rust_advanced_logger_dxe::{debugln, function, DEBUG_ERROR, DEBUG_WARN};
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;
Expand Down Expand Up @@ -389,8 +389,8 @@ impl KeyboardHidHandler {
fn initialize_keyboard_layout(&mut self) -> Result<(), efi::Status> {
self.install_layout_change_event()?;

//signal event to pick up any existing layout
self.boot_services.signal_event(self.layout_change_event);
//fake signal event to pick up any existing layout
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() {
Expand Down Expand Up @@ -555,7 +555,18 @@ impl HidReportReceiver for KeyboardHidHandler {

if let Some(report_data) = self.input_reports.get(&report_id).cloned() {
if report.len() != report_data.report_size {
break 'report_processing;
//Some devices report extra bytes in their reports. Warn about this, but try and process anyway.
debugln!(
DEBUG_VERBOSE,
"{:?}:{:?} unexpected report length for report_id: {:?}. expected {:?}, actual {:?}",
function!(),
line!(),
report_id,
report_data.report_size,
report.len()
);
debugln!(DEBUG_VERBOSE, "report: {:x?}", report);
//break 'report_processing;
}

//reset currently active keys to empty set.
Expand Down Expand Up @@ -828,8 +839,11 @@ mod test {
boot_services.expect_create_event().returning(|_, _, _, _, _| efi::Status::SUCCESS);
boot_services.expect_create_event_ex().returning(|_, _, _, _, _, _| efi::Status::SUCCESS);
boot_services.expect_install_protocol_interface().returning(|_, _, _, _| efi::Status::SUCCESS);
boot_services.expect_locate_protocol().returning(|_, _, _| efi::Status::NOT_FOUND);
boot_services.expect_signal_event().returning(|_| efi::Status::SUCCESS);
boot_services.expect_open_protocol().returning(|_, _, _, _, _, _| efi::Status::NOT_FOUND);
boot_services.expect_raise_tpl().returning(|_| efi::TPL_APPLICATION);
boot_services.expect_restore_tpl().returning(|_| ());

let mut keyboard_handler = KeyboardHidHandler::new(boot_services, 1 as efi::Handle);
let mut hid_io = MockHidIo::new();
Expand All @@ -848,6 +862,7 @@ mod test {
boot_services.expect_create_event().returning(|_, _, _, _, _| efi::Status::SUCCESS);
boot_services.expect_create_event_ex().returning(|_, _, _, _, _, _| efi::Status::SUCCESS);
boot_services.expect_install_protocol_interface().returning(|_, _, _, _| efi::Status::SUCCESS);
boot_services.expect_locate_protocol().returning(|_, _, _| efi::Status::NOT_FOUND);
boot_services.expect_signal_event().returning(|_| efi::Status::SUCCESS);
boot_services.expect_open_protocol().returning(|_, _, _, _, _, _| efi::Status::NOT_FOUND);
boot_services.expect_raise_tpl().returning(|_| efi::TPL_APPLICATION);
Expand Down Expand Up @@ -981,6 +996,47 @@ mod test {
efi::Status::SUCCESS
}

const TEST_KEYBOARD_GUID: efi::Guid =
efi::Guid::from_fields(0xf1796c10, 0xdafb, 0x4989, 0xa0, 0x82, &[0x75, 0xe9, 0x65, 0x76, 0xbe, 0x52]);

static mut TEST_KEYBOARD_LAYOUT: HiiKeyboardLayout =
HiiKeyboardLayout { keys: Vec::new(), guid: TEST_KEYBOARD_GUID, descriptions: Vec::new() };
unsafe {
//make a test keyboard layout that is different than the default.
TEST_KEYBOARD_LAYOUT = hii_keyboard_layout::get_default_keyboard_layout();
TEST_KEYBOARD_LAYOUT.guid = TEST_KEYBOARD_GUID;
TEST_KEYBOARD_LAYOUT.keys.pop();
TEST_KEYBOARD_LAYOUT.keys.pop();
TEST_KEYBOARD_LAYOUT.keys.pop();
TEST_KEYBOARD_LAYOUT.descriptions[0].description = "Test Keyboard Layout".to_string();
TEST_KEYBOARD_LAYOUT.descriptions[0].language = "ts-TS".to_string();
}

extern "efiapi" fn get_keyboard_layout(
_this: *const protocols::hii_database::Protocol,
_key_guid: *const efi::Guid,
keyboard_layout_length: *mut u16,
keyboard_layout_ptr: *mut protocols::hii_database::KeyboardLayout,
) -> efi::Status {
let mut keyboard_layout_buffer = vec![0u8; 4096];
let buffer_size = keyboard_layout_buffer.pwrite(unsafe { &TEST_KEYBOARD_LAYOUT }, 0).unwrap();
keyboard_layout_buffer.resize(buffer_size, 0);
unsafe {
if keyboard_layout_length.read() < buffer_size as u16 {
keyboard_layout_length.write(buffer_size as u16);
return efi::Status::BUFFER_TOO_SMALL;
} else {
if keyboard_layout_ptr.is_null() {
panic!("bad keyboard pointer)");
}
keyboard_layout_length.write(buffer_size as u16);
let slice = from_raw_parts_mut(keyboard_layout_ptr as *mut u8, buffer_size);
slice.copy_from_slice(&keyboard_layout_buffer);
return efi::Status::SUCCESS;
}
}
}

extern "efiapi" fn set_keyboard_layout(
_this: *const protocols::hii_database::Protocol,
_key_guid: *mut efi::Guid,
Expand All @@ -990,47 +1046,6 @@ mod test {
boot_services.expect_restore_tpl().returning(|_| ());
boot_services.expect_signal_event().returning(|_| efi::Status::SUCCESS);

const TEST_KEYBOARD_GUID: efi::Guid =
efi::Guid::from_fields(0xf1796c10, 0xdafb, 0x4989, 0xa0, 0x82, &[0x75, 0xe9, 0x65, 0x76, 0xbe, 0x52]);

static mut TEST_KEYBOARD_LAYOUT: HiiKeyboardLayout =
HiiKeyboardLayout { keys: Vec::new(), guid: TEST_KEYBOARD_GUID, descriptions: Vec::new() };
unsafe {
//make a test keyboard layout that is different than the default.
TEST_KEYBOARD_LAYOUT = hii_keyboard_layout::get_default_keyboard_layout();
TEST_KEYBOARD_LAYOUT.guid = TEST_KEYBOARD_GUID;
TEST_KEYBOARD_LAYOUT.keys.pop();
TEST_KEYBOARD_LAYOUT.keys.pop();
TEST_KEYBOARD_LAYOUT.keys.pop();
TEST_KEYBOARD_LAYOUT.descriptions[0].description = "Test Keyboard Layout".to_string();
TEST_KEYBOARD_LAYOUT.descriptions[0].language = "ts-TS".to_string();
}

extern "efiapi" fn get_keyboard_layout(
_this: *const protocols::hii_database::Protocol,
_key_guid: *const efi::Guid,
keyboard_layout_length: *mut u16,
keyboard_layout_ptr: *mut protocols::hii_database::KeyboardLayout,
) -> efi::Status {
let mut keyboard_layout_buffer = vec![0u8; 4096];
let buffer_size = keyboard_layout_buffer.pwrite(unsafe { &TEST_KEYBOARD_LAYOUT }, 0).unwrap();
keyboard_layout_buffer.resize(buffer_size, 0);
unsafe {
if keyboard_layout_length.read() < buffer_size as u16 {
keyboard_layout_length.write(buffer_size as u16);
return efi::Status::BUFFER_TOO_SMALL;
} else {
if keyboard_layout_ptr.is_null() {
panic!("bad keyboard pointer)");
}
keyboard_layout_length.write(buffer_size as u16);
let slice = from_raw_parts_mut(keyboard_layout_ptr as *mut u8, buffer_size);
slice.copy_from_slice(&keyboard_layout_buffer);
return efi::Status::SUCCESS;
}
}
}

boot_services.expect_locate_protocol().returning(|protocol, _, interface| {
unsafe {
match *protocol {
Expand Down Expand Up @@ -1062,6 +1077,7 @@ mod test {
let mut hii_database = hii_database.assume_init();
hii_database.new_package_list = new_package_list;
hii_database.set_keyboard_layout = set_keyboard_layout;
hii_database.get_keyboard_layout = get_keyboard_layout;

interface.write(Box::into_raw(Box::new(hii_database)) as *mut c_void);
}
Expand Down Expand Up @@ -1089,6 +1105,7 @@ mod test {
boot_services.expect_create_event().returning(|_, _, _, _, _| efi::Status::SUCCESS);
boot_services.expect_create_event_ex().returning(|_, _, _, _, _, _| efi::Status::SUCCESS);
boot_services.expect_install_protocol_interface().returning(|_, _, _, _| efi::Status::SUCCESS);
boot_services.expect_locate_protocol().returning(|_, _, _| efi::Status::NOT_FOUND);
boot_services.expect_signal_event().returning(|_| efi::Status::SUCCESS);
boot_services.expect_open_protocol().returning(|_, _, _, _, _, _| efi::Status::NOT_FOUND);
boot_services.expect_raise_tpl().returning(|_| efi::TPL_APPLICATION);
Expand Down Expand Up @@ -1137,6 +1154,7 @@ mod test {
boot_services.expect_create_event().returning(|_, _, _, _, _| efi::Status::SUCCESS);
boot_services.expect_create_event_ex().returning(|_, _, _, _, _, _| efi::Status::SUCCESS);
boot_services.expect_install_protocol_interface().returning(|_, _, _, _| efi::Status::SUCCESS);
boot_services.expect_locate_protocol().returning(|_, _, _| efi::Status::NOT_FOUND);
boot_services.expect_signal_event().returning(|_| efi::Status::SUCCESS);
boot_services.expect_open_protocol().returning(|_, _, _, _, _, _| efi::Status::NOT_FOUND);
boot_services.expect_raise_tpl().returning(|_| efi::TPL_APPLICATION);
Expand Down Expand Up @@ -1200,6 +1218,7 @@ mod test {
boot_services.expect_create_event().returning(|_, _, _, _, _| efi::Status::SUCCESS);
boot_services.expect_create_event_ex().returning(|_, _, _, _, _, _| efi::Status::SUCCESS);
boot_services.expect_install_protocol_interface().returning(|_, _, _, _| efi::Status::SUCCESS);
boot_services.expect_locate_protocol().returning(|_, _, _| efi::Status::NOT_FOUND);
boot_services.expect_signal_event().returning(|_| efi::Status::SUCCESS);
boot_services.expect_open_protocol().returning(|_, _, _, _, _, _| efi::Status::NOT_FOUND);
boot_services.expect_raise_tpl().returning(|_| efi::TPL_APPLICATION);
Expand Down
2 changes: 2 additions & 0 deletions HidPkg/UefiHidDxeV2/src/keyboard/simple_text_in.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -461,6 +461,7 @@ mod test {
boot_services.expect_create_event_ex().returning(|_, _, _, _, _, _| efi::Status::SUCCESS);
boot_services.expect_signal_event().returning(|_| efi::Status::SUCCESS);
boot_services.expect_open_protocol().returning(|_, _, _, _, _, _| efi::Status::NOT_FOUND);
boot_services.expect_locate_protocol().returning(|_, _, _| efi::Status::NOT_FOUND);

let mut keyboard_handler = KeyboardHidHandler::new(boot_services, 1 as efi::Handle);

Expand Down Expand Up @@ -586,6 +587,7 @@ mod test {

// used in install
boot_services.expect_create_event().returning(|_, _, _, _, _| efi::Status::SUCCESS);
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);
Expand Down
5 changes: 5 additions & 0 deletions HidPkg/UefiHidDxeV2/src/keyboard/simple_text_in_ex.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -599,6 +599,7 @@ mod test {
boot_services.expect_create_event_ex().returning(|_, _, _, _, _, _| efi::Status::SUCCESS);
boot_services.expect_signal_event().returning(|_| efi::Status::SUCCESS);
boot_services.expect_open_protocol().returning(|_, _, _, _, _, _| efi::Status::NOT_FOUND);
boot_services.expect_locate_protocol().returning(|_, _, _| efi::Status::NOT_FOUND);

let mut keyboard_handler = KeyboardHidHandler::new(boot_services, 1 as efi::Handle);

Expand Down Expand Up @@ -742,6 +743,7 @@ mod test {
CONTEXT_PTR.store(interface, core::sync::atomic::Ordering::SeqCst);
efi::Status::SUCCESS
});
boot_services.expect_locate_protocol().returning(|_, _, _| efi::Status::NOT_FOUND);

// used in set state
boot_services.expect_raise_tpl().returning(|_| efi::TPL_APPLICATION);
Expand Down Expand Up @@ -827,6 +829,7 @@ mod test {
CONTEXT_PTR.store(interface, core::sync::atomic::Ordering::SeqCst);
efi::Status::SUCCESS
});
boot_services.expect_locate_protocol().returning(|_, _, _| efi::Status::NOT_FOUND);

// used in read key stroke
boot_services.expect_raise_tpl().returning(|_| efi::TPL_APPLICATION);
Expand Down Expand Up @@ -967,6 +970,8 @@ mod test {
}
efi::Status::SUCCESS
});
boot_services.expect_locate_protocol().returning(|_, _, _| efi::Status::NOT_FOUND);

boot_services.expect_create_event_ex().returning(|_, _, _, _, _, _| efi::Status::SUCCESS);
boot_services.expect_raise_tpl().returning(|_| efi::TPL_APPLICATION);
boot_services.expect_restore_tpl().returning(|_| ());
Expand Down
33 changes: 22 additions & 11 deletions HidPkg/UefiHidDxeV2/src/pointer/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ use hidparser::{
};
use r_efi::{efi, protocols};

use rust_advanced_logger_dxe::{debugln, function, DEBUG_ERROR};
use rust_advanced_logger_dxe::{debugln, function, DEBUG_ERROR, DEBUG_VERBOSE};

use crate::{
boot_services::UefiBootServices,
Expand Down Expand Up @@ -262,7 +262,18 @@ impl HidReportReceiver for PointerHidHandler {

if let Some(report_data) = self.input_reports.get(&report_id).cloned() {
if report.len() != report_data.report_size {
break 'report_processing;
//Some devices report extra bytes in their reports. Warn about this, but try and process anyway.
debugln!(
DEBUG_VERBOSE,
"{:?}:{:?} unexpected report length for report_id: {:?}. expected {:?}, actual {:?}",
function!(),
line!(),
report_id,
report_data.report_size,
report.len()
);
debugln!(DEBUG_VERBOSE, "report: {:x?}", report);
//break 'report_processing;
}

// hand the report data to the handler for each relevant field for field-specific processing.
Expand Down Expand Up @@ -589,7 +600,7 @@ mod test {
}

#[test]
fn bad_reports_should_be_ignored() {
fn bad_reports_should_be_processed_with_best_effort() {
let boot_services = create_fake_static_boot_service();
static mut ABS_PTR_INTERFACE: *mut c_void = core::ptr::null_mut();

Expand Down Expand Up @@ -650,19 +661,19 @@ mod test {
pointer_handler.receive_report(report, &hid_io);

assert_eq!(pointer_handler.current_state.active_buttons, 0);
assert_eq!(pointer_handler.current_state.current_x, CENTER);
assert_eq!(pointer_handler.current_state.current_y, CENTER);
assert_eq!(pointer_handler.current_state.current_z, 0);
assert_eq!(pointer_handler.state_changed, false);
assert_eq!(pointer_handler.current_state.current_x, 0);
assert_eq!(pointer_handler.current_state.current_y, 4);
assert_eq!(pointer_handler.current_state.current_z, 4);
assert_eq!(pointer_handler.state_changed, true);

//report too short
let report: &[u8] = &[0x00, 0x10, 0x00, 0x10, 0x00, 0x10];
pointer_handler.receive_report(report, &hid_io);

assert_eq!(pointer_handler.current_state.active_buttons, 0);
assert_eq!(pointer_handler.current_state.current_x, CENTER);
assert_eq!(pointer_handler.current_state.current_y, CENTER);
assert_eq!(pointer_handler.current_state.current_z, 0);
assert_eq!(pointer_handler.state_changed, false);
assert_eq!(pointer_handler.current_state.current_x, 4);
assert_eq!(pointer_handler.current_state.current_y, 4);
assert_eq!(pointer_handler.current_state.current_z, 4);
assert_eq!(pointer_handler.state_changed, true);
}
}
Loading