From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932515AbbJMO6G (ORCPT ); Tue, 13 Oct 2015 10:58:06 -0400 Received: from mx1.redhat.com ([209.132.183.28]:53788 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932174AbbJMO6D (ORCPT ); Tue, 13 Oct 2015 10:58:03 -0400 Date: Tue, 13 Oct 2015 16:54:39 +0200 From: Oleg Nesterov To: "Amanieu d'Antras" Cc: linux-kernel@vger.kernel.org, Andrew Morton , "Paul E. McKenney" Subject: Re: [PATCH] signal: Make the si_code check in rt_[tg]sigqueueinfo stricter Message-ID: <20151013145439.GA19175@redhat.com> References: <1444664034-22446-1-git-send-email-amanieu@gmail.com> <20151012155444.GA26302@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.18 (2008-05-17) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 10/12, Amanieu d'Antras wrote: > > On Mon, Oct 12, 2015 at 4:54 PM, Oleg Nesterov wrote: > > Yes, copy_siginfo_to_user() does __put_user((short)from->si_code). > > But SI_FROMUSER/SI_FROMKERNEL are internal kernel checks, we mostly > > use them in copy_siginfo_to_user(). > > > > And note that if ->si_code < 0 we simply do __copy_to_user(), so > > userspace can't see something which looks like "from kernel", in > > this case we do not truncate ->si_code. > > Ah my bad, I seem to have missed that case. So copy_siginfo_to_user > seems to be safe, but it is still an issue for copy_siginfo_to_user32 > which doesn't have this check. > > Maybe this should be fixed in the compat code instead? I agree, this doesn't look right... But I'm afraid it is too late to change this, this logic predates the git history. And unless I missed something copy_siginfo_to_user32() assumes that "from" was filled by copy_siginfo_from_user32(), but this is not true if the sender is 64bit task. And this all doesn't match the behaviour with 32bit kernel. Personally, I think we should leave this ancient code alone. But I agree that something like below looks right... I dunno. And I have no idea why copy_siginfo_to_user32() abuses '>> 16', it seems that "si_code & __SI_MASK" could work just fine. Oleg. --- x/arch/x86/kernel/signal_compat.c +++ x/arch/x86/kernel/signal_compat.c @@ -17,13 +17,14 @@ 3 ints plus the relevant union member. */ put_user_ex(from->si_signo, &to->si_signo); put_user_ex(from->si_errno, &to->si_errno); - put_user_ex((short)from->si_code, &to->si_code); if (from->si_code < 0) { + put_user_ex(from->si_code, &to->si_code); put_user_ex(from->si_pid, &to->si_pid); put_user_ex(from->si_uid, &to->si_uid); put_user_ex(ptr_to_compat(from->si_ptr), &to->si_ptr); } else { + put_user_ex((short)from->si_code, &to->si_code); /* * First 32bits of unions are always present: * si_pid === si_band === si_tid === si_addr(LS half)