diff --git a/src/battery.rs b/src/battery.rs index ec390cb..f167ab0 100644 --- a/src/battery.rs +++ b/src/battery.rs @@ -161,12 +161,29 @@ fn write_sysfs_value(path: impl AsRef, value: &str) -> Result<()> { }) } +/// Safely check if a path exists and is writable +fn path_exists_and_writable(path: &Path) -> bool { + if !path.exists() { + return false; + } + + // Try to open the file with write access to verify write permission + match fs::OpenOptions::new().write(true).open(path) { + Ok(_) => true, + Err(_) => false, + } +} + /// Identifies if a battery supports threshold control and which pattern it uses fn find_battery_with_threshold_support(ps_path: &Path) -> Option> { for pattern in THRESHOLD_PATTERNS { let start_threshold_path = ps_path.join(pattern.start_path); let stop_threshold_path = ps_path.join(pattern.stop_path); - if start_threshold_path.exists() && stop_threshold_path.exists() { + + // Ensure both paths exist and are writable before considering this battery supported + if path_exists_and_writable(&start_threshold_path) + && path_exists_and_writable(&stop_threshold_path) + { return Some(SupportedBattery { name: ps_path.file_name()?.to_string_lossy().to_string(), pattern, diff --git a/src/cpu.rs b/src/cpu.rs index 62d0dd1..fb35a9a 100644 --- a/src/cpu.rs +++ b/src/cpu.rs @@ -79,6 +79,15 @@ where } pub fn set_governor(governor: &str, core_id: Option) -> Result<()> { + // Validate the governor is available on this system + if !is_governor_valid(governor)? { + return Err(ControlError::InvalidValueError(format!( + "Governor '{}' is not available on this system. Valid governors: {}", + governor, + get_available_governors()?.join(", ") + ))); + } + let action = |id: u32| { let path = format!("/sys/devices/system/cpu/cpu{id}/cpufreq/scaling_governor"); if Path::new(&path).exists() { @@ -93,6 +102,32 @@ pub fn set_governor(governor: &str, core_id: Option) -> Result<()> { core_id.map_or_else(|| for_each_cpu_core(action), action) } +/// Check if the provided governor is available in the system +fn is_governor_valid(governor: &str) -> Result { + let governors = get_available_governors()?; + Ok(governors.contains(&governor.to_string())) +} + +/// Get available CPU governors from the system +fn get_available_governors() -> Result> { + // We'll check cpu0's available governors as they're typically the same across cores + let path = "/sys/devices/system/cpu/cpu0/cpufreq/scaling_available_governors"; + + if !Path::new(path).exists() { + return Err(ControlError::NotSupported( + "Could not determine available governors".to_string(), + )); + } + + let content = fs::read_to_string(path) + .map_err(|e| ControlError::ReadError(format!("Failed to read available governors: {e}")))?; + + Ok(content + .split_whitespace() + .map(ToString::to_string) + .collect()) +} + pub fn set_turbo(setting: TurboSetting) -> Result<()> { let value_pstate = match setting { TurboSetting::Always => "0", // no_turbo = 0 means turbo is enabled @@ -153,6 +188,16 @@ fn try_set_per_core_boost(value: &str) -> Result { } pub fn set_epp(epp: &str, core_id: Option) -> Result<()> { + // Validate the EPP value against available options + let available_epp = get_available_epp_values()?; + if !available_epp.contains(&epp.to_string()) { + return Err(ControlError::InvalidValueError(format!( + "Invalid EPP value: '{}'. Available values: {}", + epp, + available_epp.join(", ") + ))); + } + let action = |id: u32| { let path = format!("/sys/devices/system/cpu/cpu{id}/cpufreq/energy_performance_preference"); if Path::new(&path).exists() { @@ -164,8 +209,37 @@ pub fn set_epp(epp: &str, core_id: Option) -> Result<()> { core_id.map_or_else(|| for_each_cpu_core(action), action) } +/// Get available EPP values from the system +fn get_available_epp_values() -> Result> { + let path = "/sys/devices/system/cpu/cpu0/cpufreq/energy_performance_available_preferences"; + + if !Path::new(path).exists() { + // If the file doesn't exist, fall back to a default set of common values + // This is safer than failing outright, as some systems may allow these values + // even without explicitly listing them + return Ok(vec![ + "default".to_string(), + "performance".to_string(), + "balance_performance".to_string(), + "balance_power".to_string(), + "power".to_string(), + ]); + } + + let content = fs::read_to_string(path).map_err(|e| { + ControlError::ReadError(format!("Failed to read available EPP values: {e}")) + })?; + + Ok(content + .split_whitespace() + .map(ToString::to_string) + .collect()) +} + pub fn set_epb(epb: &str, core_id: Option) -> Result<()> { - // EPB is often an integer 0-15. + // Validate EPB value - should be a number 0-15 or a recognized string value + validate_epb_value(epb)?; + let action = |id: u32| { let path = format!("/sys/devices/system/cpu/cpu{id}/cpufreq/energy_performance_bias"); if Path::new(&path).exists() { @@ -177,7 +251,54 @@ pub fn set_epb(epb: &str, core_id: Option) -> Result<()> { core_id.map_or_else(|| for_each_cpu_core(action), action) } +fn validate_epb_value(epb: &str) -> Result<()> { + // EPB can be a number from 0-15 or a recognized string + // Try parsing as a number first + if let Ok(value) = epb.parse::() { + if value <= 15 { + return Ok(()); + } else { + return Err(ControlError::InvalidValueError(format!( + "EPB numeric value must be between 0 and 15, got {}", + value + ))); + } + } + + // If not a number, check if it's a recognized string value + let valid_strings = [ + "performance", + "balance-performance", + "balance-power", + "power", + // Alternative forms + "balance_performance", + "balance_power", + ]; + + if valid_strings.contains(&epb) { + Ok(()) + } else { + Err(ControlError::InvalidValueError(format!( + "Invalid EPB value: '{}'. Must be a number 0-15 or one of: {}", + epb, + valid_strings.join(", ") + ))) + } +} + pub fn set_min_frequency(freq_mhz: u32, core_id: Option) -> Result<()> { + // Check if the new minimum frequency would be greater than current maximum + if let Some(id) = core_id { + validate_min_frequency(id, freq_mhz)?; + } else { + // Check for all cores + let num_cores = get_logical_core_count()?; + for id in 0..num_cores { + validate_min_frequency(id, freq_mhz)?; + } + } + let freq_khz_str = (freq_mhz * 1000).to_string(); let action = |id: u32| { let path = format!("/sys/devices/system/cpu/cpu{id}/cpufreq/scaling_min_freq"); @@ -191,6 +312,17 @@ pub fn set_min_frequency(freq_mhz: u32, core_id: Option) -> Result<()> { } pub fn set_max_frequency(freq_mhz: u32, core_id: Option) -> Result<()> { + // Check if the new maximum frequency would be less than current minimum + if let Some(id) = core_id { + validate_max_frequency(id, freq_mhz)?; + } else { + // Check for all cores + let num_cores = get_logical_core_count()?; + for id in 0..num_cores { + validate_max_frequency(id, freq_mhz)?; + } + } + let freq_khz_str = (freq_mhz * 1000).to_string(); let action = |id: u32| { let path = format!("/sys/devices/system/cpu/cpu{id}/cpufreq/scaling_max_freq"); @@ -203,6 +335,66 @@ pub fn set_max_frequency(freq_mhz: u32, core_id: Option) -> Result<()> { core_id.map_or_else(|| for_each_cpu_core(action), action) } +fn read_sysfs_value_as_u32(path: &str) -> Result { + if !Path::new(path).exists() { + return Err(ControlError::NotSupported(format!( + "File does not exist: {path}" + ))); + } + + let content = fs::read_to_string(path) + .map_err(|e| ControlError::ReadError(format!("Failed to read {path}: {e}")))?; + + content + .trim() + .parse::() + .map_err(|e| ControlError::ReadError(format!("Failed to parse value from {path}: {e}"))) +} + +fn validate_min_frequency(core_id: u32, new_min_freq_mhz: u32) -> Result<()> { + let max_freq_path = format!("/sys/devices/system/cpu/cpu{core_id}/cpufreq/scaling_max_freq"); + + if !Path::new(&max_freq_path).exists() { + return Ok(()); + } + + let max_freq_khz = read_sysfs_value_as_u32(&max_freq_path)?; + let new_min_freq_khz = new_min_freq_mhz * 1000; + + if new_min_freq_khz > max_freq_khz { + return Err(ControlError::InvalidValueError(format!( + "Minimum frequency ({} MHz) cannot be higher than maximum frequency ({} MHz) for core {}", + new_min_freq_mhz, + max_freq_khz / 1000, + core_id + ))); + } + + Ok(()) +} + +fn validate_max_frequency(core_id: u32, new_max_freq_mhz: u32) -> Result<()> { + let min_freq_path = format!("/sys/devices/system/cpu/cpu{core_id}/cpufreq/scaling_min_freq"); + + if !Path::new(&min_freq_path).exists() { + return Ok(()); + } + + let min_freq_khz = read_sysfs_value_as_u32(&min_freq_path)?; + let new_max_freq_khz = new_max_freq_mhz * 1000; + + if new_max_freq_khz < min_freq_khz { + return Err(ControlError::InvalidValueError(format!( + "Maximum frequency ({} MHz) cannot be lower than minimum frequency ({} MHz) for core {}", + new_max_freq_mhz, + min_freq_khz / 1000, + core_id + ))); + } + + Ok(()) +} + /// Sets the platform profile. /// This changes the system performance, temperature, fan, and other hardware replated characteristics. /// diff --git a/src/engine.rs b/src/engine.rs index fa50124..7001871 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -1,5 +1,5 @@ use crate::battery; -use crate::config::{AppConfig, ProfileConfig}; +use crate::config::{AppConfig, ProfileConfig, TurboAutoSettings}; use crate::core::{OperationalMode, SystemReport, TurboSetting}; use crate::cpu::{self}; use crate::util::error::{ControlError, EngineError}; @@ -177,6 +177,9 @@ fn manage_auto_turbo(report: &SystemReport, config: &ProfileConfig) -> Result<() // Get the auto turbo settings from the config, or use defaults let turbo_settings = config.turbo_auto_settings.clone().unwrap_or_default(); + // Validate the complete configuration to ensure it's usable + validate_turbo_auto_settings(&turbo_settings)?; + // Get average CPU temperature and CPU load let cpu_temp = report.cpu_global.average_temperature_celsius; @@ -202,14 +205,6 @@ fn manage_auto_turbo(report: &SystemReport, config: &ProfileConfig) -> Result<() } }; - // Validate the configuration to ensure it's usable - if turbo_settings.load_threshold_high <= turbo_settings.load_threshold_low { - return Err(EngineError::ConfigurationError( - "Invalid turbo auto settings: high threshold must be greater than low threshold" - .to_string(), - )); - } - // Decision logic for enabling/disabling turbo let enable_turbo = match (cpu_temp, avg_cpu_usage) { // If temperature is too high, disable turbo regardless of load @@ -262,3 +257,30 @@ fn manage_auto_turbo(report: &SystemReport, config: &ProfileConfig) -> Result<() Err(e) => Err(EngineError::ControlError(e)), } } + +fn validate_turbo_auto_settings(settings: &TurboAutoSettings) -> Result<(), EngineError> { + // Validate load thresholds + if settings.load_threshold_high <= settings.load_threshold_low { + return Err(EngineError::ConfigurationError( + "Invalid turbo auto settings: high threshold must be greater than low threshold" + .to_string(), + )); + } + + // Validate range of load thresholds (should be 0-100%) + if settings.load_threshold_high > 100.0 || settings.load_threshold_low < 0.0 { + return Err(EngineError::ConfigurationError( + "Invalid turbo auto settings: load thresholds must be between 0% and 100%".to_string(), + )); + } + + // Validate temperature threshold (realistic range for CPU temps in Celsius) + if settings.temp_threshold_high <= 0.0 || settings.temp_threshold_high > 110.0 { + return Err(EngineError::ConfigurationError( + "Invalid turbo auto settings: temperature threshold must be between 0°C and 110°C" + .to_string(), + )); + } + + Ok(()) +} diff --git a/src/main.rs b/src/main.rs index 3755929..e5f6fba 100644 --- a/src/main.rs +++ b/src/main.rs @@ -357,15 +357,70 @@ fn main() { cpu::set_epb(&epb, core_id).map_err(|e| Box::new(e) as Box) } Some(Commands::SetMinFreq { freq_mhz, core_id }) => { - cpu::set_min_frequency(freq_mhz, core_id) - .map_err(|e| Box::new(e) as Box) + // Basic validation for reasonable CPU frequency values + if freq_mhz == 0 { + error!("Minimum frequency cannot be zero"); + Err(Box::new(ControlError::InvalidValueError( + "Minimum frequency cannot be zero".to_string(), + )) as Box) + } else if freq_mhz > 10000 { + // Extremely high value unlikely to be valid + error!("Minimum frequency ({freq_mhz} MHz) is unreasonably high"); + Err(Box::new(ControlError::InvalidValueError(format!( + "Minimum frequency ({freq_mhz} MHz) is unreasonably high" + ))) as Box) + } else { + cpu::set_min_frequency(freq_mhz, core_id) + .map_err(|e| Box::new(e) as Box) + } } Some(Commands::SetMaxFreq { freq_mhz, core_id }) => { - cpu::set_max_frequency(freq_mhz, core_id) - .map_err(|e| Box::new(e) as Box) + // Basic validation for reasonable CPU frequency values + if freq_mhz == 0 { + error!("Maximum frequency cannot be zero"); + Err(Box::new(ControlError::InvalidValueError( + "Maximum frequency cannot be zero".to_string(), + )) as Box) + } else if freq_mhz > 10000 { + // Extremely high value unlikely to be valid + error!("Maximum frequency ({freq_mhz} MHz) is unreasonably high"); + Err(Box::new(ControlError::InvalidValueError(format!( + "Maximum frequency ({freq_mhz} MHz) is unreasonably high" + ))) as Box) + } else { + cpu::set_max_frequency(freq_mhz, core_id) + .map_err(|e| Box::new(e) as Box) + } + } + Some(Commands::SetPlatformProfile { profile }) => { + // Get available platform profiles and validate early if possible + match cpu::get_platform_profiles() { + Ok(available_profiles) => { + if !available_profiles.contains(&profile) { + error!( + "Invalid platform profile: '{}'. Available profiles: {}", + profile, + available_profiles.join(", ") + ); + Err(Box::new(ControlError::InvalidProfile(format!( + "Invalid platform profile: '{}'. Available profiles: {}", + profile, + available_profiles.join(", ") + ))) as Box) + } else { + info!("Setting platform profile to '{}'", profile); + cpu::set_platform_profile(&profile) + .map_err(|e| Box::new(e) as Box) + } + } + Err(_) => { + // If we can't get profiles (e.g., feature not supported), pass through to the function + // which will provide appropriate error + cpu::set_platform_profile(&profile) + .map_err(|e| Box::new(e) as Box) + } + } } - Some(Commands::SetPlatformProfile { profile }) => cpu::set_platform_profile(&profile) - .map_err(|e| Box::new(e) as Box), Some(Commands::SetBatteryThresholds { start_threshold, stop_threshold,