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

Add no-store configuration option for UEFI #9206

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dwmw2
Copy link
Contributor

@dwmw2 dwmw2 commented Jun 20, 2019

We don't need OSSL_STORE in the firmware. Add an option to disable it.
cf. https://www.mail-archive.com/devel@edk2.groups.io/msg01635.html

@levitte
Copy link
Member

levitte commented Jun 21, 2019

Shouldn't the UEFI target have it disabled by default, then? I.e an added disable attribute?

        disable         => [ 'store' ], 

@levitte
Copy link
Member

levitte commented Jun 21, 2019

I imagine more could be added in such an attribute. Is the openssl command interesting at all for firmwares? I would disable 'apps' as well (not available for the configuration command line, but if you look in apps/build.info, you will see that someone's listening)

@levitte
Copy link
Member

levitte commented Jun 21, 2019

Finally, I gotta ask, what else does UEFI not need? PEM? crypto/x509/by_*.c? The more I think about it, the more it looks like a huge slashing job...

@dwmw2
Copy link
Contributor Author

dwmw2 commented Jun 21, 2019

Shouldn't the UEFI target have it disabled by default, then? I.e an added disable attribute?
I imagine more could be added in such an attribute…

How much do you really want to know? :)

Intel's EDK2 is the open source implementation of the UEFI core which most of the production UEFI firmware in the world is based on. Its build system is… baroque. I stopped short of trying to beat the OpenSSL Makefiles to work with it. Instead the EDK2 build system runs Configure and then extracts the file list from OpenSSL's config data to create the OpensslLib.inf file for its own build system. It only does so for crypto/ and ssl/ and doesn't do apps. You can see the gory details at https://github.com/tianocore/edk2/blob/master/CryptoPkg/Library/OpensslLib/process_files.pl

So yes, there's a bunch of stuff which could be automatically disabled in the UEFI target, but it's also slightly redundant to do so.

Finally, I gotta ask, what else does UEFI not need? PEM? crypto/x509/by_*.c? The more I think about it, the more it looks like a huge slashing job...

The "huge slashing job" was when I went through and made OPENSSL_NO_STDIO/OPENSSL_NO_FP work :)

For a lot of things, if they're in standalone C files and not directly referenced from elsewhere, they just drop out of the build completely. Store wasn't like that because parts of it are referenced from elsewhere — it all got pulled in through things like err_load_crypto_string_int() which referenced those C units.

As for crypto/x509/by_*.c I think those survive by virtue of the fact that the file-based BIOs still exist even in the OPENSSL_NO_STDIO build. There might be merit in hiding them, and some other bits, behind an OPENSSL_REALLY_NO_FILE_IO_AT_ALL_SO_DONT_EVEN_PRETEND but I'm not massively inclined to go there.

PEM does get used for certificates.

@levitte
Copy link
Member

levitte commented Jun 21, 2019

... it all got pulled in through things like err_load_crypto_string_int() which referenced those C units.

That's surprising, I would have thought that only crypto/store/store_err.c gets involved... I haven't looked very closely at that one, though.

It sounds like we could do with some refactoring, dividing some files into smaller units. That would be healthy anyway, gods know that we have some units that are way too large and contain way too many things at the same time.

@dwmw2
Copy link
Contributor Author

dwmw2 commented Jun 21, 2019

That's surprising, I would have thought that only crypto/store/store_err.c gets involved... I haven't looked very closely at that one, though.

Yeah, I'm not sure it all got pulled in. I haven't looked at the precise trigger for them attempting to disable the store code, but it's a new directory full of code that they'd prefer to disable. It's also possible that it would have fallen out of the final link but actually failed to build in the first place, if there's (for example) a missing OPENSSL_NO_STDIO check around using BUFSIZ or something like that again.

Certainly the hacks on the UEFI build side (in the thread I replied to and added you to Cc) aren't the right way to do it. I think the STORE stuff fits squarely into the category of things that normally would have had no-xxx options to turn them off, and adding no-store is consistent and reasonable.

Now, it's entirely possible to go overboard and demand that we don't have a single machine instruction in the UEFI build of OpenSSL that isn't actively useful. That would be silly. I think we've settled on a relatively sane middle ground where we disable things that can be reasonably disabled, and live with the rest. Obviously, feedback on that strategy is welcome.

Honestly, I was glad that no-stdio already existed when I started trying to clean this stuff up, because then I was only "fixing" it and not trying to persuade you to take that as a new concept. :)

@levitte
Copy link
Member

levitte commented Jun 21, 2019

The way I would like store to go, it's as sensible to disable it as it is to disable, say, PEM.
It would make sense to disable selected loaders (such as the file loader).

@levitte
Copy link
Member

levitte commented Jun 21, 2019

Another way to put it is that it makes sense to disable back ends, it makes less sense to disable the framework...

@dwmw2
Copy link
Contributor Author

dwmw2 commented Jun 21, 2019

Makes sense. I'll try an EDK2 build with it included and look at excluding specific back ends, and see what it looks like. Thanks.

@t8m t8m added branch: master Merge to master branch triaged: feature The issue/pr requests/adds a feature labels Jul 23, 2021
@t8m t8m added this to the Post 3.0.0 milestone Jul 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch: master Merge to master branch triaged: feature The issue/pr requests/adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants