From e5577eac67398731f0e799d8e9146e63a117f81f Mon Sep 17 00:00:00 2001 From: L0STE <125566964+L0STE@users.noreply.github.com> Date: Sun, 17 Nov 2024 17:02:59 +0100 Subject: [PATCH 1/5] Added close and based_close --- sdk/pinocchio/src/account_info.rs | 32 ++++++++++++++++++++++++++++++- sdk/pinocchio/src/lib.rs | 1 + 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/sdk/pinocchio/src/account_info.rs b/sdk/pinocchio/src/account_info.rs index cc662b7..3f1c4fa 100644 --- a/sdk/pinocchio/src/account_info.rs +++ b/sdk/pinocchio/src/account_info.rs @@ -1,5 +1,4 @@ //! Data structures to represent account information. - use core::{marker::PhantomData, mem::ManuallyDrop, ptr::NonNull, slice::from_raw_parts_mut}; use crate::{program_error::ProgramError, pubkey::Pubkey, syscalls::sol_memset_}; @@ -379,6 +378,37 @@ impl AccountInfo { Ok(()) } + #[inline(always)] + pub fn close(&self) { + unsafe { + *(self.borrow_mut_data_unchecked().as_ptr().sub(48) as *mut [u8; 48]) = [0u8; 48]; + } + } + + /// todo + /// + /// # Safety + /// + /// todo + #[inline(always)] + pub unsafe fn based_close(&self) { + #[cfg(target_os = "solana")] + unsafe { + let var = 0u64; + core::arch::asm!( + "stxdw [{0}-8], {1}", + "stxdw [{0}-16], {1}", + "stxdw [{0}-24], {1}", + "stxdw [{0}-32], {1}", + "stxdw [{0}-40], {1}", + "stxdw [{0}-48], {1}", + in(reg) self.borrow_mut_data_unchecked().as_mut_ptr(), + in(reg) var, + options(nostack, preserves_flags) + ); + } + } + /// Returns the memory address of the account data. fn data_ptr(&self) -> *mut u8 { unsafe { (self.raw as *const _ as *mut u8).add(core::mem::size_of::()) } diff --git a/sdk/pinocchio/src/lib.rs b/sdk/pinocchio/src/lib.rs index 230fc54..af953f8 100644 --- a/sdk/pinocchio/src/lib.rs +++ b/sdk/pinocchio/src/lib.rs @@ -9,6 +9,7 @@ //! [`solana-program`]: https://docs.rs/solana-program/latest/solana_program/ #![no_std] +#![cfg_attr(target_os="solana", feature(asm_experimental_arch, asm_const))] pub mod account_info; pub mod entrypoint; From cb829adf588e6af31d4ee539966a4b6a953d9563 Mon Sep 17 00:00:00 2001 From: L0STE <125566964+L0STE@users.noreply.github.com> Date: Sun, 17 Nov 2024 18:55:00 +0100 Subject: [PATCH 2/5] added docs comments + wrapped up and tested both function --- sdk/pinocchio/src/account_info.rs | 44 ++++++++++++++++++++++++++++--- 1 file changed, 41 insertions(+), 3 deletions(-) diff --git a/sdk/pinocchio/src/account_info.rs b/sdk/pinocchio/src/account_info.rs index 3f1c4fa..ea90d9f 100644 --- a/sdk/pinocchio/src/account_info.rs +++ b/sdk/pinocchio/src/account_info.rs @@ -378,6 +378,26 @@ impl AccountInfo { Ok(()) } + /// Zero out the the account's data_len, lamports and owner fields, effectively + /// closing the account. + /// + /// Note: This doesn't protect against future reinitialization of the account + /// since the account_data will need to be zeroed out as well. Or if the attacker + /// has access to the keypair of the account that we're trying to close, they can + /// just add the lenght, lamports and owner back before the data is wiped out from + /// the ledger. + /// + /// Note: This works because the 48 bytes before the account data are: + /// - 8 bytes for the data_len + /// - 8 bytes for the lamports + /// - 32 bytes for the owner + /// + /// # Safety + /// + /// This method makes assumptions about the layout and location of memory + /// referenced by `AccountInfo` fields. It should only be called for + /// instances of `AccountInfo` that were created by the runtime and received + /// in the `process_instruction` entrypoint of a program. #[inline(always)] pub fn close(&self) { unsafe { @@ -385,13 +405,31 @@ impl AccountInfo { } } - /// todo + /// Zero out the the account's data_len, lamports and owner fields, effectively + /// closing the account. + /// + /// Note: This doesn't protect against future reinitialization of the account + /// since the account_data will need to be zeroed out as well. Or if the attacker + /// has access to the keypair of the account that we're trying to close, they can + /// just add the lenght, lamports and owner back before the data is wiped out from + /// the ledger. + /// + /// Note: This works because the 48 bytes before the account data are: + /// - 8 bytes for the data_len + /// - 8 bytes for the lamports + /// - 32 bytes for the owner /// /// # Safety /// - /// todo + /// This method makes assumptions about the layout and location of memory + /// referenced by `AccountInfo` fields. It should only be called for + /// instances of `AccountInfo` that were created by the runtime and received + /// in the `process_instruction` entrypoint of a program. + /// + /// This method set the variable as 0 making sure that we're actually zeroing out + /// the bytes. #[inline(always)] - pub unsafe fn based_close(&self) { + pub unsafe fn optimized_close(&self) { #[cfg(target_os = "solana")] unsafe { let var = 0u64; From f753c7c69ad3b2097b39b42fe894e78b379c1c89 Mon Sep 17 00:00:00 2001 From: L0STE <125566964+L0STE@users.noreply.github.com> Date: Sun, 17 Nov 2024 19:04:37 +0100 Subject: [PATCH 3/5] cargo clippy and fmt --- sdk/pinocchio/src/account_info.rs | 10 +++++----- sdk/pinocchio/src/lib.rs | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/sdk/pinocchio/src/account_info.rs b/sdk/pinocchio/src/account_info.rs index ea90d9f..bcf0fb9 100644 --- a/sdk/pinocchio/src/account_info.rs +++ b/sdk/pinocchio/src/account_info.rs @@ -381,12 +381,12 @@ impl AccountInfo { /// Zero out the the account's data_len, lamports and owner fields, effectively /// closing the account. /// - /// Note: This doesn't protect against future reinitialization of the account + /// Note: This doesn't protect against future reinitialization of the account /// since the account_data will need to be zeroed out as well. Or if the attacker /// has access to the keypair of the account that we're trying to close, they can /// just add the lenght, lamports and owner back before the data is wiped out from /// the ledger. - /// + /// /// Note: This works because the 48 bytes before the account data are: /// - 8 bytes for the data_len /// - 8 bytes for the lamports @@ -408,12 +408,12 @@ impl AccountInfo { /// Zero out the the account's data_len, lamports and owner fields, effectively /// closing the account. /// - /// Note: This doesn't protect against future reinitialization of the account + /// Note: This doesn't protect against future reinitialization of the account /// since the account_data will need to be zeroed out as well. Or if the attacker /// has access to the keypair of the account that we're trying to close, they can /// just add the lenght, lamports and owner back before the data is wiped out from /// the ledger. - /// + /// /// Note: This works because the 48 bytes before the account data are: /// - 8 bytes for the data_len /// - 8 bytes for the lamports @@ -425,7 +425,7 @@ impl AccountInfo { /// referenced by `AccountInfo` fields. It should only be called for /// instances of `AccountInfo` that were created by the runtime and received /// in the `process_instruction` entrypoint of a program. - /// + /// /// This method set the variable as 0 making sure that we're actually zeroing out /// the bytes. #[inline(always)] diff --git a/sdk/pinocchio/src/lib.rs b/sdk/pinocchio/src/lib.rs index af953f8..4cbceb1 100644 --- a/sdk/pinocchio/src/lib.rs +++ b/sdk/pinocchio/src/lib.rs @@ -9,7 +9,7 @@ //! [`solana-program`]: https://docs.rs/solana-program/latest/solana_program/ #![no_std] -#![cfg_attr(target_os="solana", feature(asm_experimental_arch, asm_const))] +#![cfg_attr(target_os = "solana", feature(asm_experimental_arch, asm_const))] pub mod account_info; pub mod entrypoint; From f181c70cbc4e4d1afa865319f0a8f5f23e8c43d6 Mon Sep 17 00:00:00 2001 From: L0STE <125566964+L0STE@users.noreply.github.com> Date: Mon, 18 Nov 2024 19:00:04 +0100 Subject: [PATCH 4/5] added the new close and changed the name for --- sdk/pinocchio/src/account_info.rs | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/sdk/pinocchio/src/account_info.rs b/sdk/pinocchio/src/account_info.rs index bcf0fb9..901e691 100644 --- a/sdk/pinocchio/src/account_info.rs +++ b/sdk/pinocchio/src/account_info.rs @@ -398,11 +398,21 @@ impl AccountInfo { /// referenced by `AccountInfo` fields. It should only be called for /// instances of `AccountInfo` that were created by the runtime and received /// in the `process_instruction` entrypoint of a program. + /// + /// This method is unsafe because it does not check if the account data is already + /// borrowed. It should only be called when the account is not being used. #[inline(always)] - pub fn close(&self) { - unsafe { - *(self.borrow_mut_data_unchecked().as_ptr().sub(48) as *mut [u8; 48]) = [0u8; 48]; - } + pub unsafe fn close(&self) { + // Zero out the account owner. While the field is a `Pubkey`, it is quicker + // to zero the 32 bytes as 4 x `u64`s. + *(self.data_ptr().sub(48) as *mut u64) = 0u64; + *(self.data_ptr().sub(40) as *mut u64) = 0u64; + *(self.data_ptr().sub(32) as *mut u64) = 0u64; + *(self.data_ptr().sub(16) as *mut u64) = 0u64; + // Zero the account lamports. + (*self.raw).lamports = 0; + // Zero the account data length. + (*self.raw).data_len = 0; } /// Zero out the the account's data_len, lamports and owner fields, effectively @@ -429,13 +439,17 @@ impl AccountInfo { /// This method set the variable as 0 making sure that we're actually zeroing out /// the bytes. #[inline(always)] - pub unsafe fn optimized_close(&self) { + pub unsafe fn assembly_close(&self) { #[cfg(target_os = "solana")] unsafe { let var = 0u64; core::arch::asm!( + // Zero the data lenght. "stxdw [{0}-8], {1}", + // Zero the account lamports. "stxdw [{0}-16], {1}", + // Zero out the account owner. While the field is a `Pubkey`, it is quicker + // to zero the 32 bytes as 4 x `u64`s. "stxdw [{0}-24], {1}", "stxdw [{0}-32], {1}", "stxdw [{0}-40], {1}", From fb459a3832de58dd57711ac2f9aac10a1d86f06e Mon Sep 17 00:00:00 2001 From: L0STE <125566964+L0STE@users.noreply.github.com> Date: Wed, 20 Nov 2024 19:06:30 +0100 Subject: [PATCH 5/5] fixed and tested after comments --- sdk/pinocchio/src/account_info.rs | 65 +++++++------------------------ 1 file changed, 15 insertions(+), 50 deletions(-) diff --git a/sdk/pinocchio/src/account_info.rs b/sdk/pinocchio/src/account_info.rs index 901e691..8ca912a 100644 --- a/sdk/pinocchio/src/account_info.rs +++ b/sdk/pinocchio/src/account_info.rs @@ -1,7 +1,7 @@ //! Data structures to represent account information. use core::{marker::PhantomData, mem::ManuallyDrop, ptr::NonNull, slice::from_raw_parts_mut}; -use crate::{program_error::ProgramError, pubkey::Pubkey, syscalls::sol_memset_}; +use crate::{program_error::ProgramError, pubkey::Pubkey, syscalls::sol_memset_, ProgramResult}; /// Maximum number of bytes a program may add to an account during a /// single realloc. @@ -381,16 +381,27 @@ impl AccountInfo { /// Zero out the the account's data_len, lamports and owner fields, effectively /// closing the account. /// - /// Note: This doesn't protect against future reinitialization of the account + /// This doesn't protect against future reinitialization of the account /// since the account_data will need to be zeroed out as well. Or if the attacker /// has access to the keypair of the account that we're trying to close, they can /// just add the lenght, lamports and owner back before the data is wiped out from /// the ledger. /// - /// Note: This works because the 48 bytes before the account data are: + /// This works because the 48 bytes before the account data are: /// - 8 bytes for the data_len /// - 8 bytes for the lamports /// - 32 bytes for the owner + pub fn close(&self) -> ProgramResult { + { + let _ = self.try_borrow_mut_data()?; + } + + unsafe { + self.close_unchecked(); + } + + Ok(()) + } /// /// # Safety /// @@ -402,7 +413,7 @@ impl AccountInfo { /// This method is unsafe because it does not check if the account data is already /// borrowed. It should only be called when the account is not being used. #[inline(always)] - pub unsafe fn close(&self) { + pub unsafe fn close_unchecked(&self) { // Zero out the account owner. While the field is a `Pubkey`, it is quicker // to zero the 32 bytes as 4 x `u64`s. *(self.data_ptr().sub(48) as *mut u64) = 0u64; @@ -415,52 +426,6 @@ impl AccountInfo { (*self.raw).data_len = 0; } - /// Zero out the the account's data_len, lamports and owner fields, effectively - /// closing the account. - /// - /// Note: This doesn't protect against future reinitialization of the account - /// since the account_data will need to be zeroed out as well. Or if the attacker - /// has access to the keypair of the account that we're trying to close, they can - /// just add the lenght, lamports and owner back before the data is wiped out from - /// the ledger. - /// - /// Note: This works because the 48 bytes before the account data are: - /// - 8 bytes for the data_len - /// - 8 bytes for the lamports - /// - 32 bytes for the owner - /// - /// # Safety - /// - /// This method makes assumptions about the layout and location of memory - /// referenced by `AccountInfo` fields. It should only be called for - /// instances of `AccountInfo` that were created by the runtime and received - /// in the `process_instruction` entrypoint of a program. - /// - /// This method set the variable as 0 making sure that we're actually zeroing out - /// the bytes. - #[inline(always)] - pub unsafe fn assembly_close(&self) { - #[cfg(target_os = "solana")] - unsafe { - let var = 0u64; - core::arch::asm!( - // Zero the data lenght. - "stxdw [{0}-8], {1}", - // Zero the account lamports. - "stxdw [{0}-16], {1}", - // Zero out the account owner. While the field is a `Pubkey`, it is quicker - // to zero the 32 bytes as 4 x `u64`s. - "stxdw [{0}-24], {1}", - "stxdw [{0}-32], {1}", - "stxdw [{0}-40], {1}", - "stxdw [{0}-48], {1}", - in(reg) self.borrow_mut_data_unchecked().as_mut_ptr(), - in(reg) var, - options(nostack, preserves_flags) - ); - } - } - /// Returns the memory address of the account data. fn data_ptr(&self) -> *mut u8 { unsafe { (self.raw as *const _ as *mut u8).add(core::mem::size_of::()) }