search

tpm2-software / tpm2-tss

OSS implementation of the TCG TPM2 Software Stack (TSS2)
https://tpm2-software.github.io
BSD 2-Clause "Simplified" License
730 stars 359 forks source link

Code assumes little endian #328

Closed flihp closed 7 years ago

flihp commented 7 years ago

The thing that kills me is that we require the code be changed for use on bit endian system. ARM exists. ARM users (and distro packagers) shouldn't have to make code changes to use this code. Autoconf will do this for us with the AC_C_BIGENDIAN macro. Use it.

wcarthur1 commented 7 years ago

But...some processors can be BE or LE. Microblaze for example.

On Fri, Feb 3, 2017 at 2:34 PM, Philip Tricca notifications@github.com wrote:

The thing that kills me is that we require the code be changed for use on bit endian system. ARM exists. ARM users (and distro packagers) shouldn't have to make code changes to use this code. Autoconf will do this for us with the AC_C_BIGENDIAN macro. Use it.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/01org/TPM2.0-TSS/issues/328, or mute the thread https://github.com/notifications/unsubscribe-auth/AEdleNHdcCTPR9uNuFzWS6kV1zj5KosNks5rY4FIgaJpZM4L2uAw .

flihp commented 7 years ago

microblaze can be both BE and LE at the same time?

flihp commented 7 years ago

oh I get it, it's an FPGA soft CPU, but it has to be either / or

flihp commented 7 years ago

So what's the concern @wcarthur1?

wcarthur1 commented 7 years ago

If you use some macros or functions for handling endianness that automatically do the "right thing" I'm not sure they'll work right for cpus that can go both ways. For those how will htonl and other similar macros know?

On Fri, Feb 3, 2017 at 3:00 PM Philip Tricca notifications@github.com wrote:

So what's the concern @wcarthur1 https://github.com/wcarthur1?

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/01org/TPM2.0-TSS/issues/328#issuecomment-277347369, or mute the thread https://github.com/notifications/unsubscribe-auth/AEdleIN6XJDOyTmH6DfTjHkmwjcKCX4Kks5rY4dVgaJpZM4L2uAw .

flihp commented 7 years ago

That may be the case, but it'll be hard to do much worse than the current situation:

  1. LE only.
  2. Code changes required for BE systems (aka no ARM support).
  3. 99% certain we do not support this microblaze architecture you're playing with (unless you have it configured as LE).

By the way have you got that to work?

williamcroberts commented 7 years ago

Every ARM system I have worked on has been configured for LE. ARM and other processors that support both modes are not switchable at run-time, if they are, no one does it.

flihp commented 7 years ago

Thx @williamcroberts, I don't have any experience with ARM really. Useful data. I'm just trying to improve the situation with this issue. I'm sure that whatever I come up with won't be perfect but if I can get us sane LE support I'll call it a "win". I'll worry about the more exotic stuff when it becomes an issue.

williamcroberts commented 7 years ago

What you want is sane big endian support, everything is LE so you already support that.

williamcroberts commented 7 years ago

In this PR for TPM2.0 tools, I have code for dealing with big-endianess (EDIT: with unit tests): https://github.com/01org/tpm2.0-tools/pull/213

I also don't determine endianess at build but rather detect at runtime and the make adjustments as needed.

flihp commented 7 years ago

My current approach is to use endian.h if available and just let the platform handle this stuff for me. We should probably turn this into a library since I'm sure other tools will need it.

williamcroberts commented 7 years ago

I stayed away from endian.h sys/endian.h arpa/inet.h as it can be a nightmare when porting around (from experience), the code is simple enough to implement, but that's just my two cents.

flihp commented 7 years ago

Yeah no way I'm touching arpa/init.h with a 10 foot pole, and sys/endian.h is OpenBSD only (according to the docs). Sounds like you're just writing your own version of endian.h then with a custom byte swapping thing?

williamcroberts commented 7 years ago

Essential yes. The endian.h routines like htobe32 and htole32 just check to see if the target endianess is different (in my code that would be a call to is_big_endian) followed by a byteswap (in my code string_bytes_convert_32())

The files.h routines are very similair to htobe16|32... in endian.h in that they check the host endianess and convert to be when necessary. Additional, the file routines write to the FILE object.

endian.h is not cross platform, I try to avoid including non-portable code when ever an easy solution is present.

flihp commented 7 years ago

Spent some time on this over the weekend and the additional level of abstraction required to use endian.h and the other platform specific equivalents is a bit cumbersome. The options for endian conversion are just too many (endian.h, the windows equivalent, compiler built-ins etc) and the solution ends up being a header with if elseif elseif ... endifmadness. Your points above are all spot-on. I'll back the endian.h stuff out of my prototype to simplify. Thanks for the input.

williamcroberts commented 7 years ago

Glad I can be of service for a change!

flihp commented 7 years ago

resolved in 7360972a1b3b203d20a264b391039f3934966f6c