linux-kernel - Re: [PATCH] inet/connection_sock: prefer _THIS_IP_ to current_text

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:   Wed, 1 Aug 2018 22:08:18 -0700
From:   Nathan Chancellor <natechancellor@...il.com>
To:     David Miller <davem@...emloft.net>
Cc:     ndesaulniers@...gle.com, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] inet/connection_sock: prefer _THIS_IP_ to
 current_text_addr

On Wed, Aug 01, 2018 at 09:58:25PM -0700, Nathan Chancellor wrote:
> On Wed, Aug 01, 2018 at 06:42:08PM -0700, David Miller wrote:
> > From: Nick Desaulniers <ndesaulniers@...gle.com>
> > Date: Wed,  1 Aug 2018 14:57:59 -0700
> > 
> > > -			 sk, what, when, current_text_addr());
> > > +			 sk, what, when, (void *)_THIS_IP_);
> > 
> > Is a fugly macro in all caps really better than a function()?
> > 
> > I'm really surprised that _THIS_IP_ is being chosen over
> > current_text_addr(), seriously.
> 
> Hi Dave,

Sorry for the second email so quick after the first, needed to clarify
two things.

> 
> current_text_addr is only used in five places in the entire kernel and
> this is the only non-architecture specific use.
> 
> include/net/inet_connection_sock.h:227:			 sk, what, when, current_text_addr());
> arch/sh/include/asm/kexec.h:64:		newregs->pc = (unsigned long)current_text_addr();
> arch/sh/kernel/dwarf.c:602:		pc = (unsigned long)current_text_addr();
> arch/x86/include/asm/kexec.h:135:		newregs->ip = (unsigned long)current_text_addr();
> arch/parisc/kernel/unwind.c:442:	r.iaoq[0] = (unsigned long) current_text_addr();
> 
> We're trying to merge it with _THIS_IP_ before for most architectures,

because for most...

> it is defined nearly identically to _THIS_IP_:
> 
> #define _THIS_IP_  ({ __label__ __here; __here: (unsigned long)&&__here; })
> 
> #define current_text_addr() ({ __label__ _l; _l: &&_l; })
> 
> * arc
> * arm
> * arm64
> * h8300
> * hexagon
> * m68k
> * mips
> * nds32
> * nios2
> * openrisc
> * powerpc
> * riscv
> * unicore32
> * xtensa
> 
> Ultimately, we're trying to turn off a new Clang warning in _THIS_IP_
> which requires some changes to work around what is believed to be a GCC
> bug (see https://lore.kernel.org/patchwork/patch/969101/).

and I should have mentioned that current_text_addr suffers from the
exact same issue so we're trying not to duplicate it 14 times across
the kernel.

> I guess the alternative to this patch is to just define current_text_addr
> as _THIS_IP_ for all of those architectures, I am not sure how that
> filters into Nick's plan (I think the goal of this was to try and avoid
> getting all of the architecture folks involved).
> 
> Thanks for your time!
> Nathan