[PATCH i-g-t] tests/intel/xe_sysfs_file_perm: Verify sysfs file permissions for security

[Peter Senna Tschudin]

Hi Kamil,

Please see my comments bellow.

[...]

> 
>> +
>> +/**
>> + * check_files_permissions: Checks if the files in the provided list have
>> + * correct permissions. The expected permissions are:
>> + * - Owner: root
>> + * - Group: root
>> + * - Only owner can write to the file (mode 100644).
>> + * If any file does not have the expected permissions,
>> + * it will print a warning and return false.
>> + * If all files have the expected permissions,
>> + * it will return true.
>> + *
>> + * @param files_list: List of files to check
>> + *
>> + * Returns: true if all files have correct permissions, false otherwise
>> + */
>> +static bool check_files_permissions(struct igt_list_head *files_list)
>> +{
>> +	bool header = false;
>> +	struct file_node *file_node;
>> +	bool ret = true;
>> +
>> +	/* Check if owner and group are root and if only owner can
>> +	 * write to the file.
>> +	 */
>> +	igt_list_for_each_entry(file_node, files_list, link) {
>> +		if (!file_node->permissions.root_user ||
>> +		    !file_node->permissions.root_group ||
>> +		    (file_node->permissions.mode & 0022)) {
> 
> This is error prone, rather use define with mask
> and checked permissions, like:
> 
> #define VALID_PERM 0100644
> #define MASK_PERM  0777777
> 
> use:
> 	(mode & MASK_PERM) == VALID_PERM
> 
> Or use bitwise-revert:
> 	(mode & ~VALID_PERM)

I was doing something like this but checkpatch told me that using labels
was error prone and suggested the 0022 thing. There are actual labels
already defined in shared libraries but checkpatch does not like the
existing labels.

> 
> imho you should check here your special regex-es for rw ones,
> like min|max_freq, where write is allowed. Or make PERM a param
> and use it with second list.

I will kindly ask you to change the threat model, and then I will write
a test that reflects what the model says.

> 
>> +			if (!header) {
>> +				igt_warn("\n\nFiles with incorrect permissions:\n");
>> +				header = true;
>> +			}
>> +
>> +			igt_warn("\t- %s\n\t\troot_user: %s, root_group: %s, mode: %o (expected: 100644)\n",
>> +				 file_node->full_path,
>> +				 file_node->permissions.root_user ? "true" : "false",
>> +				 file_node->permissions.root_group ? "true" : "false",
>> +				 file_node->permissions.mode);
> 
> Please print which bits are unexpected here.

Can you clarify? The message already includes "(expected: 100644)".

> 
>> +			ret = false;
>> +		}
>> +	}
>> +	if (header)
>> +		igt_warn("\n");
>> +
>> +	return ret;
>> +}
>> +
>> +igt_main
>> +{
>> +	struct igt_list_head fds_list;
>> +	struct igt_list_head files_list;
>> +	struct igt_list_head pattern_list;
>> +
>> +	/**
>> +	 * Do not change the list below without checking the Xe threat
> 
> s/list/patterns/
> 
>> +	 * modeling first. You can contact peter.senna at intel.com or
>> +	 * michal.winiarski at intel.com for questions and support.
>> +	 */
>> +
>> +	/* DO NOT CHANGE START */
> 
> Why nobody cannot change this? You already stated that in
> comment above, why repeating it here?

The large paragraph is the explanation. This line here delimits where
the protected part starts and ends.

May I ask to keep this?

> 
> Please comment why we need special regexes below,
> I guess that they have different perms, so imho struct names
> should reflect this.

What do you mean? This test is a reflection of the threat model.
Everything here reflects what the threat models calls for.

> 
>> +	static const char * const gt_patterns[] = {
>> +		"ccs_mode",
>> +		"engines/[a-zA-Z0-9]+/job_timeout_ms",
>> +		"engines/[a-zA-Z0-9]+/preempt_timeout_us",
>> +		"engines/[a-zA-Z0-9]+/timeslice_duration_min",
>> +		"engines/[a-zA-Z0-9]+/timeslice_duration_max",
>> +		"engines/[a-zA-Z0-9]+/timeslice_duration_us",
> 
> Why not just filename, without 'engines' here?

To make it more specific. We are only interested in specific files.

> 
>> +		"freq0/max_freq",
>> +		"freq0/min_freq"
> 
> Same here, why not just max|min_freq?

Please see the threat model.

> 
>> +	};
>> +
>> +	static const char * const hwmon_patterns[] = {
>> +		"power[0-9]+_crit",
>> +		"curr[0-9]+_crit",
>> +		"power[0-9]+_max",
>> +		"power[0-9]+_max_interval"
>> +	};
>> +
>> +	static const char * const devices_patterns[] = {
>> +		"vram_d3cold_threshold"
>> +	};
>> +
>> +	struct dir_input dir_inputs[DIR_COUNT] = {
>> +		{ GT_DIR, gt_patterns, ARRAY_SIZE(gt_patterns) },
>> +		{ HWMON_DIR, hwmon_patterns, ARRAY_SIZE(hwmon_patterns) },
>> +		{ DEVICES_DIR, devices_patterns, ARRAY_SIZE(devices_patterns) }
>> +	};
>> +	/* DO NOT CHANGE END */
> 
> Remove this comment.

May I ask to keep it?


[...]

Thanks!