Re: [PATCH v6 2/5] landlock: Support file truncation

["Mickaël Salaün" <mic@digikod.net>]

On 29/09/2022 21:22, Günther Noack wrote:
> On Wed, Sep 28, 2022 at 08:32:02PM +0200, Mickaël Salaün wrote:
>> On 25/09/2022 20:09, Günther Noack wrote:
>>> On Fri, Sep 23, 2022 at 10:53:23PM +0200, Mickaël Salaün wrote:
>>>> On 23/09/2022 13:21, Günther Noack wrote:
>>>>> On Mon, Sep 12, 2022 at 09:41:32PM +0200, Mickaël Salaün wrote:
>>>>>> On 08/09/2022 21:58, Günther Noack wrote:
>>>>>>> Introduce the LANDLOCK_ACCESS_FS_TRUNCATE flag for file truncation.
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>> +/**
>>>>>>> + * get_path_access_rights - Returns the subset of rights in access_request
>>>>>>> + * which are permitted for the given path.
>>>>>>> + *
>>>>>>> + * @domain: The domain that defines the current restrictions.
>>>>>>> + * @path: The path to get access rights for.
>>>>>>> + * @access_request: The rights we are interested in.
>>>>>>> + *
>>>>>>> + * Returns: The access mask of the rights that are permitted on the given path,
>>>>>>> + * which are also a subset of access_request (to save some calculation time).
>>>>>>> + */
>>>>>>> +static inline access_mask_t
>>>>>>> +get_path_access_rights(const struct landlock_ruleset *const domain,
>>>>>>> +		       const struct path *const path,
>>>>>>> +		       access_mask_t access_request)
>>>>>>> +{
>>>>>>> +	layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {};
>>>>>>> +	unsigned long access_bit;
>>>>>>> +	unsigned long access_req;
>>>>>>> +
>>>>>>> +	init_layer_masks(domain, access_request, &layer_masks);
>>>>>>> +	if (!check_access_path_dual(domain, path, access_request, &layer_masks,
>>>>>>> +				    NULL, 0, NULL, NULL)) {
>>>>>>> +		/*
>>>>>>> +		 * Return immediately for successful accesses and for cases
>>>>>>> +		 * where everything is permitted because the path belongs to an
>>>>>>> +		 * internal filesystem.
>>>>>>> +		 */
>>>>>>> +		return access_request;
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	access_req = access_request;
>>>>>>> +	for_each_set_bit(access_bit, &access_req, ARRAY_SIZE(layer_masks)) {
>>>>>>> +		if (layer_masks[access_bit]) {
>>>>>>> +			/* If any layer vetoed the access right, remove it. */
>>>>>>> +			access_request &= ~BIT_ULL(access_bit);
>>>>>>> +		}
>>>>>>> +	}
>>>>>>
>>>>>> This seems to be redundant with the value returned by init_layer_masks(),
>>>>>> which should be passed to check_access_path_dual() to avoid useless path
>>>>>> walk.
>>>>>
>>>>> True, I'll use the result of init_layer_masks() to feed it back to
>>>>> check_access_path_dual() to avoid a bit of computation.
>>>>>
>>>>> Like this:
>>>>>
>>>>>            effective_access_request =
>>>>> 		init_layer_masks(domain, access_request, &layer_masks);
>>>>> 	if (!check_access_path_dual(domain, path, effective_access_request,
>>>>> 	    &layer_masks, NULL, 0, NULL, NULL)) {
>>>>> 		// ...
>>>>> 	}
>>>>
>>>> correct
>>>>
>>>>>
>>>>> Overall, the approach here is:
>>>>>
>>>>> * Initialize the layer_masks, so that it has a bit set for every
>>>>>      access right in access_request and layer where that access right is
>>>>>      handled.
>>>>>
>>>>> * check_access_path_dual() with only the first few parameters -- this
>>>>>      will clear all the bits in layer masks which are actually permitted
>>>>>      according to the individual rules.
>>>>>
>>>>>      As a special case, this *may* return 0 immediately, in which case we
>>>>>      can (a) save a bit of calculation in the loop below and (b) we might
>>>>>      be in the case where access is permitted because it's a file from a
>>>>>      special file system (even though not all bits are cleared). If
>>>>>      check_access_path_dual() returns 0, we return the full requested
>>>>>      access_request that we received as input. >
>>>>> * In the loop below, if there are any bits left in layer_masks, those
>>>>>      are rights which are not permitted for the given path. We remove
>>>>>      these from access_request and return the modified access_request.
>>>>>
>>>>>
>>>>>> This function is pretty similar to check_access_path(). Can't you change it
>>>>>> to use an access_mask_t pointer and get almost the same thing?
>>>>>
>>>>> I'm shying away from this approach. Many of the existing different use
>>>>> cases are already realized by "doing if checks deep down". I think it
>>>>> would make the code more understandable if we managed to model these
>>>>> differences between use cases already at the layer of function calls.
>>>>> (This is particularly true for check_access_path_dual(), where in
>>>>> order to find out how the "single" case works, you need to disentangle
>>>>> to a large extent how the much more complicated dual case works.)
>>>>
>>>> I agree that check_access_path_dual() is complex, but I couldn't find a
>>>> better way.
>>>
>>> It seems out of the scope of this patch set, but I sometimes find it
>>> OK to just duplicate the code and have a set of tests to demonstrate
>>> that the two variants do the same thing.
>>>
>>> check_access_path_dual() is mostly complex because of performance
>>> reasons, as far as I can tell, and it might be possible to check its
>>> results against a parallel implementation of it which runs slower,
>>> uses more memory, but is more obviously correct. (I have used one
>>> myself to check against when developing the truncate patch set.)
>>>
>>>>> If you want to unify these two functions, what do you think of the
>>>>> approach of just using get_path_access_rights() instead of
>>>>> check_access_path()?
>>>>>
>>>>> Basically, it would turn
>>>>>
>>>>> return check_access_path(dom, path, access_request);
>>>>>
>>>>> into
>>>>>
>>>>> if (get_path_access_rights(dom, path, access_request) == access_request)
>>>>> 	return 0;
>>>>> return -EACCES;
>>>>>
>>>>> This is slightly more verbose in the places where it's called, but it
>>>>> would be more orthogonal, and it would also clarify that -EACCES is
>>>>> the only possible error in the "single" path walk case.
>>>>>
>>>>> Let me know what you think.
>>>>
>>>> What about adding an additional argument `access_mask_t *const
>>>> access_allowed` to check_access_path_dual() which returns the set of
>>>> accesses (i.e. access_masked_parent1 & access_masked_parent2) that could
>>>> then be stored to landlock_file(file)->allowed_access? If this argument is
>>>> NULL it should just be ignored. What is left from get_path_access_rights()
>>>> could then be merged into hook_file_open().
>>>
>>> IMHO, check_access_path_dual() does not seem like the right place to
>>> add this. This functionality is not needed in any of the "dual path"
>>> cases so far, and I'm not sure what it would mean. The necessary
>>> information can also be easily derived from the resulting layer_masks,
>>> which is already exposed in the check_access_path_dual() interface,
>>> and I also believe that this approach is at least equally fast as
>>> updating it on the fly when changing the layer_masks.
>>>
>>> I could be convinced to add a `access_mask_t *const access_allowed`
>>> argument to check_access_path() if you prefer that, but then again, in
>>> that case the returned boolean can be reconstructed from the new
>>> access_allowed variable, and we could as well make check_access_path()
>>> return the access_allowed result instead of the boolean and let
>>> callers check equality with what they expected...? (I admittedly don't
>>> have a good setup to test the performance right now, but it looks like
>>> a negligible difference to me?)
>>
>> Good idea, let's try to make check_access_path_dual() returns the allowed
>> accesses (according to the request) and rename it to get_access_path_dual().
>> unmask_layers() could be changed to return the still-denied accesses instead
>> of a boolean, and we could use this values (for potential both parents) to
>> return allowed_parent1 & allowed_parent2 (with access_mask_t types). This
>> would also simplify is_eaccess() and its calls could be moved to
>> current_check_refer_path(). This would merge get_path_access_rights() into
>> check_access_path_dual() and make the errno codes more explicit per hook or
>> defined in check_access_path().
> 
> Thanks for the review!
> 
> I'm afraid I don't understand this approach at the moment. I'm
> probably still missing some insight about how the "refer" logic works
> which would make this clearer.
> 
> With the proposed changes to check_access_path_dual(), it sounds like
> we would have to change the logic of the "refer" implementation quite
> a bit, which would expand the scope of the "truncate" patch set beyond
> what it was originally meant to do. Is this check_access_path_dual()
> refactoring something you'd insist on for the truncate patch set, or
> would you be OK with doing that separately?

I'd like to avoid stacking debts and I prefer to refactor code instead, 
but I got your point. Here is another proposal closer to yours. Let's 
rename check_access_path_dual() to is_access_to_paths_allowed(), make it 
returns a boolean (allowed_parent1 && allowed_parent2), and move the 
EACCES/EXDEV logic to (only) after the second call to 
check_access_path_dual() by current_check_refer_path() (because the if 
(old_dentry->d_parent == new_dir->dentry) branch cannot return EXDEV).

check_access_path() either returns 0 or -EACCES, and we should add a 
WARN_ON_ONCE(access_request & LANDLOCK_ACCESS_FS_REFER) to make sure 
this remains correct.

The get_path_access_rights() logic can be moved to hook_file_open() to 
make it more readable.


> 
> For the truncate patch set, what do you think of the lighter
> refactoring options, which I had outlined in my previous mail? - see
> the four bullet points quoted here:
> 
>>> Here are the options we have discussed, in the order that I would
>>> prefer them:
>>>
>>> * to keep it as a separate function as it already is,
>>>     slightly duplicating check_access_path(). (I think it's cleaner,
>>>     because the code path for the rest of the hooks other than
>>>     security_file_open() stays simpler.)
>>>
>>> * to make check_access_path() return the access_allowed access mask
>>>     and make callers check that it covers the access_request that they
>>>     asked for (see example from my previous mail on this thread). (This
>>>     is equivalent to discarding the existing check_access_path() and
>>>     using the get_path_access() function instead.)
>>>
>>> * to add a `access_mask_t *const access_allowed` argument to
>>>     check_access_path(), which is calculated if it's non-NULL based on
>>>     the layer_masks result. It would be used from the security_file_open
>>>     hook.
>>>
>>> * to add a `access_mask_t *const access_allowed` argument to
>>>     check_access_path_dual(). This doesn't make much sense, IMHO,
>>>     because an on-the-fly calculation of this result does not look like
>>>     a performance benefit to me, and calculating it based on the two
>>>     resulting layer_masks is already possible now. It's also not clear
>>>     to me what it would mean to calculate an access_allowed on two paths
>>>     at once, and what that would be used for.
>>>
>>> Let me know which option you prefer. In the end, I don't feel that
>>> strongly about it and I'm happy to do this either way.
> 
> Thanks,
> Günther
>