Skip to content

Commit

Permalink
Relax report length requirements (#455)
Browse files Browse the repository at this point in the history 
## Description

This PR brings in two fixes to support a wider variety of devices, some
of which might not be completely self-consistent:
* Allow report lengths that don't match the report descriptor. Reports
that are shorter than the Report Descriptor specifies will be processed
for whatever fields are fully present. Reports that are longer than the
Report descriptor specifies will simply ignore the extra bytes in the
report.
* Move away from using `signal_event` to force keyboard layout
initialization, and install call the layout change routine directly.
This avoids introducing sequencing issues based on the TPL that the
keyboard is being initialized at, resulting in more deterministic
behavior.

- [x] Impacts functionality?
- [ ] Impacts security?
- [ ] Breaking change?
- [x] Includes tests?
- [ ] Includes documentation?

## How This Was Tested

Rust unit tests updated to cover new functionality all pass. Functional
testing on hardware with the changes also passes.

## Integration Instructions

N/A
  • Loading branch information
@joschock
joschock committed Apr 18, 2024
1 parent b748acb commit cd7ce3e
Show file tree
Hide file tree
Showing 4 changed files with 93 additions and 56 deletions.
109 changes: 64 additions & 45 deletions HidPkg/UefiHidDxeV2/src/keyboard/mod.rs
View file
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
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
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
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);
}
}

0 comments on commit cd7ce3e

Please sign in to comment.