Re: [PATCH v2] userns: improve uid/gid map collision detection
On Fri, Jan 25, 2013 at 06:31:37PM -0800, Eric W. Biederman wrote: > Aritsteu after looking at both my version and yours I am going with one extra t. > mine. While my code is a little wordier I have half the number of > comparisons your code does, and I took the time to kill the variable > introducing a function to test for range collisions makes unnecessary. > On Andrews size metric my version seems noticably smaller as well. > > size $PWD-build/kernel/user_namespace.o >text data bss dec hex filename >4376 144 0452011a8 > /home/eric/projects/linux/linux-userns-devel-build/kernel/user_namespace.o > > > Short of something unexpected I plan to push all my code to linux-next > sometime tomorrow. sure, no problem Thanks Eric -- Aristeu -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] userns: improve uid/gid map collision detection
Aristeu Rozanski writes:
> On Thu, Jan 24, 2013 at 04:46:12PM -0800, Andrew Morton wrote:
>> eek, a macro! Macros are always bad.
>>
>> This one is bad because
>>
>> a) it's a macro
>>
>> b) it evaluates its args multiple times and hence will cause nasty
>>bugs if called with expressions-with-side-effects.
>>
>> c) it evaluates its args multiple times and if called with
>>non-trivial expressions the compiler might not be able to CSE those
>>expressions, leading to code bloat.
>>
>> Add lo, this patch:
>>
>> ---
>> a/kernel/user_namespace.c~userns-improve-uid-gid-map-collision-detection-fix
>> +++ a/kernel/user_namespace.c
>> @@ -521,7 +521,11 @@ struct seq_operations proc_projid_seq_op
>>
>> static DEFINE_MUTEX(id_map_mutex);
>>
>> -#define in_range(b,first,len) ((b)>=(first)&&(b)<(first)+(len))
>> +static bool in_range(u32 b, u32 first, u32 len)
>> +{
>> +return b >= first && b < first + len;
>> +}
>> +
>> static inline int extent_collision(struct uid_gid_map *new_map,
>> struct uid_gid_extent *extent)
>> {
>>
>> reduces the user_namespace.o text from 4822 bytes to 4727 with
>> gcc-4.4.4. This is a remarkably large difference.
>
> thanks Andrew
>
> (I see Eric already answered about the config option)
Aritsteu after looking at both my version and yours I am going with
mine. While my code is a little wordier I have half the number of
comparisons your code does, and I took the time to kill the variable
introducing a function to test for range collisions makes unnecessary.
On Andrews size metric my version seems noticably smaller as well.
size $PWD-build/kernel/user_namespace.o
textdata bss dec hex filename
4376 144 0452011a8
/home/eric/projects/linux/linux-userns-devel-build/kernel/user_namespace.o
Short of something unexpected I plan to push all my code to linux-next
sometime tomorrow.
Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] userns: improve uid/gid map collision detection
On Thu, Jan 24, 2013 at 04:46:12PM -0800, Andrew Morton wrote:
> eek, a macro! Macros are always bad.
>
> This one is bad because
>
> a) it's a macro
>
> b) it evaluates its args multiple times and hence will cause nasty
>bugs if called with expressions-with-side-effects.
>
> c) it evaluates its args multiple times and if called with
>non-trivial expressions the compiler might not be able to CSE those
>expressions, leading to code bloat.
>
> Add lo, this patch:
>
> ---
> a/kernel/user_namespace.c~userns-improve-uid-gid-map-collision-detection-fix
> +++ a/kernel/user_namespace.c
> @@ -521,7 +521,11 @@ struct seq_operations proc_projid_seq_op
>
> static DEFINE_MUTEX(id_map_mutex);
>
> -#define in_range(b,first,len) ((b)>=(first)&&(b)<(first)+(len))
> +static bool in_range(u32 b, u32 first, u32 len)
> +{
> + return b >= first && b < first + len;
> +}
> +
> static inline int extent_collision(struct uid_gid_map *new_map,
> struct uid_gid_extent *extent)
> {
>
> reduces the user_namespace.o text from 4822 bytes to 4727 with
> gcc-4.4.4. This is a remarkably large difference.
thanks Andrew
(I see Eric already answered about the config option)
--
Aristeu
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] userns: improve uid/gid map collision detection
I will look at this patch in a bit. I already have a similar patch in my queue, although I don't know if I have pushed that out yet. Andrew Morton writes: > btw, what the heck is up with CONFIG_UIDGID_CONVERTED? That thing > prevents userns from being compiled in an allmodconfig build, which is > rather undesirable. Isn't there a better, more user-friendly way of > doing this? CONFIG_UIDGID_CONVERTED is what keeps the allmodconfig building with user namespaces enabled. The practical issue is that the uid_t, gid_t converstion to kuid_t and kgid_t is not quite done. And it takes a lot of pain staking review in some cases to get it done. In places where the conversion is not complete the code fails to compile preventing the introduction of security holes. If you look in init/Kconfig at the definition of CONFIG_UIDGID_CONVERTED you see the list of filesystems that still need changes. I have the basic version of those changes queued up, and I believe those changes are correct. However I need to break them into smaller patches to make the changes obviously correct. One of the things I have discovered along the way of making these conversions is that I need to push kuid_t and kgid_t as deep and as thoroughly into the code as I can or I will fail to place conversions where they need to be. The amount of work required on those final pieces appears to be proportional to the size of the filesystem rather than proportional to the size of the changes to those filesystems. Because there is so much code that must be reviewed and understood. I am hoping I can get the conversions done and CONFIG_UIDGID_CONVERTED can be killed for 3.9 but we will see. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] userns: improve uid/gid map collision detection
On Thu, 24 Jan 2013 10:28:59 -0500
Aristeu Rozanski wrote:
> userns: improve uid/gid map collision detection
>
> Initial implementation of the uid/gid maps (/proc//{u,g}id_map) will
> enforce that the UID and GID maps be written in strict order as a simple
> way to check for range collision:
> local idmapped to count/range
> 0 100050
> (ids 0-50 get mapped to 1000-1050)
> 100 212010
> 500 5000200
> so for each new entry, local id must be bigger than last local id (plus
> count) and the ids it maps to also needs to be bigger than the last
> entry (plus count).
>
> This makes impossible to have a use case like this:
> local idmapped to count/range
> 0 10001
> 48 500 20
>
> because while 48+20 > 0+1, 500+20 < 1000+1.
>
> This patch implements a more elaborate collision detection allowing any
> order to be used.
>
> v2: improved the patch description as requested by Andrew
Thanks.
> --- a/kernel/user_namespace.c
> +++ b/kernel/user_namespace.c
> @@ -521,6 +521,28 @@ struct seq_operations proc_projid_seq_operations = {
>
> static DEFINE_MUTEX(id_map_mutex);
>
> +#define in_range(b,first,len) ((b)>=(first)&&(b)<(first)+(len))
eek, a macro! Macros are always bad.
This one is bad because
a) it's a macro
b) it evaluates its args multiple times and hence will cause nasty
bugs if called with expressions-with-side-effects.
c) it evaluates its args multiple times and if called with
non-trivial expressions the compiler might not be able to CSE those
expressions, leading to code bloat.
Add lo, this patch:
--- a/kernel/user_namespace.c~userns-improve-uid-gid-map-collision-detection-fix
+++ a/kernel/user_namespace.c
@@ -521,7 +521,11 @@ struct seq_operations proc_projid_seq_op
static DEFINE_MUTEX(id_map_mutex);
-#define in_range(b,first,len) ((b)>=(first)&&(b)<(first)+(len))
+static bool in_range(u32 b, u32 first, u32 len)
+{
+ return b >= first && b < first + len;
+}
+
static inline int extent_collision(struct uid_gid_map *new_map,
struct uid_gid_extent *extent)
{
reduces the user_namespace.o text from 4822 bytes to 4727 with
gcc-4.4.4. This is a remarkably large difference.
btw, what the heck is up with CONFIG_UIDGID_CONVERTED? That thing
prevents userns from being compiled in an allmodconfig build, which is
rather undesirable. Isn't there a better, more user-friendly way of
doing this?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2] userns: improve uid/gid map collision detection
userns: improve uid/gid map collision detection
Initial implementation of the uid/gid maps (/proc//{u,g}id_map) will
enforce that the UID and GID maps be written in strict order as a simple
way to check for range collision:
local idmapped to count/range
0 100050
(ids 0-50 get mapped to 1000-1050)
100 212010
500 5000200
so for each new entry, local id must be bigger than last local id (plus
count) and the ids it maps to also needs to be bigger than the last
entry (plus count).
This makes impossible to have a use case like this:
local idmapped to count/range
0 10001
48 500 20
because while 48+20 > 0+1, 500+20 < 1000+1.
This patch implements a more elaborate collision detection allowing any
order to be used.
v2: improved the patch description as requested by Andrew
Cc: Andrew Morton
Cc: "Eric W. Biederman"
Cc: "Serge E. Hallyn"
Cc: linux-security-mod...@vger.kernel.org
Signed-off-by: Aristeu Rozanski
diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
index 2b042c4..fb0e492 100644
--- a/kernel/user_namespace.c
+++ b/kernel/user_namespace.c
@@ -521,6 +521,28 @@ struct seq_operations proc_projid_seq_operations = {
static DEFINE_MUTEX(id_map_mutex);
+#define in_range(b,first,len) ((b)>=(first)&&(b)<(first)+(len))
+static inline int extent_collision(struct uid_gid_map *new_map,
+ struct uid_gid_extent *extent)
+{
+ int i;
+ struct uid_gid_extent *cur;
+
+ for (i = 0; i < new_map->nr_extents; i++) {
+ cur = &new_map->extent[i];
+ if (in_range(extent->first, cur->first, cur->count) ||
+ in_range(extent->first + extent->count, cur->first,
+cur->count))
+ return 1;
+ if (in_range(extent->lower_first, cur->lower_first,
+cur->count) ||
+ in_range(extent->lower_first + extent->count,
+cur->lower_first, cur->count))
+ return 1;
+ }
+ return 0;
+}
+
static ssize_t map_write(struct file *file, const char __user *buf,
size_t count, loff_t *ppos,
int cap_setid,
@@ -634,10 +656,7 @@ static ssize_t map_write(struct file *file, const char
__user *buf,
if ((extent->lower_first + extent->count) <=
extent->lower_first)
goto out;
- /* For now only accept extents that are strictly in order */
- if (last &&
- (((last->first + last->count) > extent->first) ||
-((last->lower_first + last->count) > extent->lower_first)))
+ if (extent_collision(&new_map, extent))
goto out;
new_map.nr_extents++;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
