jens-maus / amissl

:closed_lock_with_key: AmiSSL is the AmigaOS/MorphOS/AROS port of OpenSSL. It wraps the full functionality of OpenSSL into a full-fledged Amiga shared library that makes it possible for Amiga applications to use the full OpenSSL API through a standard Amiga shared library interface (e.g. web browsers wanting to support HTTPS, etc.)...
Apache License 2.0
88 stars 15 forks source link

Crashes on Tabor A1222 #38

Closed walkero-gr closed 1 year ago

walkero-gr commented 4 years ago

I am beta testing Tabor A1222 and the latest AmigaOS running on that machine. I get SSL issues on websites using latest IBrowse 2.5.1 and latest AWeb PPC version, which both use AmiSSL. I also have issues with pythonSSL module (http://os4depot.net/?function=showfile&file=library/misc/pythonssl.lha), which also uses AmiSSL to work.

The problem might be the FPU of the system, which as much as I know, doesn't exist.

Futaura commented 4 years ago

Can you attach a crashlog file?

walkero-gr commented 4 years ago

Thank you for your reply. I can't get a crashlog, but I did a few checks on various websites using openssl. Please check the attached text file openssl_checks.txt

If there are any tests that you would like me to do, please don't hesitate to tell me so.

Futaura commented 4 years ago

Ok, so it isn't crashing, but just not working correctly? There are a few things that somehow got messed up in the AmiSSL 4.3 release. First thing I would suggest is to check your AmiSSL:Certs files. You will probably find an abnormally large 5ad8a5d6.0 and b0f3e76e.0 - delete both and replace with the smaller correct files from the AmiSSL 4.2 archive. This can cause verification failures in my experience.

I'm not familiar with the Tabor A1222, but if it is missing some PowerPC instructions, those should be emulated in the kernel anyway - if they are not emulated correctly, the kernel needs fixing.

walkero-gr commented 4 years ago

Thank you for coming back so fast. I found those large certificates and replaced them with the ones from AmiSSL 4.2 archive, as you suggested. There was no change on the results. Still couldn't get the website certificates work. Any idea where the problem is?

salass00 commented 4 years ago

On the Tabor "openssl sha256 <filename>" generates a wrong sha-256 checksum compared to "md5sum ALGORITHM=sha-256 <filename>" or "sha256sum <filename>" on Linux.

On my Sam460 the openssl command generates a correct checksum (testing with AmiSSL 4.3).

Futaura commented 4 years ago

AFAIK, Tabor doesn't have AltiVec, right? In which case attached is the non-AltiVec sha256 code:

sha256-ppc.s.txt

Could still be that the problem is elsewhere though.

walkero-gr commented 4 years ago

@futaura I really don't know if the lack of Altivec is the problem, because on X5000 it works just fine, where I never had any issues. But if you prepare a test version, I would be glad to test it for you.

salass00 commented 4 years ago

After adding the amissl libraries to devs:load-time-blacklist the sha-256 is generated correctly, and I'm also able to use ssh2-handler to connect to my PC.

salass00 commented 4 years ago

@walkero-gr Futaura was only asking because a different code path is used by openssl when an altivec unit is available.

I think the problem is because ltemu uses r2 and this clashes with AmiSSL's use of the register (because of -mbaserel).

walkero-gr commented 4 years ago

tried on my system what @salass00 proposed and now amissl works like a charm, tried on IBrowse, AWeb and python scripts. So it seems there is a problem with SPE translation. I will inform the ExecSG or AmigaOS development team for this. I guess this should be fixed by their side and not by the AmiSSL developers, right?

Futaura commented 4 years ago

Hmmm... I'm not sure there is a way around it other than blacklisting. It seems r2 was chosen and ExecSG's previous use of r2 for baserel abandoned. Bad choice IMHO. I'm not sure how all that A1222 magic works, but maybe even you should blacklist IBrowse too, since it's libraries rely on r2 for baserel, but maybe the difference is that IBrowse probably doesn't use floating point math. For AmiSSL this cannot rely be changed due to OpenSSL's design heavily relying on global variables.

salass00 commented 4 years ago

I think it should be possible to migrate AmiSSL to small data model, which uses r13 instead of r2, though it will probably be a lot of work. Apart from better A1222 compatibility it would be nice to be able to compile AmiSSL with a newer gcc than version 4.0.4 (last version to support -mbaserel).

FWIW I at least intend to try making a small data model version of AmiSSL.

Futaura commented 4 years ago

There were already some discussions about using small data, but isn't that restricted to 64K? OpenSSL's data usage (excluding rodata) is already over 32K and unfortunately it will most likely only increase much more. Data usage has already doubled in AmiSSL v4 compared to V3.

There are lots of caveats to the way baserel is employed in AmiSSL. For instance some global variables are not baserel. The critical thing is that OpenSSL must be modified as little as possible, as to not complicate updates.

Feel free to give it a go though - I would, but there are some issues with AmiSSL 4.3 which need addressing first, mainly 68k related and I also want to merge the latest OpenSSL in, which @jens-maus was in the middle of last year, but I've been having trouble reaching him :-(

jens-maus commented 4 years ago

Sorry guys for not replying earlier, but I am a lot busy with other stuff and don't have any time to spent for my amiga projects. In additon, my motivation to work on this old amiga platform has almost reached zero since it is simply outdated and I don't see any practical benefit in keeping it alive. What it is good for in 2020?!? No modern web browser, no modern APIs and simply IMHO no practical use anymore. Furthermore, the whole amiga "community" is simply not alive anymore. Almost zero developers and almost zero users. Looking at the number of YAM users (<= 100) compared to my linux projects (> 20k) I am maintaining does not justify any more substantial work on amiga projects IMHO. So sorry, but you guys need to continue without me for a while...

Futaura commented 4 years ago

@jens-maus I've sent you an email to follow up on the last one I sent n September - not sure if you received it?

I'm not sure YAM is a good example - I don't think many rely on POP3 today, or indeed even IMAP (where Gmail is concerned). AmiSSL 4.3 has nearly 3,000 downloads from Aminet. But, anyway, I do understand where you're coming from and I am happy to continue with development once I have the required tools.

Futaura commented 2 years ago

Have been taking another look at switching from r2 to r13, but it is looking like a non-starter to me. In OpenSSL 3.0.4, we currently have data of 47K (excluding .rodata) and that is most likely going to go over 64K sooner or later. However, correct me if I'm wrong, but small data references use signed 16-bit offsets, which means there is a limit of 32K (similar to on m68k). Also, as we need to use -mcheck68kfuncptr, even if we did switch to small data, we could only go as high as gcc 4.2.4 because -mcheck68kfuncptr is broken or not implemented in newer versions.

salass00 commented 2 years ago

No, limit is 64K both on PPC and m68k. The small data is not set to the beginning of the small data segment but 0x8000 bytes in so that negative offsets can be used as well.

Futaura commented 2 years ago

Ok. The problem is still going to be when OpenSSL inevitably goes beyond 64K, unfortunately. Currently the OS3 build of OpenSSL is using 32-bit offsets for a4 relative data, similar to the r2 relative data on OS4. Ideally, we could do with baserel being added back to the OS4 compilers, but using r13 instead of r2. It's a shame r13 wasn't used originally - I guess there shouldn't be a conflict between -msdata and -mbaserel, as it wouldn't really make sense to use both together.

Futaura commented 2 years ago

@salass00 When using -msdata -G65536, this has the consequence of moving a whole bunch of stuff out of .rodata into .sdata. Are you aware of any compiler options to prevent this, so that read only data stays in .rodata?

I've successfully changed from r2 to r13 baserel usage in another project, which use far less global data than AmiSSL, and was hoping to apply these changes to AmiSSL, but .sdata ends up 0x89248 in size, so it obviously fails to link.

salass00 commented 2 years ago

The -mno-readonly-in-sdata option should fix that.

Futaura commented 2 years ago

Ok, that's good to know and indeed works as expected in GCC 11. However, I guess that's a reasonably new option, as it is not available in GCC 4 😞. So, back to square one as AmiSSL needs the -mcheck68kfuncptr option, which disappeared after GCC 4.2.4. If the function pointer use was isolated in OpenSSL, I could easily add the codes and 68k checks in the code myself. Unfortunately, with currently 1,800 different calls to a function pointer in OpenSSL, that's impossible (although, probably a fair proportion of those are likely internal function pointers, rather than functions passed to API functions by the application).

ksdhans commented 1 year ago

I just bumped into this issue again when updating to AmiSSL v5.6. Blacklisting is a constant chore, because every new sub-library must be added to the LTE blacklist. If a fix to get it playing nice with LTE isn't feasible, then perhaps the installer could automatically add AmiSSL's sub-library to the LTE...

salass00 commented 1 year ago

You don't need to add the sublibraries individually since the elf.library supports AmigaDOS style pattern matching in the LTE blacklist since a few versions back.

Futaura commented 1 year ago

@salass00 Do you know what happens when a FPU instruction is executed when the code is blacklisted? Does it just crash? Or does it trigger a trap/interrupt which the kernel then emulates (like it would do for missing instructions on the 68060, for example)?

I'm presuming the reason AmiSSL gets patched is because elf.library detects the present FPU opcodes (there are some FPU asm optimized modules, which AmiSSL disables completely for Tabor). However, I'm still confused why r2 should still be getting trashed if AmiSSL never triggers the ltemu anyway.

Looking at the elf.library code, in conjunction with my recent changes, given the patching takes place in ElfLoadSeg() after relocs have been applied, it could be changed to automatically detect that the section is using baserel code and disable patching, without it needing to be explicitly blacklisted (I already added this detection for the CopyDataSegment() rewrite).

Does amisslmaster.library have to be blacklisted too?

ksdhans commented 1 year ago

@Futaura

Do you know what happens when a FPU instruction is executed when the code is blacklisted? Does it just crash? Or does it trigger a trap/interrupt which the kernel then emulates (like it would do for missing instructions on the 68060, for example)?

FPU instructions still work, because the system will use software FPU emulation. IIRC, that fallback emulator uses a trap to intercept the invalid instructions. It's noticeably slower than the Load-Time-Emulator (LTE) which patches the instructions to emulate them using the SPE unit.

Does amisslmaster.library have to be blacklisted too?

Yes. I just tested removing it from my blacklist, and a DSI is triggered when starting ZitaFTP.

Futaura commented 1 year ago

@ksdhans

FPU instructions still work, because the system will use software FPU emulation. IIRC, that fallback emulator uses a trap to intercept the invalid instructions. It's noticeably slower than the Load-Time-Emulator (LTE) which patches the instructions to emulate them using the SPE unit.

Ah, ok - that makes sense now.

Does amisslmaster.library have to be blacklisted too?

Yes. I just tested removing it from my blacklist, and a DSI is triggered when starting ZitaFTP.

I'm now trying to understand why r2 is getting trashed in the first place. The patched/trampoline LTE code would appear to always save and restore r2 on entry and exit, so r2 should not get trashed when the patched code runs. Plus I don't think amisslmaster.library uses any FPU instructions at all, but it is baserel compiled too, so still relies on r2. I guess I must have missed something somewhere.

Futaura commented 1 year ago

@ksdhans I would be interested in seeing a crashlog with amisslmaster.library not blacklisted and amissl_v#?.library blacklisted.

ksdhans commented 1 year ago

I tried to capture a crash-log, and it's gone. I have commented out both amisslmaster.library and amissl_v#?.library from the blacklist, and no longer get any crash.

I have absolutely no idea what's changed. Scanning back at previous comments, it did crash previously with AmiSSL v5.6. Yet now, it doesn't. I don't remember making any other changes to the OS in the meantime.

Could someone else with an A1222/Tabor try AmiSSL 5.6 without the LTE blacklist?

ksdhans commented 1 year ago

That said, amissl_v#?.library does have to be in the blacklist, or I can't connect to ZitaFTP. No crash, though.

Perhaps the change since my last test is that the computer has been switched off. Whatever was in memory when it was crashing is completely gone...

salass00 commented 1 year ago

For me the problem with not having amissl blacklisted (I have not tested lately) was never crashes, just wrong checksums and authentication failing in ssh2-handler.

Wrong results being generated with LTE could also be down to the e500 SPE unit not being fully IEEE compliant, as the exception based FPU emulator uses soft float instead of SPE.

Futaura commented 1 year ago

Do you know if the LTE replaces more instructions than just FPU ones? Just wondering if any others may be patched so that the SPE is used instead. It looks to me like r2 is not getting trashed as it is getting saved/restored in the trampoline patch code, so it was probably wrong to think r2/baserel was the issue in the first place. It's possible that there is a bug in ltemu.resource somewhere, which I presume is re-entrant (otherwise that would cause problems too).

I've checked amisslmaster.library and it appears to contain no FPU instructions or references to FP registers, so that should not need blacklisting as it will not get LTE patched. Of course, amissl_v#?.library does use the FPU, mainly in the elliptic curve routines, IIRC. FPU instructions or registers should not actually be getting used at all in terms of just computing a sha hash, or at least not in any meaningful way (a floating point parameter is used when setting up the random number generator).

Does the following program also yield an incorrect hash (compile with -lamisslauto) when the library is not blacklisted?

#include <stdio.h>
#include <proto/amissl.h>

int main(int argc, char **argv)
{
   FILE *file;
   unsigned char *buffer, hash[SHA256_DIGEST_LENGTH];
   int size, i;

   if (argc > 1) {
      if ((file = fopen(argv[1], "r"))) {
         fseek(file, 0, SEEK_END);
         size = ftell(file);
         fseek(file, 0, SEEK_SET);
         if ((buffer = malloc(size))) {
            fread(buffer, size, 1, file);
            IAmiSSL->SHA256(buffer, size, hash);
            for (i = 0; i < SHA256_DIGEST_LENGTH; i++) {
               printf("%02x",hash[i]);
        }
            printf("  %s\n",argv[1]);
            free(buffer);
     }
         fclose(file);
      }
   }

   return 0;
}
ksdhans commented 1 year ago

Sorry for the delay. The test program yields different results with and without the LTE: < 1dcbcdf1cfe47cbc93338efefe4c793db7fecaabb1ff340deb8c83a9e0cdc707 amissl-test

9fd33a8fefdc26dc60fc4e75998c5d27fa425414074911baaaa4c9b219111e30 amissl-test

The first result is with amissl_v#?.library in the blacklist; the second one is with LTE enabled.

Futaura commented 1 year ago

It has taken me a while, but I think I understand the LTE's issues now, and there are two distinctly different issues:

  1. Although the LTE saves and restores r2 at entry/exit, if the instruction that the LTE is replacing itself uses r2 then that will break as the patched code overwrites r2 for its own use. If you try setenv ELF.debug 3, with AmiSSL libraries flushed from memory, then running anything that opens AmiSSL, I'm pretty sure you will seem some lines that say "Can't handle r2 case yet". A quick grep of the AmiSSL assembly indicates there are just 9 instructions which would trigger this error. which are basically simply load/stores to/from a floating point register with a r2-relative memory address.

This could be partly addressed by stopping the LTE from inserting a patch where it already detects 'r2' usage it can't handle, unless the code can be improved to workaround 'r2' (similar to how it does so for 'r1'), but this would probably mean increasing the trampoline size. That said, I'm not even sure AmiSSL would benefit from this.

  1. However, these instructions are not necessarily instructions at all - a lot of the PPC-asm optimised routines in OpenSSL place a huge amount of constants into the .text section. If any of these constants are matched by the LTE (any matched instructions - not necessarily involved r2, as that is irrelevant here), they get replaced with the patched instruction 32-bit word, hence breaking the routine. Obviously, the LTE can't differentiate between code and data in the .text section. GCC normally puts constants in .rodata, but I'm not sure if there are any instances of it putting a simple 32-bit constant in .text - this would also cause the LTE to break the code. The SHA PPC asm code contains some constant data too, so I'd bet the LTE is changing that.

This issue is the biggest one and is more tricky to resolve. It is probably the reason for other applications needing to be blacklisted. In C code, if GCC can decide to put a constant into .text instead of .rodata, then that poses a serious flaw with the LTE as the LTE assumes .text can contain only code, not data. In terms of PPC asm, if I could change the OpenSSL code to place the constants in .rodata, I would - however, I'm not sure if this woud have a negative impact on performance, as the way it is currently written, the data and code will most likely reside in the cache together, whereas this would not be the case if the data was moved to .rodata (I'm trying to remember if the we have separate data and instruction caches on OS4/PPC).

ksdhans commented 1 year ago

That's quite a complicated problem indeed. I remember hitting the "constant data in the wrong place" problem myself. Something about the way I added the data caused it to get put in the wrong area, resulting in really weird failures.

Futaura commented 1 year ago

I suppose the OpenSSL team have done it this way to cater for all PPC targets, since all the PPC asm modules are position independent with no relocs. I'm looking at the SHA256/512 code at the moment as that is the most straightforward. One thing I can't figure out is that at the end of every function, following the blr, there are some constants, but I have no clue what they're for (they do not appear to get used, but I could be wrong). Here's a snippet: anyone have any ideas?

    lwz 31,188(1)
    mtlr    0
    addi    1,1,192
    blr 
.long   0
.byte   0,12,4,1,0x80,18,3,0
.long   0
.align  4
.Lsha2_block_private:
    lwz 0,0(7)
    lwz 16,0(31)
    rotrwi  3,12,6
    rotrwi  4,12,11

I'm wondering if it is for a different ABI or platform, perhaps.

ksdhans commented 1 year ago

That's weird. No idea why that would be there.

Futaura commented 1 year ago

I think it could perhaps be TOC related for other platforms or maybe debug related. I removed them all anyway and it still works.

Can you try the library below with AmiSSL not blacklisted at all and see if it works?

amissl_whitelist.zip

This has all the constant data in .rodata, so the LTE will no longer corrupt it. The changes made no difference in speed on my A1XE, so should be true overall I hope. Certainly the SHA checksums should work now and so should AES, CHACHA and POLY1305 (not sure if these got messed with by the LTE too, but thought it was safest to assume the worst). The other asm modules didn't use any inline data. I also stripped out the POWER8 asm code which is redundant for us.

ksdhans commented 1 year ago

I just tested it, and I can connect to ZitaFTP from another machine and upload files without AmiSSL needing to be in the LTE blacklist.

Congratulations on figuring it out.

Futaura commented 1 year ago

Excellent! Glad to have sorted it out, even if it did mean having to read the elf.library source code to figure out why. But, at least we now know that baserel/r2 isn't an issue after all.