Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Writable .plt? #3

Closed
rui314 opened this issue Oct 13, 2022 · 11 comments
Closed

Writable .plt? #3

rui314 opened this issue Oct 13, 2022 · 11 comments

Comments

@rui314
Copy link

rui314 commented Oct 13, 2022

On page 33, .plt is defined as SHT_PROGBITS with attributes of SHF_ALLOC + SHF_WRITE + SHF_EXECINSTR. If we just follow the words, it ends up with a writable code segment. Shouldn't we drop + SHF_WRITE?

@Andreas-Krebbel
Copy link
Member

Looks like a bug to me. In fact our .plt section isn't writable and I don't think there is a situation where we have to make it writable but I have to double-check.

It has been like that in the first version of the ABI:
https://legacy.redhat.com/pub/redhat/linux/7.1/es/os/s390x/doc/lzsabi0.pdf

@rui314
Copy link
Author

rui314 commented Oct 13, 2022

Yeah, it looks like it's just a spec bug. We should be able to remove + SHF_WRITE from the spec.

Note that on SPARC, .plt is actually SHF_ALLOC + SHF_WRITE + SHF_EXECINSTR. After a dynamic symbol is resolved, the PLT resolver modifies the code in .plt so that the PLT entry directly jumps to the right place next time. That's horrible from the security point of view, though.

aarnez added a commit that referenced this issue Oct 13, 2022
The table titled "special sections" displays the SHF_WRITE flag as being
set in the ".plt" section's attributes.  This looks like a documentation
bug that has been present since the original ABI.  The .plt doesn't need
to be writable, and it would pose a security risk if it were.

To my knowledge (verified with Ulrich Weigand and Andreas Krebbel),
existing tools never mark the .plt writable.

To fix the issue, just drop the SHF_WRITE flag from the ".plt" section's
attributes.
@aarnez
Copy link
Collaborator

aarnez commented Oct 13, 2022

Thanks for reporting this. Yes, this looks like a bug.
I pushed commit 9bb099d to fix this in the spec.
The change will be reflected in the next release.

@aarnez aarnez closed this as completed Oct 13, 2022
@rui314
Copy link
Author

rui314 commented Oct 14, 2022

I found other minor errors.

Page 47

There's a typo: ELFDATA64MSB → ELFDATA2MSB

Page 48

A symbol table entry’s st_value field is the symbol value. If that value represents a section offset or a virtual address a symbol type is of STT_FUNC, it must be halfword aligned. This enables use of CIA-relative addressing instructions such as LARL.

... because data symbols don't have to be aligned to a 2 byte boundary.

@uweigand
Copy link
Member

I found other minor errors.

Page 47

There's a typo: ELFDATA64MSB → ELFDATA2MSB

Good catch, thanks!

Page 48

A symbol table entry’s st_value field is the symbol value. If that value represents a section offset or a virtual address a symbol type is of STT_FUNC, it must be halfword aligned. This enables use of CIA-relative addressing instructions such as LARL.

... because data symbols don't have to be aligned to a 2 byte boundary.

So this is more interesting. We do align all symbols, including data symbols, to a 2 byte boundary, specifically to enable use of LARL to load symbol addresses.

There are a few corner cases where a compiler will actually generate a non-2-aligned data symbol, e.g. as a result of a global variable definition using an "aligned(1)" attribute. But we've considered those cases, which are all triggered by non-standard language extensions, to not be covered by this default platform ABI, but rather extensions there as well. Therefore, we've had the ABI stating the 2-byte alignment requirement unconditionally.

(Without the ABI stating the requirement, it would be difficult for a compiler to reliably use LARL to access any non-local symbol. Having to avoid LARL here would significantly pessimize generated code.)

@rui314
Copy link
Author

rui314 commented Oct 14, 2022

So this is more interesting. We do align all symbols, including data symbols, to a 2 byte boundary, specifically to enable use of LARL to load symbol addresses.

Ooh, I didn't know that until now. Thank you for pointing it out!

Now I wonder if we should print out an error message from the linker if an R_390_*DBL relocation is applied to an odd address. It is not expected to happen, and if it happens, I guess it's almost certainly an error.

@uweigand
Copy link
Member

Now I wonder if we should print out an error message from the linker if an R_390_*DBL relocation is applied to an odd address. It is not expected to happen, and if it happens, I guess it's almost certainly an error.

I thought we did already, but a quick test shows that apparently we don't. But I agree this is an error and should result in a message (rather than silently dropping the low bit).

@Andreas-Krebbel should this be checked by GNU ld?

@rui314
Copy link
Author

rui314 commented Oct 15, 2022

It'd be also good if the spec specifies which relocations should be checked for overflow. Other psABIs specify that. I can use my common sense to add relocation overflow checks, but that behavior should be consistent across linkers.

@Andreas-Krebbel
Copy link
Member

Now I wonder if we should print out an error message from the linker if an R_390_*DBL relocation is applied to an odd address. It is not expected to happen, and if it happens, I guess it's almost certainly an error.

I thought we did already, but a quick test shows that apparently we don't. But I agree this is an error and should result in a message (rather than silently dropping the low bit).

@Andreas-Krebbel should this be checked by GNU ld?

Yes, I'll have a look. Might require some more changes. If I remember correctly ld is abusing the lowest order bit of the symbol address as a flag for something. Horrible, I know ;)

@aarnez
Copy link
Collaborator

aarnez commented Oct 17, 2022

Page 47

There's a typo: ELFDATA64MSB → ELFDATA2MSB

Thanks! Commit dcfabd8 has just been pushed to fix this typo.

@Andreas-Krebbel
Copy link
Member

I've posted a patch to issue an error for *DBL relocs on misaligned symbols:
https://sourceware.org/pipermail/binutils/2022-October/123668.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants