linux-kernel - Re: [PATCH] arch/tile: provide PT_FLAGS_COMPAT value in pt

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 13 Dec 2012 09:58:07 -0500
From:	Chris Metcalf <cmetcalf@...era.com>
To:	Oleg Nesterov <oleg@...hat.com>
CC:	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] arch/tile: provide PT_FLAGS_COMPAT value in pt_regs

On 12/12/2012 6:43 PM, Oleg Nesterov wrote:
> Hi Chris,
>
> it is too late for me to even try to read this patch, but...

Thanks for the review!

> On 12/12, Chris Metcalf wrote:
>> This flag is set for ptrace GETREGS or PEEKUSER for processes
>> that are COMPAT, i.e. 32-bit.
>            ^^^^^^^^^^^^^^^^^^^
>
> at least on x86 this is not the same. TS_COMPAT can also be set if a 64-bit
> task calls the 32-bit syscall.

There's no way on tile for that to happen.  The OS chooses which syscall implementation to provide based on the Elf class of the loaded executable; the syscall instruction in userspace is the same either way.

In fact, SO MUCH is identical in the two environments that I couldn't think of a good way for strace to be able to easily figure out whether it was a 32- or 64-bit environment - thus this patch :-)

>> --- a/arch/tile/include/uapi/asm/ptrace.h
>> +++ b/arch/tile/include/uapi/asm/ptrace.h
>> @@ -84,5 +84,11 @@ struct pt_regs {
>>  #define PTRACE_O_TRACEMIGRATE	0x00010000
>>  #define PTRACE_EVENT_MIGRATE	16
>>
>> +/*
>> + * Flag bits in pt_regs.flags that are part of the ptrace API.
>> + * We start our numbering higher up to avoid confusion with the
>> + * non-ABI kernel-internal values that use the low 16 bits.
>> + */
>> +#define PT_FLAGS_COMPAT		0x10000  /* process is an -m32 compat process */
> Can't understand how this connects to ptrace (I mean task->ptrace).

The idea is that while other architectures have things in their registers that identify a 32-bit execution environment, tile doesn't.  For example, PPC has a bit in the MSR and x86 has a different value in the CS register.  So for tile I just synthesize a bit to report in the existing "flags" word of the struct pt_regs.

> OK, let it live in asm/ptrace.h, but it seems that it is similar to
> (say) PT_FLAGS_RESTORE_REGS and thus it should be 8?

The other bits that live in that word are kernel-internal only, e.g. PT_FLAGS_RESTORE_REGS.  So they are not in the uapi header.  And in fact, we don't even report them out through the GETREGS API; we just set the single user-visible bit.  In principle we could even overlap the numbering and make PT_FLAGS_COMPAT have value "1" as well, but that seemed excessively confusing.

> And. arch/tile/kernel/ptrace.c:arch_ptrace() does
>
> 	case PTRACE_SETOPTIONS:
> 		/* Support TILE-specific ptrace options. */
> 		child->ptrace &= ~PT_TRACE_MASK_TILE;
> 		tmp = data & PTRACE_O_MASK_TILE;
> 		data &= ~PTRACE_O_MASK_TILE;
>
> AFAICS we need something like BUILD_BUG_ON(PTRACE_O_MASK_TILE & PTRACE_O_MASK),

I don't think so.  These are disjoint namespaces anyway.  PTRACE_O_MASK_TILE is for the actual PTRACE_SETOPTIONS ABI values.  PT_TRACE_MASK_TILE is for the values stored in task->ptrace.

> and
>
> 		ret = ptrace_request(child, request, addr, data);
> 		if (tmp & PTRACE_O_TRACEMIGRATE)
> 			child->ptrace |= PT_TRACE_MIGRATE;
>
> this also needs "ret == 0" check

The question is, what happens if we pass some illegal bit to the generic ptrace_request(), and also pass a valid PTRACE_O_MASK_TILE bit?  Currently we set the tile-specific bit, then report the error.  This is consistent with how ptrace_setoptions() handles a mix of legal and illegal bits.

> and "&= ~PT_TRACE_MASK_TILE" abobe should be moved here, no?

We could move it, but I don't think there's a correctness argument here.  Are you just seeing it would be easier to understand if the manipulation of child->ptrace was all on adjacent lines of code?  I agree that does seem a bit clearer; I'll post a separate patch for that.

> OTOH using /bin/grep I can't see where do we check ">ptrace & PT_TRACE_MIGRATE".

Yes, in our internal tree, this is part of the "dynamic homecache" code that lets us adjust the owner cache for some pages of a task's address space when it migrates to a new core.  It's not actually strictly dependent on that functionality but just got bundled in with it for convenience.  And since we haven't yet tried to return that code (it's somewhat intrusive to the core mm stuff) there's not much point to the ptrace extension. :-)  Oh well, we'll try to push it at some point.

> In short: confused ;)
>
> Oleg.

I hope this clears it up a bit.  Let me know if the patch makes sense to you now! :-)

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/