On 30/11/2022 21:00, Beata Michalska wrote:
On Wed, Nov 30, 2022 at 03:39:36PM +0100, Kevin Brodsky wrote:
On 25/11/2022 13:13, Zachary Leaf wrote:
Just a short question: we're keeping both copy_from_user and copy_from_user_with_ptr because we don't want to preserve capability tags by default and each time this is done should be intentional, right? Is there anything specific to copy_struct_from_user that would make it fine to preserve capability tags by default? I think the alternative would be to have two versions: copy_struct_from_user and copy_struct_from_user_with_ptr so that we don't preserve tags by default.
The 'intentional' part here should be applied when copying to userspace, not the other way round, with the stress point being on not providing valid capabilities to user space when not indented, unless I am missing smth (?)
Alright, I guess because there isn't a copy_struct_to_user as well, then we can just do your change without creating confusion. LGMT, thanks for the clarification!
Right, just so it's clear in my mind - if we do copy_*from*_user without _with_ptr here, then we do not preserve cap tags, that means the kernel would not be able to access or otherwise use that pointer?
I can't think of a scenario where you'd pass a pointer into to kernel but want that pointer unreadable or otherwise unusable.
The copy_*to*_user is the bit where care is needed.
Yes, *copy*_to_user* are the most critical ones in terms of enforcing the capability model. However, at times controlling *copy*_from_user* is equally important, see Amit's "Handle siginfo_t user pointer tags" series for an example (__copy_siginfo_from_user() notably). Even outside of these special cases, defaulting to a non-pointer-preserving copy is a plus in terms of intentionality, strengthening our confidence that user pointers don't get propagated by accident.
I do agree with that, though splitting those into 2 variants does not make it bulletproof against potential misuse. Though I see your point.
Agreed, we can't prevent logic errors, but at least a simple grep allows identifying where user pointers are copied in.
I am therefore strongly in favour of keeping the explicit approach (separate _with_ptr variant) for *copy*_from_user*, and logically that should apply to all variants including copy_struct_from_user.
So in other words you would like to see almost exact replica of copy_struct_from_user. Can do that.
Unfortunately yes that means duplicating copy_struct_from_user, it's not _that_ big though so probably not worth using macro magic to avoid duplication (as that ends up being a lot less readable).
Kevin