From 5c48439a66f0281fee802a00aa3cd531e008fc99 Mon Sep 17 00:00:00 2001 From: nworbnhoj Date: Sat, 18 Jun 2022 21:02:21 +1000 Subject: [PATCH 01/12] reduce memory footprint of Dither::dither() --- services/gam/src/bitmap.rs | 41 +++++++++++++++++++------------------- 1 file changed, 21 insertions(+), 20 deletions(-) diff --git a/services/gam/src/bitmap.rs b/services/gam/src/bitmap.rs index 88077e241..605e382c7 100644 --- a/services/gam/src/bitmap.rs +++ b/services/gam/src/bitmap.rs @@ -20,7 +20,7 @@ use std::ops::Deref; * author: nworbnhoj */ -#[derive(Debug, Clone)] +#[derive(Debug)] pub struct Bitmap { pub bound: Rectangle, tile_size: Point, @@ -251,8 +251,8 @@ impl From<&Img> for Bitmap { }; let mut mosaic: Vec = Vec::new(); - let pixels = Dither::new(BURKES.to_vec()).dither(&image); - let mut px_index = 0; + let words = Dither::new(BURKES.to_vec()).dither(&image); + let mut wd_index = 0; let bits_per_word: i16 = BITS_PER_WORD.try_into().unwrap(); for t in 0..tile_count { let t_top = t * tile_height; @@ -266,15 +266,8 @@ impl From<&Img> for Bitmap { for y in t_top..=t_bottom { let mut x = t_left; while x <= t_right { - let mut word: usize = 0; - for w in 0..bits_per_word { - if (x + w) > t_right { - continue; - } - let color = pixels[px_index] as usize; - word = word | (color << w); - px_index += 1; - } + let word = words[wd_index]; + wd_index += 1; let anchor = Point::new(x.try_into().unwrap(), y.try_into().unwrap()); tile.set_word(anchor, word.try_into().unwrap()); x += bits_per_word; @@ -302,7 +295,7 @@ pub enum PixelType { * author: nworbnhoj */ -#[derive(Debug, Clone)] +#[derive(Debug)] pub struct Img { pixels: Vec, width: usize, @@ -360,7 +353,7 @@ impl Img { let y_scale = self_height as f32 / height as f32; let self_x_base = x_scale * 0.5; let self_y_base = y_scale * 0.5; - + // Pretabulate horizontal pixel positions let mut self_x_tab: Vec = Vec::with_capacity(width); for buf_x in 0..width { @@ -476,21 +469,29 @@ impl Dither { PixelColor::Light } } - pub fn dither(&mut self, image: &Img) -> Vec { - let (width, height, length) = image.size(); + pub fn dither(&mut self, image: &Img) -> Vec { + let bits_per_word: u32 = BITS_PER_WORD.try_into().unwrap(); + let (width, height, _) = image.size(); self.provision(width.try_into().unwrap()); - let mut pixels: Vec = Vec::with_capacity(length); + let mut words: Vec = Vec::with_capacity((1 + width / BITS_PER_WORD) * height); for y in 0..height { + let (mut w, mut word): (Word, Word) = (0, 0); for x in 0..width { - let pixel = match image.get(x, y) { + let color = match image.get(x, y) { Some(grey) => self.pixel(*grey), None => PixelColor::Dark, }; - pixels.push(pixel); + word = word | ((color as u32) << w); + w += 1; + if w >= bits_per_word { + words.push(word); + (w, word) = (0, 0); + } self.next(); } + words.push(word); } - pixels + words } } From f20e809d8c160f804f9b1e65ef0b7e646926c70b Mon Sep 17 00:00:00 2001 From: Sean Cross Date: Thu, 23 Jun 2022 23:51:39 +0800 Subject: [PATCH 02/12] xous: add AdjustProcessLimit syscall This syscal will be used to adjust various process limits. Currently it only contains the ability to adjust the heap size and maximum. Signed-off-by: Sean Cross --- xous-rs/src/definitions.rs | 6 +++++ xous-rs/src/definitions/limits.rs | 5 ++++ xous-rs/src/syscall.rs | 43 +++++++++++++++++++++++++++++++ 3 files changed, 54 insertions(+) create mode 100644 xous-rs/src/definitions/limits.rs diff --git a/xous-rs/src/definitions.rs b/xous-rs/src/definitions.rs index ba751c703..9f2a84f15 100644 --- a/xous-rs/src/definitions.rs +++ b/xous-rs/src/definitions.rs @@ -75,6 +75,9 @@ pub use memoryflags::*; pub mod messages; pub use messages::*; +pub mod limits; +pub use limits::*; + use crate::arch::ProcessStartup; /// Server ID @@ -194,6 +197,7 @@ pub enum Error { UseBeforeInit = 24, DoubleFree = 25, DebugInProgress = 26, + InvalidLimit = 27, } impl Error { @@ -226,6 +230,7 @@ impl Error { 24 => UseBeforeInit, 25 => DoubleFree, 26 => DebugInProgress, + 27 => InvalidLimit, _ => UnknownError, } } @@ -258,6 +263,7 @@ impl Error { UseBeforeInit => 24, DoubleFree => 25, DebugInProgress => 26, + InvalidLimit => 27, UnknownError => usize::MAX, } } diff --git a/xous-rs/src/definitions/limits.rs b/xous-rs/src/definitions/limits.rs new file mode 100644 index 000000000..fad084241 --- /dev/null +++ b/xous-rs/src/definitions/limits.rs @@ -0,0 +1,5 @@ +#[repr(usize)] +pub enum Limits { + HeapMaximum = 1, + HeapSize = 2, +} \ No newline at end of file diff --git a/xous-rs/src/syscall.rs b/xous-rs/src/syscall.rs index 6735f61e9..364d3ce00 100644 --- a/xous-rs/src/syscall.rs +++ b/xous-rs/src/syscall.rs @@ -427,6 +427,36 @@ pub enum SysCall { usize, /* stack pointer */ ), + /// Adjust one of the limits within this process. Note that you must pass + /// the current limit value in order to set the new limit. The current limit + /// value is always returned, so this function may need to be called twice -- + /// once in order to get the current limit, and again to set the new limit. + /// + /// ## Arguments + /// + /// * **Index**: The item to adjust. Currently the following limits + /// are supported: + /// 1: Maximum heap size + /// 2: Current heap size + /// * **Current Limit**: Pass the current limit value here. The current + /// limit must match in order for the new limit to take + /// effect. This is used to avoid a race condition if two + /// threads try to set the same limit. + /// * **Proposed Limit**: The new value that you would like to use. + /// + /// ## Returns + /// + /// Returns a Scalar2 containing `(Index, Limit)`. + /// + /// ## Errors + /// + /// * **InvalidLimit**: The specified index was not valid + AdjustProcessLimit( + usize, /* process limit index */ + usize, /* expected current limit */ + usize, /* proposed new limit */ + ), + /// This syscall does not exist. It captures all possible /// arguments so detailed analysis can be performed. Invalid(usize, usize, usize, usize, usize, usize, usize), @@ -470,6 +500,7 @@ pub enum SysCallNumber { Disconnect = 35, JoinThread = 36, SetExceptionHandler = 37, + AdjustProcessLimit = 38, Invalid, } @@ -513,6 +544,7 @@ impl SysCallNumber { 35 => Disconnect, 36 => JoinThread, 37 => SetExceptionHandler, + 38 => AdjustProcessLimit, _ => Invalid, } } @@ -867,6 +899,16 @@ impl SysCall { 0, 0, ], + SysCall::AdjustProcessLimit(index, current, new) => [ + SysCallNumber::AdjustProcessLimit as usize, + *index, + *current, + *new, + 0, + 0, + 0, + 0, + ], SysCall::Invalid(a1, a2, a3, a4, a5, a6, a7) => [ SysCallNumber::Invalid as usize, *a1, @@ -1030,6 +1072,7 @@ impl SysCall { SysCallNumber::Disconnect => SysCall::Disconnect(a1 as _), SysCallNumber::JoinThread => SysCall::JoinThread(a1 as _), SysCallNumber::SetExceptionHandler => SysCall::SetExceptionHandler(a1 as _, a2 as _), + SysCallNumber::AdjustProcessLimit => SysCall::AdjustProcessLimit(a1, a2, a3), SysCallNumber::Invalid => SysCall::Invalid(a1, a2, a3, a4, a5, a6, a7), }) } From 08958745ba055626ac10f1ed7c4a9a679bd8f038 Mon Sep 17 00:00:00 2001 From: Sean Cross Date: Thu, 23 Jun 2022 23:55:31 +0800 Subject: [PATCH 03/12] kernel: implement AdjustProcessLimit syscall Implement the syscall to adjust a process' limit. Currently this only supports adjusting the heap maximum and the heap size. In order to adjust a limit, you must first present the current limit. This can be obtained by requesting a bogus value. The current limit is returned as part of the result. Signed-off-by: Sean Cross --- kernel/src/syscall.rs | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/kernel/src/syscall.rs b/kernel/src/syscall.rs index 925af42d0..675001e2d 100644 --- a/kernel/src/syscall.rs +++ b/kernel/src/syscall.rs @@ -717,8 +717,9 @@ pub fn handle_inner(pid: PID, tid: TID, in_irq: bool, call: SysCall) -> SysCallR MemoryRange::new( start, // 0-length MemoryRanges are disallowed -- so return 4096 as the minimum in any case, even though it's a lie - if length == 0 { 4096 } else { length } - ).unwrap() + if length == 0 { 4096 } else { length }, + ) + .unwrap() })); } @@ -920,6 +921,21 @@ pub fn handle_inner(pid: PID, tid: TID, in_irq: bool, call: SysCall) -> SysCallR MemoryManager::with_mut(|mm| mm.update_memory_flags(range, flags))?; Ok(xous_kernel::Result::Ok) } + SysCall::AdjustProcessLimit(index, current, new) => match index { + 1 => arch::process::Process::with_inner_mut(|p| { + if p.mem_heap_max == current { + p.mem_heap_max = new; + } + Ok(xous_kernel::Result::Scalar2(index, p.mem_heap_max)) + }), + 2 => arch::process::Process::with_inner_mut(|p| { + if p.mem_heap_size == current && new < p.mem_heap_max { + p.mem_heap_size = new; + } + Ok(xous_kernel::Result::Scalar2(index, p.mem_heap_size)) + }), + _ => Err(xous_kernel::Error::InvalidLimit), + }, /* https://github.com/betrusted-io/xous-core/issues/90 SysCall::SetExceptionHandler(pc, sp) => SystemServices::with_mut(|ss| { ss.set_exception_handler(pid, pc, sp) From 79c20eb4982ac5777fe7a3aa0c76c7786a3bcf57 Mon Sep 17 00:00:00 2001 From: bunnie Date: Sun, 26 Jun 2022 17:37:57 +0800 Subject: [PATCH 04/12] fix in-place edit of keys The behavior with a secret basis is as follows: - The key stays in the basis in which it was created. - So, if you created k1 in .System basis, and k2 in Secret basis, if you edit k1 while Secret basis is open, k1 stays in .System basis. it does *not* migrate into Secret basis. I think this is the "least surprising" behavior. There is an argument that "maybe I want to keep secret notes in an overlay on a key" but I feel like creating a second shadow copy of a key with a different set of notes is not the most desired behavior for this application (NB: it could be desired for others applications, and that behavior can be programmed with the PDDB API; which is why the API is a bit explicit at the moment. This flexibility goes against the goal of "foolproof deniability", unfortunately, but I think it is too early to know which default to set for the system). --- apps/vault/src/actions.rs | 113 +++++++++++++++++++-------------- apps/vault/src/ux/fido.rs | 20 +++++- apps/vault/src/ux/framework.rs | 22 ++++++- 3 files changed, 104 insertions(+), 51 deletions(-) diff --git a/apps/vault/src/actions.rs b/apps/vault/src/actions.rs index 45a3851cf..fd6acf9d8 100644 --- a/apps/vault/src/actions.rs +++ b/apps/vault/src/actions.rs @@ -441,33 +441,46 @@ impl ActionManager { VaultMode::Fido => crate::fido::U2F_APP_DICT, VaultMode::Totp => VAULT_TOTP_DICT, }; - match self.pddb.borrow().delete_key(dict, entry.key_name.as_str().unwrap_or("UTF8-error"), None) { - Ok(_) => { - self.modals.show_notification(t!("vault.completed", xous::LANG), None).ok(); - } - Err(e) => { - match e.kind() { - std::io::ErrorKind::NotFound => { - // handle special case of FIDO which is two dicts combined - if entry.mode == VaultMode::Fido { - // try the "other" dictionary - match self.pddb.borrow() - .delete_key( - crate::ctap::FIDO_CRED_DICT, - entry.key_name.as_str().unwrap_or("UTF8-error"), - None) { - Ok(_) => { - self.modals.show_notification(t!("vault.completed", xous::LANG), None).ok(); - return; - } - _ => {} - } - } - self.report_err(t!("vault.error.not_found", xous::LANG), None::); + // first "get" the key, to resolve exactly what basis the key is in. This is because `delete_key()` will + // only look in the most recently unlocked secret basis, it won't automatically descend into the database + // and try to cull something willy-nilly. + match self.pddb.borrow().get(dict, entry.key_name.as_str().unwrap_or("UTF8-error"), + None, false, false, None, None:: + ) { + Ok(candidate) => { + let attr = candidate.attributes().expect("couldn't get key attributes"); + match self.pddb.borrow().delete_key(dict, + entry.key_name.as_str().unwrap_or("UTF8-error"), + Some(&attr.basis)) { + Ok(_) => { + self.modals.show_notification(t!("vault.completed", xous::LANG), None).ok(); + } + Err(e) => { + match e.kind() { + std::io::ErrorKind::NotFound => { + // handle special case of FIDO which is two dicts combined + if entry.mode == VaultMode::Fido { + // try the "other" dictionary + match self.pddb.borrow() + .delete_key( + crate::ctap::FIDO_CRED_DICT, + entry.key_name.as_str().unwrap_or("UTF8-error"), + Some(&attr.basis)) { + Ok(_) => { + self.modals.show_notification(t!("vault.completed", xous::LANG), None).ok(); + return; + } + _ => {} + } + } + self.report_err(t!("vault.error.not_found", xous::LANG), None::); + } + _ => self.report_err(t!("vault.error.internal_error", xous::LANG), Some(e)), + } } - _ => self.report_err(t!("vault.error.internal_error", xous::LANG), Some(e)), } } + Err(e) => self.report_err(t!("vault.error.internal_error", xous::LANG), Some(e)), } } } @@ -485,6 +498,8 @@ impl ActionManager { false, false, None, Some(crate::basis_change) ) { Ok(mut record) => { + // resolve the basis of the key, so that we are editing it "in place" + let attr = record.attributes().expect("couldn't get key attributes"); let mut data = Vec::::new(); let maybe_update = match record.read_to_end(&mut data) { Ok(_len) => { @@ -501,13 +516,13 @@ impl ActionManager { pw.password = edit_data.content()[2].content.as_str().unwrap().to_string(); pw.notes = edit_data.content()[3].content.as_str().unwrap().to_string(); pw.atime = utc_now().timestamp() as u64; - Some(pw) + pw } else { log::error!("record error"); - self.report_err(t!("vault.error.record_error", xous::LANG), None::); None } + self.report_err(t!("vault.error.record_error", xous::LANG), None::); return } } - Err(e) => { log::error!("internal error"); self.report_err(t!("vault.error.internal_error", xous::LANG), Some(e)); None } + Err(e) => { log::error!("internal error"); self.report_err(t!("vault.error.internal_error", xous::LANG), Some(e)); return } }; - maybe_update + Some((maybe_update, attr.basis)) } Err(e) => { match e.kind() { @@ -523,12 +538,12 @@ impl ActionManager { None } }; - if let Some(update) = maybe_update { - self.pddb.borrow().delete_key(dict, entry.key_name.as_str().unwrap(), None) + if let Some((update, basis)) = maybe_update { + self.pddb.borrow().delete_key(dict, entry.key_name.as_str().unwrap(), Some(&basis)) .unwrap_or_else(|e| {log::error!("internal error"); self.report_err(t!("vault.error.internal_error", xous::LANG), Some(e))}); match self.pddb.borrow().get( - dict, entry.key_name.as_str().unwrap(), None, + dict, entry.key_name.as_str().unwrap(), Some(&basis), false, true, Some(VAULT_ALLOC_HINT), Some(crate::basis_change) ) { @@ -555,6 +570,8 @@ impl ActionManager { false, false, None, Some(crate::basis_change) ) { Ok(mut record) => { + // resolve the basis of the key, so that we are editing it "in place" + let attr = record.attributes().expect("couldn't get key attributes"); let mut data = Vec::::new(); let maybe_update = match record.read_to_end(&mut data) { Ok(_len) => { @@ -568,26 +585,26 @@ impl ActionManager { ai.name = edit_data.content()[0].content.as_str().unwrap().to_string(); ai.notes = edit_data.content()[1].content.as_str().unwrap().to_string(); ai.atime = utc_now().timestamp() as u64; - Some(ai) - } else { self.report_err(t!("vault.error.record_error", xous::LANG), None::); None } + ai + } else { self.report_err(t!("vault.error.record_error", xous::LANG), None::); return } } - Err(e) => { self.report_err(t!("vault.error.internal_error", xous::LANG), Some(e)); None } + Err(e) => { self.report_err(t!("vault.error.internal_error", xous::LANG), Some(e)); return } }; - maybe_update + Some((maybe_update, attr.basis)) } Err(e) => { match e.kind() { std::io::ErrorKind::NotFound => self.report_err(t!("vault.error.fido2", xous::LANG), None::), _ => self.report_err(t!("vault.error.internal_error", xous::LANG), Some(e)), } - None + return } }; - if let Some(update) = maybe_update { - self.pddb.borrow().delete_key(dict, entry.key_name.as_str().unwrap(), None) + if let Some((update, basis)) = maybe_update { + self.pddb.borrow().delete_key(dict, entry.key_name.as_str().unwrap(), Some(&basis)) .unwrap_or_else(|e| self.report_err(t!("vault.error.internal_error", xous::LANG), Some(e))); match self.pddb.borrow().get( - dict, entry.key_name.as_str().unwrap(), None, + dict, entry.key_name.as_str().unwrap(), Some(&basis), false, true, Some(VAULT_ALLOC_HINT), Some(crate::basis_change) ) { @@ -607,6 +624,8 @@ impl ActionManager { false, false, None, Some(crate::basis_change) ) { Ok(mut record) => { + // resolve the basis of the key, so that we are editing it "in place" + let attr = record.attributes().expect("couldn't get key attributes"); let mut data = Vec::::new(); let maybe_update = match record.read_to_end(&mut data) { Ok(_len) => { @@ -633,26 +652,26 @@ impl ActionManager { if let Ok(d) = u32::from_str_radix(edit_data.content()[5].content.as_str().unwrap(), 10) { pw.digits = d; } - Some(pw) - } else { self.report_err(t!("vault.error.record_error", xous::LANG), None::); None } + pw + } else { self.report_err(t!("vault.error.record_error", xous::LANG), None::); return } } - Err(e) => { self.report_err(t!("vault.error.internal_error", xous::LANG), Some(e)); None } + Err(e) => { self.report_err(t!("vault.error.internal_error", xous::LANG), Some(e)); return } }; - maybe_update + Some((maybe_update, attr.basis)) } Err(e) => { match e.kind() { std::io::ErrorKind::NotFound => self.report_err(t!("vault.error.not_found", xous::LANG), None::), _ => self.report_err(t!("vault.error.internal_error", xous::LANG), Some(e)), } - None + return } }; - if let Some(update) = maybe_update { - self.pddb.borrow().delete_key(dict, entry.key_name.as_str().unwrap(), None) + if let Some((update, basis)) = maybe_update { + self.pddb.borrow().delete_key(dict, entry.key_name.as_str().unwrap(), Some(&basis)) .unwrap_or_else(|e| self.report_err(t!("vault.error.internal_error", xous::LANG), Some(e))); match self.pddb.borrow().get( - dict, entry.key_name.as_str().unwrap(), None, + dict, entry.key_name.as_str().unwrap(), Some(&basis), false, true, Some(VAULT_ALLOC_HINT), Some(crate::basis_change) ) { diff --git a/apps/vault/src/ux/fido.rs b/apps/vault/src/ux/fido.rs index a78fb5b8e..d7a8890fc 100644 --- a/apps/vault/src/ux/fido.rs +++ b/apps/vault/src/ux/fido.rs @@ -457,12 +457,28 @@ pub(crate) fn start_fido_ux_thread() { } } }; - pddb.delete_key(U2F_APP_DICT, &app_id_str, None).ok(); - match pddb.get( + // this get determines which basis the key is in + let basis = match pddb.get( U2F_APP_DICT, &app_id_str, None, true, true, Some(256), Some(crate::basis_change) + ) { + Ok(app_data) => { + let attr = app_data.attributes().expect("couldn't get attributes"); + attr.basis + } + Err(e) => { + log::error!("error updating app atime: {:?}", e); + continue; + } + }; + pddb.delete_key(U2F_APP_DICT, &app_id_str, Some(&basis)).ok(); + match pddb.get( + U2F_APP_DICT, + &app_id_str, + Some(&basis), true, true, + Some(256), Some(crate::basis_change) ) { Ok(mut app_data) => { app_data.write(&ser).expect("couldn't update atime"); diff --git a/apps/vault/src/ux/framework.rs b/apps/vault/src/ux/framework.rs index 257592d4a..e66a98941 100644 --- a/apps/vault/src/ux/framework.rs +++ b/apps/vault/src/ux/framework.rs @@ -686,14 +686,32 @@ impl VaultUx { return Err(xous::Error::ProcessNotFound); } }; - match self.pddb.borrow().delete_key(crate::actions::VAULT_PASSWORD_DICT, entry, None) { + + // this get determines which basis the key is in + let basis = match self.pddb.borrow().get( + crate::actions::VAULT_PASSWORD_DICT, + entry, + None, true, true, + Some(256), Some(crate::basis_change) + ) { + Ok(app_data) => { + let attr = app_data.attributes().expect("couldn't get attributes"); + attr.basis + } + Err(e) => { + log::error!("error updating key atime: {:?}", e); + return Err(xous::Error::InternalError); + } + }; + + match self.pddb.borrow().delete_key(crate::actions::VAULT_PASSWORD_DICT, entry, Some(&basis)) { Ok(_) => {} Err(_e) => { return Err(xous::Error::InternalError); } } match self.pddb.borrow().get( - crate::actions::VAULT_PASSWORD_DICT, entry, None, + crate::actions::VAULT_PASSWORD_DICT, entry, Some(&basis), false, true, Some(crate::actions::VAULT_ALLOC_HINT), Some(crate::basis_change) ) { From 9cf48fcaa5112b495f1eee5e78c847cafd795db6 Mon Sep 17 00:00:00 2001 From: bunnie Date: Mon, 27 Jun 2022 00:04:46 +0800 Subject: [PATCH 05/12] add storage limits on FIDO cred count perfomance regression on the PDDB with the latest fixes make the device stop passing compliance when the database grows to about 40-ish keys. For now, we set the cred limit to 32, until the performance issues are addressed. (note that yubico has a limit of 25 credentials, so, 32 is not insanely low). --- apps/vault/src/ctap/storage.rs | 17 ++++++++++++----- apps/vault/src/main.rs | 5 +---- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/apps/vault/src/ctap/storage.rs b/apps/vault/src/ctap/storage.rs index 0bc97a524..7b9f8d27c 100644 --- a/apps/vault/src/ctap/storage.rs +++ b/apps/vault/src/ctap/storage.rs @@ -65,8 +65,9 @@ pub(crate) const CRED_INITAL_SIZE: usize = 512; // With P=20 and K=150, we have I=2M which is enough for 500 increments per day for 10 years. #[allow(dead_code)] // openSK legacy const NUM_PAGES: usize = 20; -#[allow(dead_code)] // OpenSK legacy -const MAX_SUPPORTED_RESIDENTIAL_KEYS: usize = 150; +// The limit is based on the performance of the PDDB to get a query response within +// the compliance time-out limit. Optimization is needed to improve this number! +const MAX_SUPPORTED_RESIDENTIAL_KEYS: usize = 32; const MAX_PIN_RETRIES: u8 = 8; #[cfg(feature = "with_ctap2_1")] @@ -389,6 +390,7 @@ openssl asn1parse -in opensk_cert.pem -inform pem Some(CREDENTIAL_ID_SIZE), Some(crate::basis_change) ) { Ok(mut cred) => { + log::trace!("find {}", shortid); let mut data = Vec::::new(); cred.read_to_end(&mut data).or(Err(Ctap2StatusCode::CTAP2_ERR_VENDOR_INTERNAL_ERROR))?; match deserialize_credential(&data) { @@ -428,6 +430,9 @@ openssl asn1parse -in opensk_cert.pem -inform pem &mut self, new_credential: PublicKeyCredentialSource, ) -> Result<(), Ctap2StatusCode> { + if self.count_credentials()? >= MAX_SUPPORTED_RESIDENTIAL_KEYS { + return Err(Ctap2StatusCode::CTAP2_ERR_KEY_STORE_FULL); + } let shortid = self.cid_to_str(&new_credential.credential_id); match self.pddb.borrow().get( FIDO_CRED_DICT, @@ -437,6 +442,7 @@ openssl asn1parse -in opensk_cert.pem -inform pem ) { Ok(mut cred) => { let value = serialize_credential(new_credential)?; + log::trace!("writing {}", shortid); cred.write(&value) .or(Err(Ctap2StatusCode::CTAP2_ERR_VENDOR_INTERNAL_ERROR))?; self.pddb.borrow().sync() @@ -465,11 +471,13 @@ openssl asn1parse -in opensk_cert.pem -inform pem None, false, false, Some(CREDENTIAL_ID_SIZE), Some(crate::basis_change) ).ok() { + log::trace!("checking {}", cred_name); let mut data = Vec::::new(); cred_entry.read_to_end(&mut data).or(Err(Ctap2StatusCode::CTAP2_ERR_VENDOR_INTERNAL_ERROR))?; if let Some(cred) = deserialize_credential(&data) { if cred.rp_id == rp_id && (cred.is_discoverable() || !check_cred_protect) { + log::trace!("filtered {}", cred_name); result.push(cred); } } @@ -479,9 +487,8 @@ openssl asn1parse -in opensk_cert.pem -inform pem } /// Returns the number of credentials. - #[cfg(test)] pub fn count_credentials(&self) -> Result { - let mut cred_list = self.pddb.borrow().list_keys( + let cred_list = self.pddb.borrow().list_keys( FIDO_CRED_DICT, None).unwrap_or(Vec::new()); Ok(cred_list.len()) } @@ -615,7 +622,7 @@ openssl asn1parse -in opensk_cert.pem -inform pem let mut pin_hash = [0u8; PIN_AUTH_LENGTH]; match ph.read(&mut pin_hash) { Ok(PIN_AUTH_LENGTH) => { - log::info!("pin_hash: {:x?}", &pin_hash); + log::trace!("pin_hash: {:x?}", &pin_hash); Ok(Some(pin_hash)) }, Ok(l) => { diff --git a/apps/vault/src/main.rs b/apps/vault/src/main.rs index eec47bff9..61adf217f 100644 --- a/apps/vault/src/main.rs +++ b/apps/vault/src/main.rs @@ -108,9 +108,6 @@ To clear test entries: pddb dictdelete fido.cred pddb dictdelete fido.u2fapps -Issue: editing an entry in a non-secret basis while a secret basis is open won't work because -the delete key function doesn't work in that case. Need to modify the `get` routine to return -the basis so we can specify which basis to edit... */ #[derive(Debug, num_derive::FromPrimitive, num_derive::ToPrimitive)] @@ -161,7 +158,7 @@ const ERR_TIMEOUT_MS: usize = 5000; fn main() -> ! { log_server::init_wait().unwrap(); - log::set_max_level(log::LevelFilter::Debug); + log::set_max_level(log::LevelFilter::Info); log::info!("my PID is {}", xous::process::id()); let xns = xous_names::XousNames::new().unwrap(); From 0e5fb14894c1fce67ffa97237c75ebfe4c347ef5 Mon Sep 17 00:00:00 2001 From: bunnie Date: Mon, 27 Jun 2022 02:00:22 +0800 Subject: [PATCH 06/12] add a compatibility gate on USB HID interface If the SoC isn't compatible, don't try starting the U2F server. --- apps/vault/src/main.rs | 61 +++++++++++++++------------- services/usb-device-xous/src/api.rs | 3 ++ services/usb-device-xous/src/lib.rs | 18 ++++++++ services/usb-device-xous/src/main.rs | 6 +++ 4 files changed, 60 insertions(+), 28 deletions(-) diff --git a/apps/vault/src/main.rs b/apps/vault/src/main.rs index 61adf217f..82118d9f3 100644 --- a/apps/vault/src/main.rs +++ b/apps/vault/src/main.rs @@ -192,41 +192,46 @@ fn main() -> ! { let mut rng = ctap_crypto::rng256::XousRng256::new(&xns); // this call will block until the PDDB is mounted. let usb = usb_device_xous::UsbHid::new(); - let mut ctap_state = CtapState::new(&mut rng, check_user_presence, boot_time); - let mut ctap_hid = CtapHid::new(); - loop { - match usb.u2f_wait_incoming() { - Ok(msg) => { - log::trace!("FIDO listener got message: {:?}", msg); - let now = ClockValue::new(tt.elapsed_ms() as i64, 1000); - let reply = ctap_hid.process_hid_packet(&msg.packet, now, &mut ctap_state); - // This block handles sending packets. - for pkt_reply in reply { - let mut reply = RawFidoMsg::default(); - reply.packet.copy_from_slice(&pkt_reply); - let status = usb.u2f_send(reply); - match status { - Ok(()) => { - log::trace!("Sent U2F packet"); - } - Err(e) => { - log::error!("Error sending U2F packet: {:?}", e); + // only run the main loop if the SoC is compatible + if usb.is_soc_compatible() { + let mut ctap_state = CtapState::new(&mut rng, check_user_presence, boot_time); + let mut ctap_hid = CtapHid::new(); + loop { + match usb.u2f_wait_incoming() { + Ok(msg) => { + log::trace!("FIDO listener got message: {:?}", msg); + let now = ClockValue::new(tt.elapsed_ms() as i64, 1000); + let reply = ctap_hid.process_hid_packet(&msg.packet, now, &mut ctap_state); + // This block handles sending packets. + for pkt_reply in reply { + let mut reply = RawFidoMsg::default(); + reply.packet.copy_from_slice(&pkt_reply); + let status = usb.u2f_send(reply); + match status { + Ok(()) => { + log::trace!("Sent U2F packet"); + } + Err(e) => { + log::error!("Error sending U2F packet: {:?}", e); + } } } } - } - Err(e) => { - match e { - xous::Error::ProcessTerminated => { // unplug happened, reset the authenticator - log::info!("CTAP unplug_reset"); - ctap_state.unplug_reset(); - }, - _ => { - log::warn!("FIDO listener got an error: {:?}", e); + Err(e) => { + match e { + xous::Error::ProcessTerminated => { // unplug happened, reset the authenticator + log::info!("CTAP unplug_reset"); + ctap_state.unplug_reset(); + }, + _ => { + log::warn!("FIDO listener got an error: {:?}", e); + } } } } } + } else { + log::warn!("SoC rev is incompatible with USB HID operations, the U2F/FIDO2 server is not started"); } } }); diff --git a/services/usb-device-xous/src/api.rs b/services/usb-device-xous/src/api.rs index 4782384ac..bcd55b93c 100644 --- a/services/usb-device-xous/src/api.rs +++ b/services/usb-device-xous/src/api.rs @@ -28,6 +28,9 @@ pub(crate) enum Opcode { /// Blocks the caller, waiting for a U2F message U2fRxDeferred, + /// Query if the HID driver was able to start + IsSocCompatible, + /// Handle the USB interrupt UsbIrqHandler, /// Suspend/resume callback diff --git a/services/usb-device-xous/src/lib.rs b/services/usb-device-xous/src/lib.rs index ca9983174..20078203e 100644 --- a/services/usb-device-xous/src/lib.rs +++ b/services/usb-device-xous/src/lib.rs @@ -29,6 +29,24 @@ impl UsbHid { conn } } + /// used to query if the HID core was able to start. Mainly to handle edge cases between updates. + pub fn is_soc_compatible(&self) -> bool { + match send_message( + self.conn, + Message::new_blocking_scalar( + Opcode::IsSocCompatible.to_usize().unwrap(), + 0, 0, 0, 0 + ) + ) { + Ok(xous::Result::Scalar1(code)) => { + match code { + 0 => false, + _ => true + } + } + _ => panic!("Internal error: illegal return type"), + } + } /// this will always trigger a reset, even if it's the same core we're switching to pub fn switch_to_core(&self, core: UsbDeviceType) -> Result<(), xous::Error> { match send_message( diff --git a/services/usb-device-xous/src/main.rs b/services/usb-device-xous/src/main.rs index 4fea3bfcd..b234b8398 100644 --- a/services/usb-device-xous/src/main.rs +++ b/services/usb-device-xous/src/main.rs @@ -112,6 +112,9 @@ fn main() -> ! { // block any rx requests forever fido_listener = Some(msg); } + Some(Opcode::IsSocCompatible) => msg_blocking_scalar_unpack!(msg, _, _, _, _, { + xous::return_scalar(msg.sender, 0).expect("couldn't return compatibility status") + }), Some(Opcode::Quit) => { break; } @@ -200,6 +203,9 @@ fn main() -> ! { usbmgmt.xous_resume(); lockstatus_force_update = true; // notify the status bar that yes, it does need to redraw the lock status, even if the value hasn't changed since the last read }), + Some(Opcode::IsSocCompatible) => msg_blocking_scalar_unpack!(msg, _, _, _, _, { + xous::return_scalar(msg.sender, 1).expect("couldn't return compatibility status") + }), Some(Opcode::U2fRxDeferred) => { if fido_listener_pid.is_none() { fido_listener_pid = msg.sender.pid(); From a7ce3e62047da1673d1151cd5e5dd7d0a3635b6a Mon Sep 17 00:00:00 2001 From: bunnie Date: Mon, 27 Jun 2022 02:18:55 +0800 Subject: [PATCH 07/12] make keyboard tick() blocks non-fatal at the moment if for some reason the USB stack can't send a tick there's a panic. it's not actually a life-ending event if that happens. --- services/usb-device-xous/src/main.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/services/usb-device-xous/src/main.rs b/services/usb-device-xous/src/main.rs index b234b8398..31f335d7e 100644 --- a/services/usb-device-xous/src/main.rs +++ b/services/usb-device-xous/src/main.rs @@ -180,7 +180,7 @@ fn main() -> ! { { let keyboard = composite.interface::, _>(); keyboard.write_report(&Vec::::new()).ok(); - keyboard.tick().unwrap(); + keyboard.tick().ok(); } #[cfg(any(target_os = "none", target_os = "xous"))] @@ -414,11 +414,11 @@ fn main() -> ! { { let keyboard = composite.interface::, _>(); keyboard.write_report(&codes).ok(); - keyboard.tick().unwrap(); + keyboard.tick().ok(); tt.sleep_ms(30).ok(); if auto_up { keyboard.write_report(&[]).ok(); // this is the key-up - keyboard.tick().unwrap(); + keyboard.tick().ok(); tt.sleep_ms(30).ok(); } } @@ -450,10 +450,10 @@ fn main() -> ! { { let keyboard = composite.interface::, _>(); keyboard.write_report(&codes).ok(); - keyboard.tick().unwrap(); + keyboard.tick().ok(); tt.sleep_ms(30).ok(); keyboard.write_report(&[]).ok(); // this is the key-up - keyboard.tick().unwrap(); + keyboard.tick().ok(); tt.sleep_ms(30).ok(); } sent += 1; From ed4f7b2092445c6553fb08ba65ad7187b6f0161e Mon Sep 17 00:00:00 2001 From: bunnie Date: Mon, 27 Jun 2022 02:33:23 +0800 Subject: [PATCH 08/12] restrict debug access when running vault app this removes the security message about the debug core being unlocked --- apps/vault/src/ux/framework.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/apps/vault/src/ux/framework.rs b/apps/vault/src/ux/framework.rs index e66a98941..590a067b4 100644 --- a/apps/vault/src/ux/framework.rs +++ b/apps/vault/src/ux/framework.rs @@ -774,5 +774,6 @@ impl VaultUx { } pub(crate) fn ensure_hid(&self) { self.usb_dev.ensure_core(usb_device_xous::UsbDeviceType::Hid).unwrap(); + self.usb_dev.restrict_debug_access(true).unwrap(); } } \ No newline at end of file From fd63a330782546a072e24f0254292c15d22387d6 Mon Sep 17 00:00:00 2001 From: bunnie Date: Mon, 27 Jun 2022 04:03:33 +0800 Subject: [PATCH 09/12] add PDDB heap limit increase based on passing a threshold --- services/pddb/src/main.rs | 34 ++++++++++++++++++++++++++++++---- 1 file changed, 30 insertions(+), 4 deletions(-) diff --git a/services/pddb/src/main.rs b/services/pddb/src/main.rs index 107c6e123..35fd48f3f 100644 --- a/services/pddb/src/main.rs +++ b/services/pddb/src/main.rs @@ -423,8 +423,6 @@ fn main() -> ! { let pddb_sid = xns.register_name(api::SERVER_NAME_PDDB, None).expect("can't register server"); log::trace!("registered with NS -- {:?}", pddb_sid); - log::trace!("ready to accept requests"); - // shared entropy cache across all process-local services (it's more efficient to request entropy in blocks from the TRNG) let entropy = Rc::new(RefCell::new(TrngPool::new())); @@ -542,8 +540,11 @@ fn main() -> ! { let mut initial_heap: usize = 0; let mut latest_heap: usize = 0; let mut latest_cache: usize = 0; - const HEAP_GC_THRESH: usize = 400 * 1024; // the heap breaks at 512kiB, so try cleaning up at 400k - const HEAP_GC_TARGET: usize = 64 * 1024; // how much to try cleaning out in any one go. + const HEAP_LARGER_LIMIT: usize = 2048 * 1024; + const HEAP_GC_THRESH: usize = 1500 * 1024; // the largel limit is at 2048kiB, so try cleaning up at 1500k + const HEAP_GC_TARGET: usize = 1024 * 1024; // how much to try cleaning out in any one go. + const HEAP_INC_THRESH: usize = 256 * 1024; // increase from the default heap size of 512k if we pass this limit + let mut heap_increased = false; // register a suspend/resume listener let mut susres = susres::Susres::new(Some(susres::SuspendOrder::Early), &xns, @@ -634,6 +635,31 @@ fn main() -> ! { latest_heap = heap_usage(); log::info!("{} pruned, now: {} heap, {} cache", pruned, latest_heap, basis_cache.cache_size()) } + + // one-shot increase of heap limit once we pass a threshold + // we don't put this at the very top of main because there may be some concurrency issue + // if a heap increase is attempted while a lot of new processes are spawning. + if (latest_heap > HEAP_INC_THRESH) && !heap_increased { + let new_limit = HEAP_LARGER_LIMIT; + let result = xous::rsyscall(xous::SysCall::AdjustProcessLimit( + xous::Limits::HeapMaximum as usize, + 0, + new_limit, + )); + + if let Ok(xous::Result::Scalar2(1, current_limit)) = result { + xous::rsyscall(xous::SysCall::AdjustProcessLimit( + xous::Limits::HeapMaximum as usize, + current_limit, + new_limit, + )) + .unwrap(); + log::info!("Heap limit increased to: {}", new_limit); + } else { + panic!("Unsupported syscall!"); + } + heap_increased = true; + } } Some(Opcode::ListBasis) => { let mut buffer = unsafe { Buffer::from_memory_message_mut(msg.body.memory_message_mut().unwrap()) }; From 79cb4cd98219a2bbe065757cff5a8b3aa3e1307a Mon Sep 17 00:00:00 2001 From: bunnie Date: Mon, 27 Jun 2022 04:03:49 +0800 Subject: [PATCH 10/12] image display now works, once heap increase is called takes about 1MiB of heap to decode the test PNG. Putting the heap increase at boot seems to not work, it causes other processes to not spawn. The condition is not terribly reproducible, so it's probably a concurrency issue of some type. --- services/shellchat/src/cmds/net_cmd.rs | 21 ++++++++++++++++++++- services/shellchat/src/main.rs | 2 +- 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/services/shellchat/src/cmds/net_cmd.rs b/services/shellchat/src/cmds/net_cmd.rs index 972256cbd..ac3cfa45f 100644 --- a/services/shellchat/src/cmds/net_cmd.rs +++ b/services/shellchat/src/cmds/net_cmd.rs @@ -289,6 +289,25 @@ impl<'a> ShellCmdApi<'a> for NetCmd { } #[cfg(feature="ditherpunk")] "image" => { + let new_limit = 2048 * 1024; + let result = xous::rsyscall(xous::SysCall::AdjustProcessLimit( + xous::Limits::HeapMaximum as usize, + 0, + new_limit, + )); + + if let Ok(xous::Result::Scalar2(1, current_limit)) = result { + xous::rsyscall(xous::SysCall::AdjustProcessLimit( + xous::Limits::HeapMaximum as usize, + current_limit, + new_limit, + )) + .unwrap(); + log::info!("Our new heap is: {}", new_limit); + } else { + panic!("Unsupported syscall!"); + } + #[cfg(feature="tracking-alloc")] use tracking_allocator::{ AllocationGroupToken, AllocationRegistry, @@ -360,7 +379,7 @@ impl<'a> ShellCmdApi<'a> for NetCmd { } else { if len < last_chunk { log::info!("heuristic termination of read loop"); - // break; + break; } } }, diff --git a/services/shellchat/src/main.rs b/services/shellchat/src/main.rs index 20ede01a6..50ca84ef3 100644 --- a/services/shellchat/src/main.rs +++ b/services/shellchat/src/main.rs @@ -362,7 +362,7 @@ fn main () -> ! { wrapped_main(); #[cfg(feature="ditherpunk")] - let stack_size = 1024 * 1024; + let stack_size = 2048 * 1024; #[cfg(feature="ditherpunk")] std::thread::Builder::new() .stack_size(stack_size) From 75b370a4949a832cc64e4ebc6da75d218e2fe21b Mon Sep 17 00:00:00 2001 From: bunnie Date: Mon, 27 Jun 2022 04:04:54 +0800 Subject: [PATCH 11/12] fix iterations on the test modal generator the previous setting was so large that it took a long time to run. the current setting takes maybe 2-3 minutes to complete. --- services/modals/src/tests.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/services/modals/src/tests.rs b/services/modals/src/tests.rs index 6194386e1..c82c96f77 100644 --- a/services/modals/src/tests.rs +++ b/services/modals/src/tests.rs @@ -163,8 +163,8 @@ pub fn spawn_test() { */ #[cfg(feature = "ditherpunk")] { - log::info!("testing image"); let img = clifford(); + log::info!("showing image"); modals.show_image(&img).expect("show image modal failed"); log::info!("image modal test done"); } @@ -181,7 +181,8 @@ fn clifford() -> Img { let mut buf = vec![255u8; (SIZE * SIZE).try_into().unwrap()]; let (a, b, c, d) = (-2.0, -2.4, 1.1, -0.9); let (mut x, mut y): (f32, f32) = (0.0, 0.0); - for _ in 0..=4000000 { + log::info!("generating image"); + for _ in 0..=400000 { // this takes a couple minutes to run let x1 = f32::sin(a * y) + c * f32::cos(a * x); let y1 = f32::sin(b * x) + d * f32::cos(b * y); (x, y) = (x1, y1); @@ -191,6 +192,7 @@ fn clifford() -> Img { buf[i] -= 1; } } + log::info!("done: {:x?}", &buf[..32]); Img::new(buf, SIZE.try_into().unwrap(), PixelType::U8) } From ac3f6a06fbf296e117e86e3f5b71e764fab1ce98 Mon Sep 17 00:00:00 2001 From: bunnie Date: Mon, 27 Jun 2022 04:25:18 +0800 Subject: [PATCH 12/12] fix status bar (ran out of precision during the floating pt refactoring) --- services/status/src/main.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/status/src/main.rs b/services/status/src/main.rs index 433f3cf6a..966754248 100644 --- a/services/status/src/main.rs +++ b/services/status/src/main.rs @@ -530,7 +530,7 @@ fn wrapped_main() -> ! { let (latest_activity, period) = llio .activity_instantaneous() .expect("couldn't get CPU activity"); - let activity_to_width = (((latest_activity * 1000) / (period)) * (cpuload_rect.width() - 4) as u32) / 1000; + let activity_to_width = ((latest_activity as u64) * 1000u64 * (cpuload_rect.width() as u64 - 4)) / (period as u64 * 1000u64); draw_list.push(GamObjectType::Rect( Rectangle::new_coords_with_style( cpuload_rect.tl().x + 2,