lkml.org 
[lkml]   [2010]   [May]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: Q: sys_personality() && misc oddities
    Date
    > > Though the high bit might be set on 32-bit, there still should not really
    > > be a danger of misinterpreting a value as an error code--as long as we
    > > haven't used up all 10 of those middle bits. The test userland (glibc)
    > > uses is not "long < 0" but "u_long > -4095UL". So as long as at least
    > > one bit in 0xff00 remains clear, it won't match.
    >
    > Yes, libc itself is fine. But from the application's pov, personality()
    > returns int, not long.

    That doesn't really matter to error/success ambiguity. Since what I said
    is true, it won't ever return exactly -1 for a non-error. But even if it
    did, the application can use errno=0;personality(x);errno!=0 checking.

    > > For 64-bit you want to avoid sign-extension of the old value just so it
    > > looks valid (even though it won't look like an error code). I think the
    > > most sensible thing is to change the task_struct field to 'unsigned int'.
    >
    > it is already 'unsigned int' ;)

    Ok, then there is no bug right now, is there?

    > Yes! and despite the fact it returns -EINVAL, current->personality was
    > changed. This can't be right.

    Agreed.

    > > So, perhaps you are right about checking high
    > > bits. Then I'd make it:
    > >
    > > if ((int) personality != -1) {
    > > if (unlikely((unsigned int) personality != personality))
    > > return -EINVAL;
    >
    > Well. Think about personality(0xffffffff - 1). It passes both checks
    > and we change current->personality. Then the application calls
    > personality() again, we return the old value, and since the user-space
    > expects "int" it gets -2.

    Yes, it never really made any sense to me that it doesn't validate any of
    the flag bits.

    > How about
    >
    > if (personality != 0xffffffff) {
    > if (personality >= 0x7fffffff)
    > return -EINVAL;
    > set_personality(personality);
    > }
    >
    > ? Now that personality always fits into "insigned int" we don't need
    > to recheck current->personality == personality, and "< 0x7fffffff"
    > gurantees that "int old_personality = personality(whatever)" in user
    > space can be never misinterpeted as error.

    Sure.

    > As for the other oddities, they need the separate patches. Or we can
    > just leave this code alone ;)

    I can't see any sign that anybody cares.


    Thanks,
    Roland


    \
     
     \ /
      Last update: 2010-05-26 22:33    [W:0.026 / U:93.632 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site