PATCH: PR ld/5149: Set PF_X for PT_GNU_RELRO segment only when needed

PATCH: PR ld/5149: Set PF_X for PT_GNU_RELRO segment only when needed

[H.J. Lu]

We currently set PF_X for PT_GNU_RELRO segment when the corresponding
PT_LOAD segment has PF_X. It isn't necessary. This patch checks
sections in PT_GNU_RELRO segment before setting PF_X.


H.J.
---
2007-10-22  H.J. Lu  <hongjiu.lu@intel.com>

	PR ld/5149
	* elf.c (assign_file_positions_for_non_load_sections): Set
	PF_X for PT_GNU_RELRO segment only if a section has
	SHF_EXECINSTR.

--- bfd/elf.c.relro	2007-10-22 12:22:32.000000000 -0700
+++ bfd/elf.c	2007-10-22 15:25:55.000000000 -0700
@@ -4728,27 +4728,57 @@ assign_file_positions_for_non_load_secti
 	  else if (p->p_type == PT_GNU_RELRO)
 	    {
 	      Elf_Internal_Phdr *lp;
+	      struct elf_segment_map *lm;
 
-	      for (lp = phdrs; lp < phdrs + count; ++lp)
+	      for (lm = elf_tdata (abfd)->segment_map, lp = phdrs;
+		   lm != NULL; lm = lm->next, lp++)
 		{
 		  if (lp->p_type == PT_LOAD
-		      && lp->p_vaddr <= link_info->relro_end
+		      && lp->p_vaddr < link_info->relro_end
 		      && lp->p_vaddr >= link_info->relro_start
-		      && (lp->p_vaddr + lp->p_filesz
+		      && ((lp->p_vaddr + lp->p_filesz)
 			  >= link_info->relro_end))
 		    break;
 		}
 
-	      if (lp < phdrs + count
-		  && link_info->relro_end > lp->p_vaddr)
+	      if (lm != NULL)
 		{
+		  unsigned long p_flags = lp->p_flags & ~PF_W;
+
 		  p->p_vaddr = lp->p_vaddr;
 		  p->p_paddr = lp->p_paddr;
 		  p->p_offset = lp->p_offset;
 		  p->p_filesz = link_info->relro_end - lp->p_vaddr;
 		  p->p_memsz = p->p_filesz;
 		  p->p_align = 1;
-		  p->p_flags = (lp->p_flags & ~PF_W);
+
+		  if ((p_flags & PF_X) != 0)
+		    {
+		      Elf_Internal_Shdr *hdr;
+
+		      /* Clear PF_X.  */
+		      p_flags &= ~PF_X;
+
+		      for (i = 0; i < lm->count; i++) 
+			{
+			  hdr = &elf_section_data (lm->sections[i])->this_hdr;
+
+			  if (hdr->sh_addr >= link_info->relro_start)
+			    {
+			      if (hdr->sh_addr >= link_info->relro_end)
+				break;
+
+			      /* Set PF_X only if a section has
+				 SHF_EXECINSTR.  */
+			      if ((hdr->sh_flags & SHF_EXECINSTR))
+				{
+				  p_flags |= PF_X;
+				  break;
+				}
+			    }
+			}
+		    }
+		  p->p_flags = p_flags;
 		}
 	      else
 		{

Re: PATCH: PR ld/5149: Set PF_X for PT_GNU_RELRO segment only when needed

[Alan Modra]

On Mon, Oct 22, 2007 at 03:31:09PM -0700, H.J. Lu wrote:
> 	PR ld/5149
> 	* elf.c (assign_file_positions_for_non_load_sections): Set
> 	PF_X for PT_GNU_RELRO segment only if a section has
> 	SHF_EXECINSTR.

I took a look at what glibc ld.so does with PT_GNU_RELRO and it
appears that p_flags are completely ignored.  The mprotect call
always uses PROT_READ.  So I don't really see the point of this
patch.

-- 
Alan Modra
Australia Development Lab, IBM

Re: PATCH: PR ld/5149: Set PF_X for PT_GNU_RELRO segment only when needed

[H.J. Lu]

On Wed, Oct 24, 2007 at 03:25:59PM +0930, Alan Modra wrote:
> On Mon, Oct 22, 2007 at 03:31:09PM -0700, H.J. Lu wrote:
> > 	PR ld/5149
> > 	* elf.c (assign_file_positions_for_non_load_sections): Set
> > 	PF_X for PT_GNU_RELRO segment only if a section has
> > 	SHF_EXECINSTR.
> 
> I took a look at what glibc ld.so does with PT_GNU_RELRO and it
> appears that p_flags are completely ignored.  The mprotect call
> always uses PROT_READ.  So I don't really see the point of this
> patch.
> 

I don't think we should use glibc as spec here. If it is case,
why not just set it to PROT_READ?

The question is if PT_GNU_RELRO can have the PF_X bit set and
what it means. Roland, do you have comments?


H.J.

Re: PATCH: PR ld/5149: Set PF_X for PT_GNU_RELRO segment only when needed

[Roland McGrath]

Sorry for the slow reply.

> On Wed, Oct 24, 2007 at 03:25:59PM +0930, Alan Modra wrote:
> > On Mon, Oct 22, 2007 at 03:31:09PM -0700, H.J. Lu wrote:
> > > 	PR ld/5149
> > > 	* elf.c (assign_file_positions_for_non_load_sections): Set
> > > 	PF_X for PT_GNU_RELRO segment only if a section has
> > > 	SHF_EXECINSTR.
> > 
> > I took a look at what glibc ld.so does with PT_GNU_RELRO and it
> > appears that p_flags are completely ignored.  The mprotect call
> > always uses PROT_READ.  So I don't really see the point of this
> > patch.
> > 
> 
> I don't think we should use glibc as spec here. If it is case,
> why not just set it to PROT_READ?
> 
> The question is if PT_GNU_RELRO can have the PF_X bit set and
> what it means. Roland, do you have comments?

My understanding from talking with Jakub is that it can make sense to have
SHF_EXECINSTR sections fall inside the PT_GNU_RELRO range in some cases.
Those are when PLT has to be writable for reloc, but using -z relro -z now.
That can arise on maybe sparc, alpha, and ppc32 -mbss-plt, that I know of.
(For those targets, with -z relro but without -z now, the relro range must
not cover the executable sections in question, since they are changed
dynamically after startup.)

For the targets with no read-only PLT ABI available, it really is
legitimate and unavoidable (for now) to have the executable-in-relro case
under -z now.  To support this case, ld.so should change to honor the
p_flags of the PT_GNU_RELRO phdr, at least its PF_X bit (and I'm inclined
to make it follow exactly the combination of PF_R|PF_X given, in case there
is some all-PLT PF_X-only case in theory).  

The constraint I think should be required of the linker to call its phdrs
correct is that p_flags of the PT_GNU_RELRO is a subset of the p_flags of
the PT_LOAD into which it falls (so that an ld.so without PT_GNU_RELRO
processing will always provide sufficient access), that it does not include
PF_W, and that its PT_LOAD does include PF_W (so that the PT_GNU_RELRO has
any actual purpose at all).

If I'm correct about all this, and that there is no other legitimate case
for bits other than PF_R in PT_GNU_RELRO, then I will do the glibc part
(and fix eu-elflint not to complain for the case).


Thanks,
Roland