# HG changeset patch # User Paul Fisher # Date 1750802073 14400 # Node ID 94b51fa4f797c490b4bac7793884a0c494cc74f1 # Parent 3f11b8d30f63fc7021b6df52d9a80b1f97141c99 Fix memory soundness issues: - Ensure Questions are pinned in memory when sending them through PAM. - Hold on to the PAM conversation struct after we build it. (Linux-PAM is leninent about this and copies the pam_conv structure.) diff -r 3f11b8d30f63 -r 94b51fa4f797 src/libpam/answer.rs --- a/src/libpam/answer.rs Tue Jun 24 17:08:01 2025 -0400 +++ b/src/libpam/answer.rs Tue Jun 24 17:54:33 2025 -0400 @@ -6,10 +6,10 @@ pub use crate::libpam::pam_ffi::Answer; use crate::{ErrorCode, Result}; use std::ffi::CStr; +use std::mem::ManuallyDrop; use std::ops::{Deref, DerefMut}; use std::ptr::NonNull; -use std::{iter, mem, ptr, slice}; -use std::mem::ManuallyDrop; +use std::{iter, ptr, slice}; /// The corridor via which the answer to Messages navigate through PAM. #[derive(Debug)] diff -r 3f11b8d30f63 -r 94b51fa4f797 src/libpam/conversation.rs --- a/src/libpam/conversation.rs Tue Jun 24 17:08:01 2025 -0400 +++ b/src/libpam/conversation.rs Tue Jun 24 17:54:33 2025 -0400 @@ -2,7 +2,7 @@ use crate::conv::{Conversation, ErrorMsg, InfoMsg, MaskedQAndA, Message, QAndA}; use crate::libpam::answer::BinaryAnswer; use crate::libpam::answer::{Answer, Answers, TextAnswer}; -use crate::libpam::memory::{CBinaryData, Immovable}; +use crate::libpam::memory::CBinaryData; use crate::libpam::pam_ffi::AppData; pub use crate::libpam::pam_ffi::LibPamConversation; use crate::libpam::question::QuestionsTrait; @@ -21,7 +21,6 @@ callback: Self::wrapper_callback::, appdata: (conv as *const C).cast(), life: PhantomData, - _marker: Immovable(PhantomData), } } @@ -66,13 +65,13 @@ impl Conversation for LibPamConversation<'_> { fn communicate(&self, messages: &[Message]) { let internal = || { - let questions = Questions::new(messages)?; + let questions = Box::pin(Questions::new(messages)?); let mut response_pointer = std::ptr::null_mut(); // SAFETY: We're calling into PAM with valid everything. let result = unsafe { (self.callback)( messages.len() as c_int, - questions.ptr(), + questions.as_ref().ptr(), &mut response_pointer, self.appdata, ) diff -r 3f11b8d30f63 -r 94b51fa4f797 src/libpam/handle.rs --- a/src/libpam/handle.rs Tue Jun 24 17:08:01 2025 -0400 +++ b/src/libpam/handle.rs Tue Jun 24 17:54:33 2025 -0400 @@ -11,7 +11,6 @@ use num_enum::{IntoPrimitive, TryFromPrimitive}; use std::cell::Cell; use std::ffi::{c_char, c_int, CString}; -use std::marker::PhantomData; use std::ops::{Deref, DerefMut}; use std::ptr; @@ -33,9 +32,12 @@ /// An owned PAM handle. pub struct OwnedLibPamHandle<'a> { + /// The handle itself. handle: HandleWrap, + /// The last return value from the handle. last_return: Cell>, - _conversation_lifetime: PhantomData<&'a mut ()>, + /// If set, the Conversation that this PAM handle owns. + conversation: Option>>, } #[derive(Debug, PartialEq)] @@ -77,20 +79,26 @@ username: Option, conversation: &impl Conversation, ) -> Result { - let conv = LibPamConversation::wrap(conversation); + let conv = Box::new(LibPamConversation::wrap(conversation)); let service_cstr = CString::new(service_name).map_err(|_| ErrorCode::ConversationError)?; let username_cstr = memory::prompt_ptr(memory::option_cstr(username.as_deref())?.as_ref()); let mut handle: *mut LibPamHandle = ptr::null_mut(); // SAFETY: We've set everything up properly to call `pam_start`. // The returned value will be a valid pointer provided the result is OK. - let result = - unsafe { pam_ffi::pam_start(service_cstr.as_ptr(), username_cstr, &conv, &mut handle) }; + let result = unsafe { + pam_ffi::pam_start( + service_cstr.as_ptr(), + username_cstr, + conv.as_ref(), + &mut handle, + ) + }; ErrorCode::result_from(result)?; Ok(Self { handle: HandleWrap(handle), last_return: Cell::new(Ok(())), - _conversation_lifetime: Default::default(), + conversation: Some(conv), }) } } diff -r 3f11b8d30f63 -r 94b51fa4f797 src/libpam/pam_ffi.rs --- a/src/libpam/pam_ffi.rs Tue Jun 24 17:08:01 2025 -0400 +++ b/src/libpam/pam_ffi.rs Tue Jun 24 17:54:33 2025 -0400 @@ -57,7 +57,6 @@ /// but for requests with style `PAM_BINARY_PROMPT`, /// this will be `CBinaryData` (a Linux-PAM extension). pub data: Option>, - pub _marker: Immovable, } /// The callback that PAM uses to get information in a conversation. @@ -86,8 +85,9 @@ /// The pointer that will be passed as the last parameter /// to the conversation callback. pub appdata: *const AppData, + /// Marker to associate the lifetime of this with the conversation + /// that was passed in. pub life: PhantomData<&'a mut ()>, - pub _marker: Immovable, } /// Gets a string version of an error message. diff -r 3f11b8d30f63 -r 94b51fa4f797 src/libpam/question.rs --- a/src/libpam/question.rs Tue Jun 24 17:08:01 2025 -0400 +++ b/src/libpam/question.rs Tue Jun 24 17:54:33 2025 -0400 @@ -11,6 +11,7 @@ use crate::Result; use num_enum::{IntoPrimitive, TryFromPrimitive}; use std::ffi::{c_void, CStr}; +use std::pin::Pin; use std::slice; /// Abstraction of a collection of questions to be sent in a PAM conversation. @@ -64,7 +65,7 @@ Self: Sized; /// Gets the pointer that is passed . - fn ptr(&self) -> *const *const Question; + fn ptr(self: Pin<&Self>) -> *const *const Question; /// Converts a pointer into a borrowed list of Questions. /// @@ -115,7 +116,7 @@ }) } - fn ptr(&self) -> *const *const Question { + fn ptr(self: Pin<&Self>) -> *const *const Question { &self.pointer as *const *const Question } @@ -131,10 +132,9 @@ #[derive(Debug)] #[repr(C)] pub struct LinuxPamQuestions { - #[allow(clippy::vec_box)] // we need to do this. + #[allow(clippy::vec_box)] // we need to box vec items. /// The place where the questions are. questions: Vec>, - _marker: Immovable, } impl LinuxPamQuestions { @@ -155,11 +155,10 @@ .collect(); Ok(Self { questions: questions?, - _marker: Default::default(), }) } - fn ptr(&self) -> *const *const Question { + fn ptr(self: Pin<&Self>) -> *const *const Question { self.questions.as_ptr().cast() } @@ -246,7 +245,6 @@ Ok(Self { style: style.into(), data: Some(data), - _marker: Default::default(), }) } } @@ -328,15 +326,15 @@ use super::super::*; #[test] fn standard() { - let interrogation = <$typ>::new(&[ + let interrogation = Box::pin(<$typ>::new(&[ MaskedQAndA::new("hocus pocus").message(), QAndA::new("what").message(), QAndA::new("who").message(), InfoMsg::new("hey").message(), ErrorMsg::new("gasp").message(), ]) - .unwrap(); - let indirect = interrogation.ptr(); + .unwrap()); + let indirect = interrogation.as_ref().ptr(); let remade = unsafe { $typ::borrow_ptr(indirect, interrogation.len()) }; let messages: Vec = remade @@ -364,11 +362,11 @@ #[test] #[cfg(feature = "linux-pam-extensions")] fn linux_extensions() { - let interrogation = <$typ>::new(&[ + let interrogation = Box::pin(<$typ>::new(&[ BinaryQAndA::new((&[5, 4, 3, 2, 1], 66)).message(), RadioQAndA::new("you must choose").message(), - ]).unwrap(); - let indirect = interrogation.ptr(); + ]).unwrap()); + let indirect = interrogation.as_ref().ptr(); let remade = unsafe { $typ::borrow_ptr(indirect, interrogation.len()) }; let messages: Vec = remade