[PSEUDOPATCH] rename is_compat_task

[PSEUDOPATCH] rename is_compat_task

[Andy Lutomirski]

Hi all-

Every time I look at is_compat_task, I cringe.  That function
determines whether we're in a compat syscall, not whether we're in a
compat task.  There are probably architectures (arm64?) under which
these are the same conditions, but they are definitely *not* the same
thing on x86.

Can we just fix it?  I propose the following patch:

$ find -type f |xargs sed -i -e 's/is_compat_task/in_compat_syscall/g'

If there's general agreement, can we do that at the end of the next
merge window?

I could also send a patch series to add in_compat_syscall, change all
the users, then delete the old stuff, but that seems overcomplicated
for something that's literally just renaming a token.

--Andy

Re: [PSEUDOPATCH] rename is_compat_task

[Arnd Bergmann]

On Monday 07 December 2015 15:12:59 Andy Lutomirski wrote:
> Hi all-
> 
> Every time I look at is_compat_task, I cringe.  That function
> determines whether we're in a compat syscall, not whether we're in a
> compat task.  There are probably architectures (arm64?) under which
> these are the same conditions, but they are definitely *not* the same
> thing on x86.
> 
> Can we just fix it?  I propose the following patch:
> 
> $ find -type f |xargs sed -i -e 's/is_compat_task/in_compat_syscall/g'
> 
> If there's general agreement, can we do that at the end of the next
> merge window?
> 
> I could also send a patch series to add in_compat_syscall, change all
> the users, then delete the old stuff, but that seems overcomplicated
> for something that's literally just renaming a token.

As far as I know, x86 is the special case here, on all other architectures,
this actually checks the task, and it's impossible to call a system call
of the other kind.

	Arnd

Re: [PSEUDOPATCH] rename is_compat_task

[Andy Lutomirski]

On Mon, Dec 7, 2015 at 3:16 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Monday 07 December 2015 15:12:59 Andy Lutomirski wrote:
>> Hi all-
>>
>> Every time I look at is_compat_task, I cringe.  That function
>> determines whether we're in a compat syscall, not whether we're in a
>> compat task.  There are probably architectures (arm64?) under which
>> these are the same conditions, but they are definitely *not* the same
>> thing on x86.
>>
>> Can we just fix it?  I propose the following patch:
>>
>> $ find -type f |xargs sed -i -e 's/is_compat_task/in_compat_syscall/g'
>>
>> If there's general agreement, can we do that at the end of the next
>> merge window?
>>
>> I could also send a patch series to add in_compat_syscall, change all
>> the users, then delete the old stuff, but that seems overcomplicated
>> for something that's literally just renaming a token.
>
> As far as I know, x86 is the special case here, on all other architectures,
> this actually checks the task, and it's impossible to call a system call
> of the other kind.
>

Nonetheless, it's still nasty.  I'm very slowly trying to get the
kernel to stop checking "is this task a compat task" at all on x86
except in the *very* small number of cases where it's correct.  I've
already found and fixed one security bug that resulted from confusing
the conditions.

I don't think that the other (more sensible) architectures lose
anything from making my proposed change.  After all, most of the users
are in generic code, and they'll still be correct on all architectures
assuming that they were correct in the first place.

--Andy

Re: [PSEUDOPATCH] rename is_compat_task

[Al Viro]

On Tue, Dec 08, 2015 at 12:16:51AM +0100, Arnd Bergmann wrote:

> As far as I know, x86 is the special case here, on all other architectures,
> this actually checks the task, and it's impossible to call a system call
> of the other kind.

sparc uses
	ta 0x10
for 32bit syscalls and
	ta 0x6d
for 64bit ones.  And yes, the same process can call both just fine.

Re: [PSEUDOPATCH] rename is_compat_task

[Ingo Molnar]

* Arnd Bergmann <arnd@arndb.de> wrote:

> On Monday 07 December 2015 15:12:59 Andy Lutomirski wrote:
> > Hi all-
> > 
> > Every time I look at is_compat_task, I cringe.  That function
> > determines whether we're in a compat syscall, not whether we're in a
> > compat task.  There are probably architectures (arm64?) under which
> > these are the same conditions, but they are definitely *not* the same
> > thing on x86.
> > 
> > Can we just fix it?  I propose the following patch:
> > 
> > $ find -type f |xargs sed -i -e 's/is_compat_task/in_compat_syscall/g'
> > 
> > If there's general agreement, can we do that at the end of the next
> > merge window?
> > 
> > I could also send a patch series to add in_compat_syscall, change all
> > the users, then delete the old stuff, but that seems overcomplicated
> > for something that's literally just renaming a token.
> 
> As far as I know, x86 is the special case here, on all other architectures, this 
> actually checks the task, and it's impossible to call a system call of the other 
> kind.

Well, even on architectures that don't allow mixed mode system calls for the same 
task the name 'in_compat_syscall()' is still correct: it just happens to also be a 
permanent condition for the life time of a task.

On architectures that allow mixed mode syscalls the assumption and confusion 
carried by the 'is_compat_task()' misnomer has resulted in real security bugs, 
hence Andy's suggestion for a rename.

So without my x86 hat on I'd still argue that 'is_compat_syscall()' is the more 
expressive (and hence more robust, safer) name. On architectures that don't care 
the change carries zero costs.

So are there any deep objections to doing this rename in a single, quick, 
pain-minimized fashion right at the end of the next merge window, when the amount 
of pending patches in various maintainer trees is at a cyclical minimum? We can 
also keep an is_compat_task() migratory define for one more cycle just in case.

Thanks,

	Ingo

Re: [PSEUDOPATCH] rename is_compat_task

[Al Viro]

On Tue, Dec 08, 2015 at 05:36:49AM +0100, Ingo Molnar wrote:

> So are there any deep objections to doing this rename in a single, quick, 
> pain-minimized fashion right at the end of the next merge window, when the amount 
> of pending patches in various maintainer trees is at a cyclical minimum? We can 
> also keep an is_compat_task() migratory define for one more cycle just in case.

Again, what about sparc?  There we have both 64bit and 32bit syscalls possible
to issue from the same process *and* no indication which trap had been used;
how do you implement is_compat_syscall() there?  There's a TIF_32BIT, which
is used by mmap() and friends, signal delivery, etc., but that's not a matter
of which syscall flavour had been issued.  Said that, arch/sparc doesn't use
is_compat_task(); it's open-coded everywhere...

Re: [PSEUDOPATCH] rename is_compat_task

[Ingo Molnar]

* Al Viro <viro@ZenIV.linux.org.uk> wrote:

> On Tue, Dec 08, 2015 at 05:36:49AM +0100, Ingo Molnar wrote:
> 
> > So are there any deep objections to doing this rename in a single, quick, 
> > pain-minimized fashion right at the end of the next merge window, when the 
> > amount of pending patches in various maintainer trees is at a cyclical 
> > minimum? We can also keep an is_compat_task() migratory define for one more 
> > cycle just in case.
> 
> Again, what about sparc?  There we have both 64bit and 32bit syscalls possible 
> to issue from the same process *and* no indication which trap had been used; how 
> do you implement is_compat_syscall() there?  There's a TIF_32BIT, which is used 
> by mmap() and friends, signal delivery, etc., but that's not a matter of which 
> syscall flavour had been issued.  Said that, arch/sparc doesn't use 
> is_compat_task(); it's open-coded everywhere...

Hm, so if Sparc has no notion of compat-ness of the system call then how does it 
implement runtime compat checks, such as AUDIT_ARCH et al?

Thanks,

	Ingo

Re: [PSEUDOPATCH] rename is_compat_task

[Al Viro]

On Tue, Dec 08, 2015 at 06:01:48AM +0100, Ingo Molnar wrote:

> Hm, so if Sparc has no notion of compat-ness of the system call then how does it 
> implement runtime compat checks, such as AUDIT_ARCH et al?

Badly.  Things like compat_sys_ioctl() vs. ioctl() work (we use different
arrays of function pointers in 32bit and 64bit traps), but anything
dynamic assumes that things match the task.  Not that we had a lot of
such dynamic checks, actually...

Re: [PSEUDOPATCH] rename is_compat_task

[Andy Lutomirski]

On Mon, Dec 7, 2015 at 9:15 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Tue, Dec 08, 2015 at 06:01:48AM +0100, Ingo Molnar wrote:
>
>> Hm, so if Sparc has no notion of compat-ness of the system call then how does it
>> implement runtime compat checks, such as AUDIT_ARCH et al?
>
> Badly.  Things like compat_sys_ioctl() vs. ioctl() work (we use different
> arrays of function pointers in 32bit and 64bit traps), but anything
> dynamic assumes that things match the task.  Not that we had a lot of
> such dynamic checks, actually...

I wouldn't take x86 as a shining example of how to do this, but it
does mostly work.  In 4.4, it's not even all that messy, modulo a
bunch of checks that check the wrong condition (hence, in part, this
proposal).

--Andy