From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752732AbbCXUwP (ORCPT ); Tue, 24 Mar 2015 16:52:15 -0400 Received: from mail-db3on0066.outbound.protection.outlook.com ([157.55.234.66]:15088 "EHLO emea01-db3-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752212AbbCXUwN (ORCPT ); Tue, 24 Mar 2015 16:52:13 -0400 Message-ID: <5511CE58.3020507@ezchip.com> Date: Tue, 24 Mar 2015 16:51:36 -0400 From: Chris Metcalf User-Agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Catalin Marinas CC: , , , , , "dingtianhong@huawei.com" , , , Will Deacon , "lizefan@huawei.com" , Bamvor Jian Zhang , , , , , , , , "linux-arm-kernel@lists.infradead.org" , Martin Schwidefsky , Heiko Carstens , "James E.J. Bottomley" , Helge Deller Subject: Re: [PATCH] tile: use si_int instead of si_ptr for compat_siginfo References: <1423563011-12377-1-git-send-email-bamvor.zhangjian@huawei.com> <20150210122718.GC32052@e104818-lin.cambridge.arm.com> <54DB3B60.4050100@huawei.com> <20150211154054.GD9058@e104818-lin.cambridge.arm.com> <54DDAF2B.2070707@huawei.com> <20150213104455.GA3508@e104818-lin.cambridge.arm.com> <54DE730D.3090100@ezchip.com> <20150214112220.GB10246@MBP.local> <54ECF309.3020509@ezchip.com> <201503161908.t2GJ8fs5021877@farm-0002.internal.tilera.com> <20150323120253.GA12757@e104818-lin.cambridge.arm.com> In-Reply-To: <20150323120253.GA12757@e104818-lin.cambridge.arm.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [12.216.194.146] X-ClientProxiedBy: BLUPR11CA0019.namprd11.prod.outlook.com (10.141.240.29) To AM3PR02MB0533.eurprd02.prod.outlook.com (25.160.6.139) Authentication-Results: gmx.de; dkim=none (message not signed) header.d=none; X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:AM3PR02MB0533; X-Microsoft-Antispam-PRVS: X-Forefront-Antispam-Report: BMV:1;SFV:NSPM;SFS:(10009020)(6009001)(6049001)(24454002)(51704005)(377454003)(479174004)(15975445007)(122386002)(23746002)(93886004)(33656002)(76176999)(87266999)(54356999)(65816999)(40100003)(59896002)(64126003)(46102003)(50466002)(77096005)(36756003)(62966003)(77156002)(66066001)(19580405001)(65956001)(47776003)(86362001)(2950100001)(19580395003)(83506001)(92566002)(110136001)(42186005)(50986999)(80316001)(87976001)(65806001)(7059030)(18886065003);DIR:OUT;SFP:1101;SCL:1;SRVR:AM3PR02MB0533;H:[10.7.0.41];FPR:;SPF:None;MLV:sfv;LANG:en; X-Exchange-Antispam-Report-Test: UriScan:; X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(601004)(5005006)(5002010);SRVR:AM3PR02MB0533;BCL:0;PCL:0;RULEID:;SRVR:AM3PR02MB0533; X-Forefront-PRVS: 0525BB0ADF X-OriginatorOrg: ezchip.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 Mar 2015 20:51:42.7914 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM3PR02MB0533 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org (+s390 and parisc maintainers) On 03/23/2015 08:02 AM, Catalin Marinas wrote: > On Mon, Mar 16, 2015 at 03:04:05PM -0400, Chris Metcalf wrote: >> To be compatible with the generic get_compat_sigevent(), the >> copy_siginfo_to_user32() and thus copy_siginfo_from_user32() >> have to use si_int instead of si_ptr. Using si_ptr means that >> for the case of ILP32 compat code running in big-endian mode, >> we would end up copying the high 32 bits of the pointer value >> into si_int instead of the desired low 32 bits. >> >> Signed-off-by: Chris Metcalf >> Cc: Catalin Marinas >> --- >> arch/tile/kernel/compat_signal.c | 9 +++------ >> 1 file changed, 3 insertions(+), 6 deletions(-) >> >> diff --git a/arch/tile/kernel/compat_signal.c b/arch/tile/kernel/compat_signal.c >> index 8c5abf2e4794..bca13054afb4 100644 >> --- a/arch/tile/kernel/compat_signal.c >> +++ b/arch/tile/kernel/compat_signal.c >> @@ -68,7 +68,7 @@ int copy_siginfo_to_user32(struct compat_siginfo __user *to, const siginfo_t *fr >> if (from->si_code < 0) { >> err |= __put_user(from->si_pid, &to->si_pid); >> err |= __put_user(from->si_uid, &to->si_uid); >> - err |= __put_user(ptr_to_compat(from->si_ptr), &to->si_ptr); >> + err |= __put_user(from->si_int, &to->si_int); >> } else { >> /* >> * First 32bits of unions are always present: >> @@ -93,8 +93,7 @@ int copy_siginfo_to_user32(struct compat_siginfo __user *to, const siginfo_t *fr >> break; >> case __SI_TIMER >> 16: >> err |= __put_user(from->si_overrun, &to->si_overrun); >> - err |= __put_user(ptr_to_compat(from->si_ptr), >> - &to->si_ptr); >> + err |= __put_user(from->si_int, &to->si_int); >> break; > It's usually the __SI_TIMER and __SI_MESGQ cases that matters here (the > latter already handled). I'm not entirely sure about the si_code < 0 > change. To be honest, I'm not even sure what path sets si_code < 0. I see that that is SI_FROMUSER(), but I don't see where it gets set. In any case, I guess a risk here is that of a 64-bit process doing a sigqueue() targeting a 32-bit process. It seems like an impossible problem for the 32-bit process to know whether the 64-bit process wrote a 32-bit pointer to the 64-bit sival_ptr field (and thus we should deliver the second 32-bit word of the union sigval to the 32-bit user), or if the 64-bit process wrote a 32-bit value to the 32-bit sival_int field (and thus we should deliver the first 32-bit word of the union sigval). Little-endian makes some things a little bit easier :-) All that said, my inclination is to use si_int here just because that's what we're using elsewhere. But I'm not entirely sure either. >> /* This is not generated by the kernel as of now. */ >> case __SI_RT >> 16: >> @@ -110,7 +109,6 @@ int copy_siginfo_to_user32(struct compat_siginfo __user *to, const siginfo_t *fr >> int copy_siginfo_from_user32(siginfo_t *to, struct compat_siginfo __user *from) >> { >> int err; >> - u32 ptr32; >> >> if (!access_ok(VERIFY_READ, from, sizeof(struct compat_siginfo))) >> return -EFAULT; >> @@ -121,8 +119,7 @@ int copy_siginfo_from_user32(siginfo_t *to, struct compat_siginfo __user *from) >> >> err |= __get_user(to->si_pid, &from->si_pid); >> err |= __get_user(to->si_uid, &from->si_uid); >> - err |= __get_user(ptr32, &from->si_ptr); >> - to->si_ptr = compat_ptr(ptr32); >> + err |= __get_user(to->si_int, &from->si_int); > We have a memset(to, 0, sizeof(*to)) on arm64 in this function but I > can't see it on tile. Some members or even half of si_ptr would be left > uninitialised. So here we presumably have the reverse problem, which is a 32-bit process doing a sigqueue() to a 64-bit process. If the 64-bit process inspects the sival_ptr, it does seem like it might find garbage in it. But it also doesn't seem portable in much the same way as the reverse direction; for a 32-bit process to signal a 64-bit process means the 64-bit process can't read si_ptr or it will get different values depending on what endianness is in force, so garbage is only part of the problem. I was modeling this code on the very similar code for parisc and s390. I've added their maintainers to the cc list for this email thread. I see that x86 uses si_ptr in its equivalent code, but of course it has no issues with big-endianness. -- Chris Metcalf, EZChip Semiconductor http://www.ezchip.com