Re: [PATCH 2/2] sigaction.2: wfix - Clarify si_addr description. - Yu, Yu-cheng

From: "Yu, Yu-cheng" <yu-cheng.yu@intel.com>
To: Stefan Puiu <stefan.puiu@gmail.com>, Borislav Petkov <bp@alien8.de>
Cc: lnx-man <linux-man@vger.kernel.org>,
	Alejandro Colomar <alx.manpages@gmail.com>,
	Michael Kerrisk <mtk.manpages@gmail.com>,
	Andy Lutomirski <luto@kernel.org>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	Florian Weimer <fweimer@redhat.com>,
	"H.J. Lu" <hjl.tools@gmail.com>,
	linux-kernel@vger.kernel.org, linux-api@vger.kernel.org
Subject: Re: [PATCH 2/2] sigaction.2: wfix - Clarify si_addr description.
Date: Thu, 11 Mar 2021 09:33:16 -0800	[thread overview]
Message-ID: <92fa4a4b-3100-cbd3-47cb-11072e4c6844@intel.com> (raw)
In-Reply-To: <CACKs7VBLnQTc_RgnXk8X-XgVRvR5_uXY0wL0snW7P5iDjpb8fA@mail.gmail.com>

On 3/11/2021 9:17 AM, Stefan Puiu wrote:
> Hi,
> 
> My 2 cents below.
> 
> On Tue, Mar 9, 2021, 16:33 Borislav Petkov <bp@alien8.de 
> <mailto:bp@alien8.de>> wrote:
> 
>     On Mon, Mar 08, 2021 at 01:46:07PM -0800, Yu, Yu-cheng wrote:
>      > I think the sentence above is vague, but probably for the reason
>     that each
>      > arch is different.  Maybe this patch is unnecessary and can be
>     dropped?
> 
>     Maybe.
> 
>     If you want to clarify it, you should audit every arch. But what
>     would that bring? IOW, is it that important to specify when si_addr
>     is populated and when not...? I don't know of an example but I'm
>     no userspace programmer anyway, to know when this info would be
>     beneficial...
> 
> 
> I've worked on projects where the SIGSEGV sig handler would also print 
> si_addr. When diagnosing a crash, the address that triggered the fault 
> is useful to know. If you can't reproduce the crash in a debugger, or 
> there's no core dump, at least you have an idea if it's a NULL pointer 
> dereference or some naked pointer dereferencing. So I think it's useful 
> to know when si_addr can be used to infer such information and when not.

At least for x86, the faulting ip is already in ucontext, and si_addr is 
mostly the memory address being accessed if that was the reason of the 
fault (i.e. the memory is not supposed to be accessed).  That way, the 
signal handler has both the instruction pointer and the memory address.

For shadow stack violation, for example, it is not because the memory 
being accessed; it is the instruction itself causing the violation.  It 
is unnecessary to duplicate the ip in si_addr.  Setting si_addr to zero 
also indicates this is not a memory type fault.

--
Yu-cheng