[refpolicy] [PATCH 1/2]: cpucontrol module updates (stricter policy for CPU microcode updates)

[cpebenito@tresys.com (Christopher J. PeBenito)]

On 08/24/12 10:02, Guido Trentalancia wrote:
> cpucontrol module updates:
> - introduce the file contexts according to the standard location
>    of the most common application named microcode_ctl
>    (http://www.urbanmyth.org/microcode);
> - add file contexts for CPUs from two different vendors, taking
>    into consideration specific customization of the location
>    for one distribution;
> - add file contexts and declarations for the init script;
> - introduce the ability to update the CPU microcode as
>    tunable policy, as apparently such operation might
>    modify the original licensing conditions (under some or
>    all circumstances: "personal, non-commercial use only");
> - create a new device type specifically for the CPU microcode
>    updating functionality and modify the cpucontrol module so
>    that such distinct new type is used for the microcode
>    updating operation, thus leaving an open door for further
>    modifications that distinguish different CPU-related
>    applications/utilities to create further isolation;
> - modify the interface definitions so that the CPU microcode
>    update utility can only write and not read (unneeded) the
>    corresponding above mentioned device type.
> 
> Signed-off-by: Guido Trentalancia <guido@trentalancia.com>
> ---
>   policy/modules/contrib/cpucontrol.fc |   18 +++++++++-
>   policy/modules/contrib/cpucontrol.te |   16 ++++++++-
>   policy/modules/kernel/devices.fc     |    3 +
>   policy/modules/kernel/devices.if     |   62 
> +++++++++++++++++++++++++++++++----
>   policy/modules/kernel/devices.te     |    8 +++-
>   5 files changed, 96 insertions(+), 11 deletions(-)
> 
> diff -pruN refpolicy-08082012/policy/modules/contrib/cpucontrol.fc 
> refpolicy-08082012-microcode/policy/modules/contrib/cpucontrol.fc
> --- refpolicy-08082012/policy/modules/contrib/cpucontrol.fc	2011-09-09 
> 18:29:23.563610858 +0200
> +++ refpolicy-08082012-microcode/policy/modules/contrib/cpucontrol.fc 
> 2012-08-09 01:13:59.615119168 +0200
> @@ -1,7 +1,23 @@
> +/etc/microcode\.dat	--	gen_context(system_u:object_r:cpucontrol_conf_t,s0)
> +/etc/microcode_amd\.bin	-- 
> gen_context(system_u:object_r:cpucontrol_conf_t,s0)
> +/etc/firmware/microcode\.dat	-- 
> gen_context(system_u:object_r:cpucontrol_conf_t,s0)
> +/etc/firmware/microcode_amd.bin	-- 
> gen_context(system_u:object_r:cpucontrol_conf_t,s0)
> 
> -/etc/firmware/.*	--	gen_context(system_u:object_r:cpucontrol_conf_t,s0)
> +/etc/rc\.d/init\.d/microcode	-- 
> gen_context(system_u:object_r:cpucontrol_initrc_exec_t,s0)
> 
> +ifdef(`distro_redhat',`
> +/lib/firmware/intel-ucode(/.*)? 
> gen_context(system_u:object_r:cpucontrol_conf_t,s0)
> +/lib/firmware/amd-ucode(/.*)? 
> gen_context(system_u:object_r:cpucontrol_conf_t,s0)
> +/lib/firmware/microcode\.dat	-- 
> gen_context(system_u:object_r:cpucontrol_conf_t,s0)
> +/lib/firmware/microcode_amd\.bat	-- 
> gen_context(system_u:object_r:cpucontrol_conf_t,s0)
> +')

I'm conflicted.  I assume that the /etc/microcode and /etc/firmware locations are the default for this app, /lib/firmware seems more appropriate.  I suspect this is not a redhat-specific location.

> +/usr/sbin/microcode_ctl	-- 
> gen_context(system_u:object_r:cpucontrol_exec_t,s0)
> +/usr/local/sbin/microcode_ctl	-- 
> gen_context(system_u:object_r:cpucontrol_exec_t,s0)

The /usr/local label is not necessary, as it will be handled by the file context path substitutions.

> +ifdef(`distro_redhat',`
>   /sbin/microcode_ctl	--	gen_context(system_u:object_r:cpucontrol_exec_t,s0)
> +')

I also don't think this change is really necessary.

>   /usr/sbin/cpufreqd	--	gen_context(system_u:object_r:cpuspeed_exec_t,s0)
>   /usr/sbin/cpuspeed	--	gen_context(system_u:object_r:cpuspeed_exec_t,s0)
> diff -pruN refpolicy-08082012/policy/modules/contrib/cpucontrol.te 
> refpolicy-08082012-microcode/policy/modules/contrib/cpucontrol.te
> --- refpolicy-08082012/policy/modules/contrib/cpucontrol.te	2011-09-09 
> 18:29:23.563610858 +0200
> +++ refpolicy-08082012-microcode/policy/modules/contrib/cpucontrol.te 
> 2012-08-09 00:23:07.079532236 +0200
> @@ -5,10 +5,21 @@ policy_module(cpucontrol, 1.3.0)
>   # Declarations
>   #
> 
> +## <desc>
> +## <p>
> +## Allow cpucontrol to upload new microcode
> +## to the CPU.
> +## </p>
> +## </desc>
> +gen_tunable(cpucontrol_can_upload_cpu_microcode, false)
> +
>   type cpucontrol_t;
>   type cpucontrol_exec_t;
>   init_system_domain(cpucontrol_t, cpucontrol_exec_t)
> 
> +type cpucontrol_initrc_exec_t;
> +init_script_file(cpucontrol_initrc_exec_t)
> +
>   type cpucontrol_conf_t;
>   files_type(cpucontrol_conf_t)
> 
> @@ -37,7 +48,6 @@ kernel_read_proc_symlinks(cpucontrol_t)
>   kernel_read_kernel_sysctls(cpucontrol_t)
> 
>   dev_read_sysfs(cpucontrol_t)
> -dev_rw_cpu_microcode(cpucontrol_t)
> 
>   fs_search_auto_mountpoints(cpucontrol_t)
> 
> @@ -54,6 +64,10 @@ logging_send_syslog_msg(cpucontrol_t)
> 
>   userdom_dontaudit_use_unpriv_user_fds(cpucontrol_t)
> 
> +tunable_policy(`cpucontrol_can_upload_cpu_microcode',`
> +	dev_write_cpu_microcode(cpucontrol_t)
> +')

I don't understand why this is conditional, especially since you removed the read permission on /dev/microcode.  The point of the app is to load microcode.

-- 
Chris PeBenito
Tresys Technology, LLC
www.tresys.com | oss.tresys.com