Re: [edk2] Drop CSM support in OvmfPkg?

[David Woodhouse]

On Thu, 2019-01-24 at 01:48 +, Ni, Ray wrote:
> David,
> I think we got an agreement here to move CSM components in OvmfPkg.
> I prefer we firstly clone the required CSM components in OvmfPkg right no.
> Finally I can remove the IntelFrameworkModulePkg/IntelFrameworkPkg in one 
> patch.
> (I say "finally" because OVMF CSM dependency is not the only case that 
> prevent removing
> the two framework packages.)
> 
> Would you like to do the clone? Or if you are busy, I can do that.

I keep asking this question, I don't believe I've seen an
answer. Apologies if I've missed it.

Is this code genuinely not going to continue to exist anywhere else in
the Intel ecosystem, any more?

No TianoCore-based images from this point forth are ever going to even
have the option of supporting CSM?

Unless some third party also chooses to fork the CSM support code and
keep it on for themselves? 



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] Drop CSM support in OvmfPkg?

[David Woodhouse]

On Wed, 2019-01-23 at 10:46 +0100, Laszlo Ersek wrote:
> I'm fine if we move the generic CSM components into OvmfPkg, however I'm
> going to ask David to assume reviewer responsibilities for them.
> 
> Given the current format of "Maintainers.txt", we couldn't spell out the
> exact pathnames of the CSM components, so we'd add a line like
> 
> R: David Woodhouse 
> 
> under OvmfPkg. There is "prior art" for this pattern, see:
> 
> R: Anthony Perard 
> R: Julien Grall 
> 
> Because Anthony and Julien are the authority on Xen-related code under
> OvmfPkg. (See commit 337fe6a06eda, "Maintainers.txt: add Xen reviewers
> to OvmfPkg", 2017-09-26.)
> 
> 
> If we keep CSM support in OvmfPkg in any form at all, then I would
> prefer holding all the related stuff in the core edk2 repository (with
> the above Reviewership), over requiring people to deal with multiple
> repositories. I agree (from experience) that PACKAGES_PATH / multiple
> workspaces work fine, but in this case I think keeping one shared
> history is an advantage.

This all makes sense to me.


smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] Drop CSM support in OvmfPkg?

[David Woodhouse]

On Wed, 2019-01-23 at 07:12 +0100, Gerd Hoffmann wrote:
> > A one-size-fits-all BIOS using OVMF+CSM is very much
> > preferable.
> 
> Building a one-size-fits-all BIOS is pretty much impossible due to CSM
> being incompatible with secure boot.

Booting with CSM is incompatible with Secure Boot, of course.

But it doesn't prevent Secure Boot unless we actually use the CSM for
booting, surely?


I'm interested in what happened to generic CSM support. Has Intel
really ditched CSM completely from all other TianoCore builds? Nobody
else will ever be able to build a Tiano-based UEFI firmware again,
unless they also squirrel away a copy of the code before it disappears?
The code really is otherwise being deleted?

Or is this an "open source regression", with code that was in the
public repository now disappearing and only being maintained in
private?



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] Drop CSM support in OvmfPkg?

[David Woodhouse]

On Tue, 2019-01-22 at 16:13 +, Ni, Ray wrote:
> David,
> I'd like to re-start the discussion.
> Could you please kindly explain the background/reason of adding CSM
> support in OVMF?
> Maybe knowing the reason can help to make further decisions of
> whether to
> A. keep it outside OvmfPkg
> B. keep it inside OvmfPkg
> C. maybe have a chance to just remove the CSM support after
> revisiting


The idea was to make it simple to have a single firmware image for
virtual machines which would support both UEFI and Legacy boot for
guests simultaneously.

In libvirt there has been an alternative approach, where the BIOS image
is switched between OVMF and SeaBIOS according to the configuration of
the guest VM.

That's fine for libvirt, but in situations where VM hosting is provided
as a service, it becomes quite painful to manage the 'UEFI' vs.
'Legacy' flags on guest images and then switch firmware images
accordingly. A one-size-fits-all BIOS using OVMF+CSM is very much
preferable.




smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] Drop CSM support in OvmfPkg?

[David Woodhouse]

On Thu, 2018-12-20 at 07:44 +0100, Gerd Hoffmann wrote:
> On Mon, Dec 17, 2018 at 10:54:25AM +0100, Laszlo Ersek wrote:
> > (Adding Kevin, Gerd, David)
> > 
> > On 12/17/18 03:23, Ni, Ruiyu wrote:
> > > Hi OvmfPkg maintainers and reviewers,
> > > I am working on removing IntelFrameworkModulePkg and IntelFrameworkPkg. 
> > > The biggest dependency now I see is the CSM components that OVMF depends 
> > > on.
> > > So I'd like to know your opinion about how to handle this. I see two 
> > > options here:
> > > 
> > >   1.  Drop CSM support in OvmfPkg.
> > >   2.  Create a OvmfPkg/Csm folder to duplicate all CSM components there.
> > > 
> > > What's your opinion about this?
> > 
> > (1) Personally I never use CSM builds of OVMF. The OVMF builds in RHEL
> > and Fedora also don't enable the CSM (mainly because I had found
> > debugging & supporting the CSM *extremely* difficult). For
> > virtualization, we generally recommend "use SeaBIOS directly if you need
> > a traditional BIOS guest".
> 
> On a virtual machine it is very simple to switch the firmware (unlike on
> physical machines), which I think is the main reason ovmf+csm never
> really took off.

Hm, that's true for virtual machines where you own the host system too
and switching BIOS is just a matter of configuration. If you're running
VM hosting at scale, however, and the customers don't get that level of
control, then offering a single BIOS image which does UEFI and CSM in a
"one size fits all" does have some merit.

> > (3) However, David and Kevin had put a *lot* of work into enabling
> > SeaBIOS to function as a CSM in combination with OVMF. Today, the CSM
> > target is a dedicated / separate "build mode" of SeaBIOS.
> 
> IIRC there are still some corner cases which are not working and nobody
> wants put any effort into fixing them.  S3 suspend comes to mind.

Don't think that should be hard to fix if anyone really cares...

> I'm not even sure it still works.  It builds, yes, my jenkins instance
> does that.  But there is no testing beyond that, and I doubt that
> someone else does regular ovmf+csm regression testing.  So the chances
> that any runtime breakage goes unnoticed are pretty high ...
> 
> > (4) I also think an open source CSM implementation should exist, just so
> > people can study it and experiment with it.
> 
> It'll not be deleted from git, so it'll be there even when removed from
> master branch.
> 
> > In short, I think the community would benefit if someone continued to
> > maintain the CSM infrastructure in edk2,

Ruiyu (and Jordan), what's actually happening here? You said you were
deprecating IntelFrameworkPkg... in the internal Intel builds, what
replaces the CSM part? We fought to get parts of CSM support published
to TianoCore from the internal tree, and I'm concerned that this is a
regression — we end up with CSM support only being internal again. Or
is it being dropped from the Intel tree entirely? 

I am very much against *increasing* the number of features which are
supported in private repositories and not the public one.


smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] CryptoPkg: update openssl to ignore RVCT 3079

[David Woodhouse]

On Tue, 2016-07-05 at 17:04 +, Long, Qin wrote:
> Yes, this unset issue was already fixed in OpenSSL HEAD.
> The patch is OK for me to ignore the warning for current 1.0.2
> version. Or we can backport some cleanups into our 1.0.2xx patch. 

My main concern is that we don't accumulate hacks and workarounds. So
add it to OpensslLib.inf by all means, but add a comment indicating the
specific issue (OpenSSL RT# under which it was fixed, and/or the commit
in OpenSSL HEAD Which fixed it), and that it's fixed in 1.1 so can be
dropped then.

Otherwise this patch is just building up technical debt, because
*someone* later needs to clean these things up and make sure they're
not stale. Excessive warning-suppression in this code has *already* led
to subtle bugs which the warnings *would* have told us about.

-- 
dwmw2

smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] CryptoPkg: update openssl to ignore RVCT 3079

[David Woodhouse]

On Tue, 2016-07-05 at 15:53 +, Cohen, Eugene wrote:
> Getting openssl 1.0.2g building with ARM RVCT requires a change to ignore an 
> unset
> variable used before set was necessary.
> 
> corrects x509_vfy.c(875): error C3017: ok may be used before being set
> 
> Change-Id: I0d38193569b29f96861a191908c343831fd957c2
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Eugene Cohen <eug...@hp.com>

Can we "fix" the upstream code instead?

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation

smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] edk2-staging/HTTPS-TLS

[David Woodhouse]

On Thu, 2016-05-19 at 10:30 +0200, Laszlo Ersek wrote:
> 
> master final merge
> *---*-*---**---*--**-->
>  \ \    \ /
>   \ \    \   /
>    *-*-*-[discont.]  *-*-*-[discont.] *-*-*-*
>    feature   rebased feature  rebased feature
> 
> 
> 1.2. Merging. It has the benfit that your development history on the
> feature (staging) branch will be preserved in edk2 master too. 

Do not underestimate this benefit. When you are rebasing a complex
piece of work, and changes in master have affected things which it
interacts with (like OpenSSL being changed and updated), there is a
*distinct* possibility that during one of the rebases, you introduce a
bug. Something that *did* work when you originally wrote your code, now
no longer works.

And because you rebased and the old discontinued branches are lost in
the mists of time (and no longer reachable from the version control
history), the effect is basically that it *never* worked. You're making
it look like you committed code that was broken from day one.

So six months down the line when you realise that some specific corner
case is broken, and you think "oh, I could have *sworn* I tested that",
and you go back with all the tools like git-bisect to find where the
breakage occurred, you're screwed because the history doesn't reflect
reality.

If you rebase, you are not using the version control system as it is
intended to be used. You are wilfully *damaging* the history, and you
do so at your peril.

> On the other hand, you have to be careful about the frequency of
> master-to-staging merges. If you do it very frequently, you'll end up
> with a complex history.

Conversely... do not *overestimate* the relevance of a "complex
history". History is non-linear and contains merged. That is an
accurate representation of what happened, which is what a version
control system exists to report.

You'll mostly neither notice nor care even if you do a gratuitous merge
from master into your topic branch every day. It just annoys the neat-
freaks, that's all. So sure, merge back from master only when you
*need* to. But don't lose sleep over it.

> In the Linux community, merging is preferred, and a master-to-topic
> merge is advised whenever the master branch receives a feature or a
> bugfix that is important for the topic branch as well. That is, no
> spurious merges.

That's the norm for basically everyone who is using the version control
system as it is intended. There are some exceptions, but they're mostly
doing odd things like pretending git is svn (as edk2 currently is, but
hopefully not for *too* much longer).


-- 
dwmw2

smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] Csm16.bin(seabios) failed to work in OVMF.

[David Woodhouse]

On Wed, 2016-05-04 at 13:27 +0200, Laszlo Ersek wrote:
> 
> Hm, I don't think so.
> 
> OVMF includes CpuS3DataDxe with SMM_REQUIRE=TRUE only. My understanding
> is that the reporter encountered the issue with CSM_ENABLE=TRUE,
> SMM_REQUIRE=FALSE.

<digression@>

Of course, now we have SMM, perhaps we should revisit the model where
the SeaBIOS CSM just takes over the hardware completely (and cannot
return to UEFI).

I understand that the "traditional" model is that the CSM does
basically nothing for itself, and all its drivers (like the INT 13h
services) actually just thunk through SMM to the UEFI runtime?

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch 0/3] Use new BDS and UiApp for OvmfPkg

[David Woodhouse]

On Tue, 2016-05-03 at 12:37 +0200, Laszlo Ersek wrote:
> 
> However, CSM is apparently generally borked at the moment (see
> ), and I don't think I can
> spend time on analyzing and fixing that.
> 
> I guess this is not good news, but I thought it better to report than to
> drop the item silently.

Thank you for the explicit prod. Once I've sorted out the new
regressions in building against newly-opaque structures in OpenSSL
HEAD, I'll do a 'git bisect' and see when this broke.

I just hope the history is preserved accurately enough to point the
finger correctly, and that there *is* a point in the (apparent) history
during which it actually worked... :)

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] EDK2 Staging Proposal 3rd draft, final?

[David Woodhouse]

On Fri, 2016-03-18 at 14:24 +0100, Laszlo Ersek wrote:
> Except my own actions. I'm already watching the github issue tracker and
> get emails of the actions of others. No emails about my own actions. It
> makes the email audit trail completely unusable.

That's your personal email notification. If it's set up to mail
everything to the list (even if we create a dummy github user for the
list, and subscribe it to everything), then that won't be an issue,
will it?

Actually, I just went poking about in the settings for repositories. I
see there appears to be a setting to disable merges by merge commit or
by squashing. If you turn those both off on a repository, what happens?
Does that not prevent the merges you were worried about, in either
form?

> I must have missed those discoverability criteria, apologies. In any
> case, I can hardly imagine anything more discoverable than a [PULL] on
> the mailing list.

I think the idea is you should be able to go to e web page and easily
find a list of the stuff that is currently outstanding.

Searching a mailing list for pull requests doesn't really help, because
there's no easy way to see which are merged and which are not.

Patchwork does that kind of thing for some projects, but I don't think
patchwork is the right tool for managing submissions which are intended
to be outstanding for some time, as they undergo refinement and testing
— like the OpenSSL one, for example. There's no way we want that until
the final 1.1 release but we *do* want people to be playing with it.


> I disagree. It is costly. We've had two unintended merges thus far (from
> incorrect command line usage), and they took me personally hours to
> analyze and explain.

I love you, Laszlo, and I *love* the fact that you will take hours to
analyse and explain things. You do it *very* well, and it has been
extremely useful to me and to a lot of other people.

But sometimes perhaps you need to resist the temptation to do it :)

Sometimes, stuff just broke and you just fix it up and move on. Let the
people who *did* it work it out for themselves and that actually forms
a *better* learning experience for them.

And as for committing trivial fixes without review... well, sometimes
it's better to ask for forgiveness than permission. We should never
allow a process to get in the way of getting real work done.

> The fact that these issues were introduced while using the git command
> line supports your point that such problems can occur "anyway". My point
> is that I would like to keep them as rare as possible, and github pull
> requests don't point in that direction.

As noted above, do you want to look in
https://github.com/tianocore/edk2/settings and try turning off the
'Allow merge commits with the merge button' and 'Allow squash merging
with the merge button' options? Or is that only for personal projects?
I don't see it in other github repositories that I have write access
to; only my own.

> If you are volunteering to analyze and undo unintended or incorrectly
> resolved merges from github pull reqs -- I have no further comments then.
> 
> > 
> > And the chances of the CRLF conversion one actually *applying* if
> > someone stupidly hits the button, after *one* more commit hits the
> > tree, are fairly much zero anyway. So you're defending against an
> > impossibility in that case.
> In this specific case, you are right. My point was to prevent building a
> precedent.
> 
> > 
> > I don't know if I should be offended on behalf of the maintainers who
> > are mostly my colleagues, that you have such a low opinion of their
> > capabilities.
> I don't have a low opinion of the capabilities of other edk2
> maintainers. It's a fact that they have just recently started using git.
> It is another fact that I personally mis-click a button (or mistype a
> shortcut) in this or that GUI application occasionally.
> 
> Fixing build breakage (or functional breakage) in the edk2 repository is
> not exactly fun. I find myself analyzing and fixing up bugs from others
> (outside of OvmfPkg/) more and more. Simply because I tend to run into
> them with OVMF (and with Gerd's Jenkins instance) earlier than most
> other people. (I'm not always the first to find out, of course.) I think
> it's natural from me not wanting to ease the introduction of such issues.

Hm, don't we have automatic CI on our pull requests? That's an
oversight... the tools can run builds on various platforms and right
there in the ticket it can show that it doesn't build in certain
configurations. With a big red cross right above the 'merge' button :)

(Which reminds me, I must try to fix things up so that OpenSSL PRs get
tested in something as close to the EDK2 build setup as possible.)

> I'd like to invite other edk2 maintainers to chime in on a github PR
> centered workflow. If the consensus is to use them,

The point here was to have a *look* at them, to make an informed
decision.

>  I'll try to adapt the best I can, although I think it's a 

Re: [edk2] EDK2 Staging Proposal 3rd draft, final?

[David Woodhouse]

On Fri, 2016-03-18 at 17:08 +0100, Laszlo Ersek wrote:
> On 03/18/16 14:59, David Woodhouse wrote:
> > Sometimes, stuff just broke and you just fix it up and move on. Let the
> > people who *did* it work it out for themselves and that actually forms
> > a *better* learning experience for them.
>
> Yes, this is how it should work, but if the fix takes an inconveniently
> long time, the problem will likely impede my own work.

If we can agree on a policy of "if it breaks, revert it", then such
things *shouldn't* take an inconveniently long time.

As long as history is preserved correctly and subsequent work hasn't
been rebased on top of the offending commit(s), of course. If you have
a full history, even if merges have happened, then it's not so hard to
take out the offending commits as long as they're recent.

Of course, CI on submissions helps to avoid the problem altogether. To
a certain extent, at least.
   
> > That's an
> > oversight... the tools can run builds on various platforms and right
> > there in the ticket it can show that it doesn't build in certain
> > configurations. With a big red cross right above the 'merge' button :)
>
> I've never heard of this. Would github operate the CI, or would it be
> some external CI, to be integrated with github?

External, triggered by github.

See https://github.com/openssl/openssl/pulls for example.

Each PR has a green tick or a red cross by it, indicating whether the
automatic builds succeeded. Although it's perhaps a bad example because
OpenSSL has a lot of false failures at the moment. But still you get
the idea. Go into one of the failed, and one of the succeeded, PRs and
you'll see what it looks like in the ticket itself as you view it.

> > > Feel free to reopen the pull requests (if you don't have the
> > > credentials, I can reopen them for you).
>
> You didn't respond to this. Is it okay with you if we delay reopening
> your pulls (#70 and #71) until we can disable the merge button (with
> those two settings you discovered)?

That was the intention implicit in the fact that I waited. I may reopen
the CRLF one as soon as the commit *after* my Windows build fix (qv)
goes into the tree, because that will be *guaranteed* not to merge even
if someone does hit the button :)

> I'll probably feel less threatened about github PRs once the merge
> button is disabled, but I would nonetheless like all patches in a pull
> request to be posted to the list, with the pull request details present
> in the blurb.

Yes, I think that's an important part of any process we come up with
even when it includes github PRs. It doesn't *bypass* the mailing list.

> > Perhaps if we can fix the CRLF thing then applying patches will get a
> > little easier.

> It should, yes.

FWIW this seems to be working. I can do checkouts on Windows and if I
have core.autocrlf set in my git config then I get CRLF line endings in
all the checked out files just as before. And if I *don't* have that
set, then I get LF but it works fine anywy.,

With core.autocrlf set, just pulling the commit which changes the
*internal* representation from CRLF to LF basically does nothing to the
checked-out files since they remain CRLF.

I haven't worked out how to *automatically* set the core.autocrlf for
the repository, similar to the way that .gitattributes works. But given
that things work OK even without it, I think that's fine.

I want to redo all my testing after that VS2008 build fix has landed,
so I'm not perturbing all my tests. And obviously let some other people
test it. But I'm fairly happy with it.

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH v2] OvmfPkg/PlatformPei: Fix VS2008 build breakage

[David Woodhouse]

Commit 7b8fe6356 ("OvmfPkg: PlatformPei: enable PCIEXBAR (aka MMCONFIG /
ECAM) on Q35") broke the VS2008 build, causing it to complain of
"potentially uninitialized local variable 'PciExBarBase' used" at
line 275.

On this occasion it isn't actually wrong — the mHostBridgeDevId variable
is global, so theoretically it *could* change between the two if()
statements.

Of course VS2008 is also a steaming pile of poo, and even if we use
a local boolean variable which definitely *can't* change in the middle,
it still can't work it out and still emits the warning.

So just initialise PciExBarBase to zero to shut the compiler up.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: David Woodhouse <david.woodho...@intel.com>
---
Oops, my build test on Windows still had the temporary 'PciExBarBase=0'
to shut the compiler up, so wasn't really exercising the fix I was
submitting. Sorry for the noise.

It turns out that VS2008 really is just a stupid pile of crap.

OvmfPkg/PlatformPei/Platform.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
index 0fc2278..20008d0 100644
--- a/OvmfPkg/PlatformPei/Platform.c
+++ b/OvmfPkg/PlatformPei/Platform.c
@@ -212,7 +212,7 @@ MemMapInitialization (
 
   if (!mXen) {
 UINT32  TopOfLowRam;
-UINT64  PciExBarBase;
+UINT64  PciExBarBase = 0;
 UINT32  PciBase;
 UINT32  PciSize;
 
-- 
2.5.5

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] Problem with Author, Sign Off, and IP concerns

[David Woodhouse]

On Wed, 2016-03-16 at 12:15 +0100, Laszlo Ersek wrote:
> My only request is that the "Author:" git metadata header be matched by
> the first Signed-off-by tag.

We use the Signed-off-by tag the same way as everyone else does, don't
we? With reference to the "Developer's Certificate of Origin" as seen
at http://developercertificate.org/? See item (b) therein:

(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications,


This covers the case of a Signed-off-by: which doesn't come from the
author, doesn't it? 

The 'Signed-off-by' is something that only that individual can create —
it should never be 'assumed' and constructed by someone else.

So if someone has written some code and committed it to an internal
repository (without a signed-off-by), and has then left the company, it
seems perfectly reasonable for them to still be listed as the *author*
of the work when it's pushed upstream, but for their Signed-off-by to
be absent. And for someone *else* in the company to provide a
Signed-off-by: tag indicating that they have the right to submit it.


-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2] OvmfPkg/PlatformPei: Fix VS2008 build breakage

[David Woodhouse]

On Fri, 2016-03-18 at 17:53 +0100, Laszlo Ersek wrote:
> 
> (1) The commit message uses at least one non-ASCII character, the EM
> DASH (U+2014). Can you please replace it with a "--"? Yes, I know you
> hate me for asking this. Please just write me off as stupid and replace
> the character.

No. This isn't 1994. If you wish to change it, go ahead. Don't ask me
to do stupid things for no good reason.

> (2) In the edk2 coding style, initialization of local variables is not
> permitted. Can you please add an assignment instead?

Er, really? What insanity is this? Where did these guidelines come from
and can they be fixed? Why would initialisation of local variables
*ever* be frowned upon? How do they make this stuff up?

> (3) I tried to apply your patch nonetheless. It doesn't apply. I looked
> at the patch email that I saved from Thunderbird. The code section of
> the email contains =C2=A0 quoted-printable sequences. Since the charset
> is UTF-8 (according to the Content-Type header), 0xC2 0xA0 translates to
> U+00A0, "NO-BREAK SPACE".

Hm, that would appear to be an Evolution bug. I think it triggers when
lines have trailing whitespace. I'll investigate. Thanks for pointing
it out.

I can put it in a PR if you prefer... :)

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] EDK2 Staging Proposal 3rd draft, final?

[David Woodhouse]

I could have sworn I'd responded to this last night, but it was late,
and I see no evidence of such in my outbox or on the list. Apologies if
I'm repeating myself...

On Thu, 2016-03-17 at 00:15 +, Kinney, Michael D wrote:
> Jordan asked a similar question about adding a 'staging' directory or 
> 'staging'
> branches to the edk2 repo.  If you look at the edk2 repo today, it has master 
> and the supported UDK* branches.  In the transition from SVN -> GIT, the old 
> tags
> were removed and archived.
> 
> The reason for suggesting a new edk2-staging repo is to keep content that
> is not ready for products to be clearly separated.  We do not want anyone
> to pick up any of the staging content and put it into shipping FW images.
> 
> There was also a minor concern about the size of the edk2 repo, but it
> was shown that this is not really an issue at this time.
> 
> So the choice here is a set of staging feature branches in the current 
> edk2 repo with clear communication that none of them are product ready.  
> Or a new edk2-staging repo with feature branches (current proposal).

This seems to me to be a false dichotomy. You've left out the third
option, which is just to use the tools as they were designed to be
used.

Let people set up their submissions in github repositories of their own
(which works for groups as well as individuals, and allows loosely-
formed collaborative groups as well as more formal projects). And you
can already track their submissions at
https://github.com/tianocore/edk2/pulls

> I think a process to create/merge/remove feature branches in the 
> tianocore.org repos is required because we really do not want too 
> many branches.  If the feature being developed is smaller in 
> scope, developer owned github branches or email PRs should be sufficient.

Developer-owned github repositories are suitable for larger submissions
too, surely? (With associated discussion on the mailing list which was
always part of the plan, of course. I'm not suggesting that it should
necessarily happen *only* in the github PR format.)

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] EDK2 Staging Repository 2nd Draft

[David Woodhouse]

> On 03/15/16 18:20, Kinney, Michael D wrote:
> I don't object to pulling, if the submitter explicitly requests it, and
> if we're making this option official now.

Let's say "at the discretion of the person doing the merge" rather than
writing down that it should need an explicit request.

Over time, as people get used to using the tools properly, I would expect
that pulling will become the default behaviour. By choosing to rebase
instead, *you* are choosing to discard the correct history of the original
development vs. the merge, and to take the responsibility for any
resulting problems.

> Regarding Reviewed-by tags, I like to review series per-patch, and I
> also like the tags to be present in the commit history per-patch. For
> example, when a series modifies several top-level packages, I don't /
> can't always review all patches in full depth; so I might give an
> Acked-by for some patches, or even nothing.
>
> In this case I'm fine with the submitter rebasing one last time, adding
> the tags, and then I can do the pull. (I'll have to remember to verify
> the individual patches on the branch being pulled, and the labels on the
> commit messages, against the mailing list traffic.)

Of course you had to do that verification anyway, as the patch in your
inbox might not actually match the "same" message on the list. So no
change there.

It's also worth noting that we use "rebase" for two different things. In
normal usage it's considered bad form just to recommit the same patch on
the *same* parent. We will probably continue to do that freely here, and
as long as you don't move it to a new parent, that's OK. But people
generally still call that "rebasing".


-- 
dwmw2

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2] OvmfPkg/PlatformPei: Fix VS2008 build breakage

[David Woodhouse]

On Fri, 2016-03-18 at 20:02 +0100, Laszlo Ersek wrote:
> On 03/18/16 19:40, David Woodhouse wrote:
> 
> > 
> > But this is different. This is the commit messages. And what would you
> > know... the last commit message in the log which isn't ASCII *isn't*
> > that other one I pointed out; it's one from you (7daf2401) in which you
> > commit the heinous crime of slepping Michał Zegan's name correctly :)
> I went to extreme lengths to commit his name correctly. It wasn't at all
> natural for me. But names are names.
> 
> > 
> > No, I genuinely don't know of any reason to eschew non-ASCII characters
> > in commit messages.

> Because they are a PITA for people who don't use UTF-8 generally? (I
> don't.)

This doesn't really make sense at any level. You must have to jump
through hoops to still operate a legacy 20th-century character set on a
modern Linux distribution, surely? And it must confer *no* benefit to
you — unless you count it as a benefit that you have to go to 'extreme
lengths' just to get someone's *name* right — something that normally
ought to Just Work™ when you use 'git am'.

And once it's been committed, either your system just doesn't display
the commit logs correctly at all (and gets names like Michał's wrong),
or you shouldn't even *notice* the emdash. Which is it?

I strongly suspect the latter, and that you only noticed because you
were looking closely at the encoding because of that Evolution bug?

> People still write RFC's today, and I think those are all pure ASCII 
> as well.

∃ lots of things which can fit in pure ASCII.
∃ lots of things which don't.

> Anyway, we won't convince each other; we've been through this. Feel free
> to post non-ASCII in the commit messages; should I come across them,
> I'll try to fix them up (except in names).

No. Please don't. If this was another of the bizarre but enforced-by-
the-project things then I was prepared to tolerate it until it could be
fixed, but if you are just mangling my commit messages to make them
less grammatically correct for an unsupportable personal preference,
then that's not OK.

The correct character to use in that situation is the emdash, If you
*absolutely* must, then rewrite the whole sentence to avoid using it.
Do *not* replace it with hyphens. And when you do tweak a commit
message, it is best practice to *note* that you have done so, and why.

Rewriting it for this reason is *not* acceptable.

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2] OvmfPkg/PlatformPei: Fix VS2008 build breakage

[David Woodhouse]

On Fri, 2016-03-18 at 19:05 +0100, Laszlo Ersek wrote:
> On 03/18/16 18:45, David Woodhouse wrote:
> > 
> > On Fri, 2016-03-18 at 17:53 +0100, Laszlo Ersek wrote:
> > > 
> > > 
> > > (1) The commit message uses at least one non-ASCII character, the EM
> > > DASH (U+2014). Can you please replace it with a "--"? Yes, I know you
> > > hate me for asking this. Please just write me off as stupid and replace
> > > the character.
> > No. This isn't 1994.
> Really? I wouldn't know. If I look at what ISO C features we're allowed
> to use, we certainly seem to be stuck in 1995. Or, well, sometime before
> 1989, because we can't even use structure assignment.

That's a separate issue. Yes, we still insist on supporting the
Microsoft toolchains despite the fact that they are so badly maintained
that they don't even support the last C standard of the *previous*
century, let alone anything from the 2000s.

But this is different. This is the commit messages. And what would you
know... the last commit message in the log which isn't ASCII *isn't*
that other one I pointed out; it's one from you (7daf2401) in which you
commit the heinous crime of slepping Michał Zegan's name correctly :)

> There is a good reason. You know it and you don't care about it. Not a
> problem; I'll fix up the commit message.

No, I genuinely don't know of any reason to eschew non-ASCII characters
in commit messages.

> > Er, really? What insanity is this?
>
> Please ask your colleagues at Intel (but please also make sure that you
> don't take offense on their behalf, when you call their guidelines
> insane ;) ).

Hehehe. Touché.

> > 
> > Where did these guidelines come from
> > and can they be fixed? Why would initialisation of local variables
> > *ever* be frowned upon? How do they make this stuff up?
> Please consult "EDK II C Coding Standards Specification.pdf", section
> 6.8 "C Function Layout":
> 
>   *Initializing a variable as part of its declaration is illegal.*
> 
> (Emphasis theirs.)

Insanity theirs :)

Why in $DEITY's name would anyone write this? Was it done for a bet?

> \o/
> 
> Finally something I'm not held responsible for!
> 
> (BTW, why did you mail out the patch with Evolution, rather than
> git-send-email? Are you sure you are using the tools as they were
> intended? :))

:)

I like to have copies in my sent-mail folder of everything I sent. It
also gets S/MIME signatures, and it's easier to hit 'reply' and have a
properly-threaded message, rather than having to extract the Message-Id 
of the message I want to reply to and feed it to git-format-patch.

On the whole (modulo evo bugs) it's easier just to insert the plain
text file into the email I'm sending, for a single patch at least.

And I seem to have got away without triggering that Evo bug for a long
time, by working on code which cares more about messy trailing
whitespace on lines, than it does about other things :)

-- 
dwmw2


smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] EDK2 Staging Proposal 3rd draft, final?

[David Woodhouse]

On Wed, 2016-03-16 at 20:17 +, Mangefeste, Tony wrote:
> 
> Here's the 3rd iteration based on your input.  Please take one last
> opportunity to review, we'll settle on this if there are no major
> requests by Friday of this week.

I still haven't seen an answer to my question of what this buys us,
over the normal process of having such submissions come via
contributors' github repositories, with associated pull requests.

Why invent new processes and not just use the existing tools that are
basically *designed* for this workflow?

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 3/6] OvmfPkg: PlatformPei: enable PCIEXBAR (aka MMCONFIG / ECAM) on Q35

[David Woodhouse]

On Tue, 2016-03-08 at 15:20 +0100, Laszlo Ersek wrote:
> +UINT64  PciExBarBase;
...
>  if (mHostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
...
> +  PciExBarBase = FixedPcdGet64 (PcdPciExpressBaseAddress);
...
>  }
...
>  if (mHostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
...
> +  AddReservedMemoryBaseSizeHob (PciExBarBase, SIZE_256MB, FALSE);

The mHostBridgeDevId variable is global. Therefore as far as the
compiler knows, it can change between those two if() statements.

VS2008 complains thus:

e:\edk2\ovmfpkg\platformpei\platform.c(275) : warning C4701: potentially 
uninitialized local variable 'PciExBarBase' used


If only we had some kind of code submission process where your
submission could be automatically subjected to build tests on all
platforms, and the status of those builds could be automatically
displayed right there with your request... :)
 
-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] EDK2 Staging Proposal 3rd draft, final?

[David Woodhouse]

On Fri, 2016-03-18 at 13:18 +0100, Laszlo Ersek wrote:
> 
> Thanks for the work. I'm willing to help you test these.
> 
> I'm going to close the github pull requests now.

Please don't. The point is to look at the workflow that the github PR
tickets allow, not to actually merge the code — which is why they both
explicitly state that they aren't to be merged.

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] EDK2 Staging Proposal 3rd draft, final?

[David Woodhouse]

On Thu, 2016-03-17 at 21:28 +, Kinney, Michael D wrote:
> Yes.  Use of developer github forks is supported.  I had summarized
> 3 development methods earlier in this thread.
> 
> 1) PR emails send to edk2-devel.  There is a Wiki page that details process
> for developers and maintainer.  
> 
> 2) Branch on developer owned github fork of edk2.  There have been 
> discussions on
> how to support a pull request.  This is still under investigation.  

I'm glad to see this. Note that the 'backporting' into Subversion (as
Jordan mentioned) shouldn't be an issue; you can always *choose* a
linear path through the DAG of commits, so a merge such as

  A → B → C → D
   ↘           ↘
    B' →  C' →  E

...can *either* be represented as A → B → C → D → E in Subversion, or
perhaps as A → B' → C' → E depending on which side was actually *seen*
in the EDK2 repository in the interim.

But it certainly shouldn't be a blocker for allowing merge commits, and
I can't think of anything *else* that would be.

> 3) Branch on edk2-staging (Specific process being discussed in this thread)
> 
> In order to properly capture your feedback, I think a higher level overview
> Wiki page on the development methods available needs to be written up with
> edk2-staging just being one of the 3 methods (and likely most rare).
> 
> I will work with Tony to get Wiki pages updated with all this information.
> 
> Will this address your concerns?

It still hasn't really answered the question of why we need a whole new
tree, process and associated documentation for this third and 'most
rare' method of merging contributions, when the two existing standard
methods should happily cover all use cases.

It just seems entirely gratuitous.

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH] OvmfPkg/PlatformPei: Fix VS2008 build breakage

[David Woodhouse]

Commit 7b8fe6356 ("OvmfPkg: PlatformPei: enable PCIEXBAR (aka MMCONFIG /
ECAM) on Q35") broke the VS2008 build, causing it to complain of
"potentially uninitialized local variable 'PciExBarBase' used" at
line 275.

The problem is that the mHostBridgeDevId variable is global. Therefore
as far as the compiler knows, its value can change between the two if()
statements where it is first set, and then subsequently used.

By using a local boolean variable which definitely *cannot* change, we
avoid this problem.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: David Woodhouse <david.woodho...@intel.com>
---
 OvmfPkg/PlatformPei/Platform.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
index 0fc2278..60d8a41 100644
--- a/OvmfPkg/PlatformPei/Platform.c
+++ b/OvmfPkg/PlatformPei/Platform.c
@@ -211,13 +211,14 @@ MemMapInitialization (
   AddIoMemoryRangeHob (0x0A, BASE_1MB);
 
   if (!mXen) {
+BOOLEAN isQ35 = (mHostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID);
 UINT32  TopOfLowRam;
 UINT64  PciExBarBase;
 UINT32  PciBase;
 UINT32  PciSize;
 
 TopOfLowRam = GetSystemMemorySizeBelow4gb ();
-if (mHostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
+if (isQ35) {
   //
   // The MMCONFIG area is expected to fall between the top of low RAM and
   // the base of the 32-bit PCI host aperture.
@@ -249,7 +250,7 @@ MemMapInitialization (
 PcdSet64 (PcdPciMmio32Size, PciSize);
 AddIoMemoryBaseSizeHob (0xFEC0, SIZE_4KB);
 AddIoMemoryBaseSizeHob (0xFED0, SIZE_1KB);
-if (mHostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
+if (isQ35) {
   AddIoMemoryBaseSizeHob (ICH9_ROOT_COMPLEX_BASE, SIZE_16KB);
   //
   // Note: there should be an
-- 
2.5.5

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2] OvmfPkg/PlatformPei: Fix VS2008 build breakage

[David Woodhouse]

On Sat, 2016-03-19 at 02:23 +0100, Laszlo Ersek wrote:
> On 03/19/16 02:15, David Woodhouse wrote:
> 
> > So we treat it as an opaque sequence of bytes on the way *in*, then
> > make assumptions on the way *out* about what it was?
> 
> On the way in, it is assumed to be UTF-8, unless the user says
> otherwise. If the user says otherwise (in i18n.commitencoding), that
> statement is captured in the commit object. Either way, the commit
> message is not converted.

Right. That is exactly the problem.

In the legacy world of mixed character sets, *every* string needs to be
explicitly labelled with its character set, and everything handling
strings needs to *either* convert its input to its desired output
charset, or at the very least pass that label along intact.

To *assume* that your input matches your output format (as git-commit
does), and to wilfully ignore the explicit labelling on it (LC_CTYPE,
as you mention), is wrong. You are deliberately *dropping* the label on
the input bytestream, and slapping an incorrect label on it instead.

But it happens. All the time. Even in software written by people who
ought to know better.

Because consistently labelling and converting character sets is hard. 

In fact, there's some room for debate about whether LC_CTYPE *is* the
correct label for the git-commit input. It's possible that there *is*
no right answer here, and no way for git to avoid being buggy.

If you're providing a message with -m on the comment line, then sure,
LC_CTYPE is correct. But when it comes from a file? Every file can come
from a different place, and can be in its *own* character set. I bet
you have *plenty* of text files on your system which aren't in latin2.
And if you're applying a patch with 'git-am' then the temporary file
storing the extracted message is almost *certainly* in UTF-8 or the
original charset from the email, not your LC_CTYPE. Right?

In the legacy charset world we'd need a way to label every text file
with its character set — LC_CTYPE is only an approximation.

But hey, thank $DEITY we solved that in the 21st century by just
standardising on UTF-8 and letting the labelling issue be a thing of
the distant past. :)

Few people fully comprehend the true horror of the "simple" system they
try to cling to...

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2] OvmfPkg/PlatformPei: Fix VS2008 build breakage

[David Woodhouse]

On Fri, 2016-03-18 at 17:53 +0100, Laszlo Ersek wrote:
> 
> (1) The commit message uses at least one non-ASCII character, the EM
> DASH (U+2014). Can you please replace it with a "--"? Yes, I know you
> hate me for asking this. Please just write me off as stupid and
> replace the character.

There's another one in commit 5121a7646 btw :)

Or should I say ☺

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] EDK2 Staging Proposal 3rd draft, final?

[David Woodhouse]

On Fri, 2016-03-18 at 13:30 +0100, Laszlo Ersek wrote:
> 
> Our workflow should not be centered on github pull requests in any case,
> so I don't see the point in testing them out. 

Well, thanks for destroying the test I spent this morning setting up,
because you don't believe it would have useful results.

I happen to disagree about using github pull requests. Every change
there triggers an email, and we can set things up so that email is also
copied to the mailing list. So it's just as well-archived as the other
list traffic, and we don't have to worry about github.com disappearing
in a puff of smoke any more than we already did.

> workflow centered on pull requests that are sent to the mailing list
> should be tested out, and I'd like to work with you on that.

That works for me, but doesn't seem to cover the criteria that Michael
set out for discoverability.

> I understand that the PRs you created on github explicitly stated they
> weren't to be merged. It doesn't matter. One misclick from an edk2
> maintainer who happens to be paying a bit less attention, and poof, we
> have mess in the git history.

Which is fairly much true of *any* work, right? Any of those people
could 'accidentally' push stuff from their working tree out to the main
repository, at any time. We either revert it with a new commit, or we
quickly undo the commit and rewind the contents of the tree before too
many people update from it. It's hardly the end of the world.

And the chances of the CRLF conversion one actually *applying* if
someone stupidly hits the button, after *one* more commit hits the
tree, are fairly much zero anyway. So you're defending against an
impossibility in that case.

I don't know if I should be offended on behalf of the maintainers who
are mostly my colleagues, that you have such a low opinion of their
capabilities.

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2] OvmfPkg/PlatformPei: Fix VS2008 build breakage

[David Woodhouse]

On Fri, 2016-03-18 at 22:53 +0100, Laszlo Ersek wrote:
> It happens to display Michał's name correctly, because it fits in latin2.

Ah, OK. You got lucky on that one. Lots of names *don't* fit in
ISO8859-2.

> The extreme lengths that I had to go to were necessary to convince
> git-send-email not to mess up Michał's CC in the email headers, picking
> it up from the commit message. The commit message was all right, but the
> email header got mis-encoded (it's a git bug; it thought the CC line was
> UTF-8, despite knowing that the full commit message was latin2).

I'd be interested in a reference to the upstream bug report for that
one.

It's going to be caused by the use of 'i18n.commitencoding=latin2'.

Hm... did you realise that when you use that setup and you commit and
push directly, that means you are putting latin2-encoded objects into
the upstream EDK2 git repository?

I'm surprised that even works without corrupting things for everyone —
it looks like your commits actually carry an explicit label marking
them as latin2.

However, I really do think that's best avoided.

The lesson we learned in the 20th century was that labelling character
sets just doesn't work very well. The labels fall off, assumptions get
made and things interpreted in the "default" character set. So your
non-standard latin2 commits *are* at some point going to be wrongly
interpreted as UTF-8. In fact, wait a minute... you just *gave* me an
example of that happening :)

The labelling problem hasn't gone away entirely in the 21st century —
everyone using a modern setup still ought to be explicitly labelling
their output as UTF-8, and correctly converting from legacy on the way
in.

But in a system where everything is UTF-8 all of the time, mislabelling
errors are completely harmless. Errors like the one above — while they
might happen — don't actually cause a problem. Something was UTF-8 and
you mistakenly interpreted it as UTF-8 because you misplaced its label?
Oh well, nobody noticed :)

So yes, I'm interested in the bug because it should be fixed. But
basically, you brought it upon yourself by operating in a mode that is
*known* to invite such errors, and was abandoned by most other people a
*long* time ago. And you're pushing that choice into the EDK2 history
so that others are exposed to the same class of bugs. :(

> emdash doesn't display correctly for me indeed, in the output of "git
> log". It's not a big annoyance, but if I am to apply a patch, I try to
> prevent it.
> 
> > 
> > I strongly suspect the latter, and that you only noticed because you
> > were looking closely at the encoding because of that Evolution bug?
> No. I tend to notice glyphs by the naked eye that are not ascii / latin1
> / latin2. Here's another example:
> 
> http://thread.gmane.org/gmane.comp.bios.edk2.devel/8563/focus=8566

I'll see that one and raise you a 'There is nothing you can put in the
man page source to make it output the correct dash on all devices' :)

https://bugzilla.redhat.com/show_bug.cgi?id=1173619


> Okay. I've googled the use of emdash in the English language, and it
> seems to be more or less interchangeable with parens. Is that okay?

In some usages, yes — but probably not this example.

Two dashes is often considered an acceptable rendering of the emdash,
if you *really* must... but in this case since it isn't actually
necessary, I'd very much prefer that you leave it as it is.

-- 
dwmw2

smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2] OvmfPkg/PlatformPei: Fix VS2008 build breakage

[David Woodhouse]

On Sat, 2016-03-19 at 01:55 +0100, Laszlo Ersek wrote:
> 
> Okay, here's what I'll do. I will switch i18n.commitencoding back to
> UTF-8. And, I will add a commit-msg hook that converts the commit
> message in-place from latin2 to UTF-8, with "iconv". That should keep
> us both happy. Deal?

That sounds like a good solution; thanks.

I think it's arguably a git bug that it's necessary. Git ought to
honour the locale settings when dealing with user input. If it can
convert on *reading* from repository files, why wouldn't it convert on
*writing* them?

The 'git commit' man page says both that

       ·   The commit log messages are uninterpreted sequences of non-NUL
   bytes.

and also that

        2. git log, git show, git blame and friends look at the encoding
   header of a commit object, and try to re-code the log message into
   UTF-8 unless otherwise specified. You can specify the desired
   output encoding with i18n.logoutputencoding ...

... which seems self-contradictory. It goes on to say:

       Note that we deliberately chose not to re-code the commit log message
   when a commit is made to force UTF-8 at the commit object level,
   because re-coding to UTF-8 is not necessarily a reversible operation.

So we treat it as an opaque sequence of bytes on the way *in*, then
make assumptions on the way *out* about what it was?

Which is just one of the set of classic "oops, I dropped the label"
bugs which rendered the legacy charsets unworkable, but this time it
almost seems to be *deliberate*.

The logic behind not re-coding is silly. Because throwing away the
charset label on the input text and assuming it's already in
i18n.commitencoding definitely *isn't* a reversible operation :)

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] OvmfPkg/PlatformPei: suppress wrong VS2008 warning (use of uninited local)

[David Woodhouse]

On Sat, 2016-03-19 at 00:35 +0100, Laszlo Ersek wrote:
> VS2008 seems to think that the "PciExBarBase" variable (introduced in
> commit 7b8fe63561b4) can be evaluated for the
> AddReservedMemoryBaseSizeHob() function call with its value being
> uninitialized / indeterminate. This is not the case (see
> "mHostBridgeDevId"); suppress the warning.
> 
> Reported-by: David Woodhouse <dw...@infradead.org>
> Ref: http://thread.gmane.org/gmane.comp.bios.edk2.devel/8871/focus=94
> 31
> Cc: David Woodhouse <dw...@infradead.org>
> Cc: Jordan Justen <jordan.l.jus...@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <ler...@redhat.com>

Works here with VS2008 (perhaps unsurprisingly, but hey — my first
attempt didn't ☺). Thanks.

Reviewed-by: David Woodhouse <david.woodho...@intel.com>

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2] OvmfPkg/PlatformPei: Fix VS2008 build breakage

[David Woodhouse]

On Fri, 2016-03-18 at 23:26 +0100, Laszlo Ersek wrote:
> 
> Whenever you contribute to a project, do you always start with making a
> huge noise, calling everyone around (or their rules) insane, "makes no
> sense at all", and so on?

Not at all. Some weeks I'll work on as many as a dozen or so projects.
This week it's been EDK2, OpenSSL, Linux, MIT krb5, GSS-NTLMSSP, git,
NSS, and two or three GNOME projects. And possibly others; I distinctly
remember there being some python but I tend to blot out those memories
as quickly as I can.

Most projects are relatively easy to work with. If they have coding
styles that they adhere to, then they're obvious in the code you're
working on — and in the ideal case, encoded in the source files in a
form which emacs can automatically understand.

I can cope with 'this project suffers from supporting Microsoft tools
and thus we can't even use C99' — I've seen that enough times.

But what we have here is a particularly special combination — the
insistence on supporting crappy Microsoft tools with false warnings
about uninitialised variables, in conjunction with a rule that "we
don't initialise variables on declaration". That *combination* just
blows the mind, surely?

I have *never* dealt with projects that seem to be trying *so* hard to
do the wrong thing and make life hard, in so many ways. To the point
where one has to go and look up the special project-specific
instructions just to do something as trivial as applying a patch from
the mailing list.

And sure, you do that every day so you don't find it problematic. You
even *wrote* those instructions. But that's doesn't mean it's OK.
That's just Stockholm syndrome.

What's really frustrating is that that was a known issue, and it's
something that *could* have been handled before the final switch to
git. I had even worked on doing the Subversion → git conversion such
that the CRLF thing was fixed in all commits as they came across.

But it didn't get done, and now we still have the same issues for no
good reason — and a longer period of transition(s) before we finally
get to where we need to be.

The same goes for the half-measure of using git as if it was
subversion. Again it means *another* change before we get there.

So yes. I'm sorry. I get frustrated.

> CRLF has been working out for most people. Rebasing patches has been
> working out. Staying away from non-ASCII in commit messages ditto.
> Keeping full context when quoting patches ditto. You want edk2 to
> change its ways in all aspects at once. And since I seem to be the
> guy who talks to you the most, I'm the one you yell at the most. I've
> never experienced this before. I'm stunned.

CRLF works for *you* because you're doing it all the time, and can
remember all the magic you have to do to work around it. Throwing away
history works until you actually find you need to refer back to it. You
*aren't* staying away from non-ASCII in commit messages; your
configuration has led to pushing esoteric legacy charsets into the EDK2
git objects themselves, inviting all the labelling problems that we
used to have in the 20th century before we all started using UTF-8. And
you've shown an example of that happening already. The patch citation
you're picking on was clearly showing the i'f(x) {set} … if(x) {use}'
pattern which was triggering the warning, in a way that wouldn't have
been readable (I might as well have elided it completely) if I'd left
the full thing. Although you claim it made it hard to find the code in
question, the bit that said 'line 275' ought to have given it away.

And sure, I'm a whole lot better at dealing with computers than I am
with people, and I tend not to be very good at hiding my frustration.
I'm sorry if you interpret that as 'yelling', especially 'at you'. I
respect you greatly, and I keep telling you how much I appreciate the
help that you offer to me and to others.

It's not that I "want edk2 to change its way in all aspects at once". 

All I wanted to do was due diligence on the OpenSSL 1.1 support that
I've been working on.

I wanted to pull in the HTTPS patches and actually get that working,
but it's just too painful. Sure, I probably *could* have done it, and
thanks again for giving me yet another magic incantation to work around
yet another aspect of the brokenness I had tried to avoid by pointing
it out before we set it in stone. But the moment has passed — the
additional time it would require has pushed it to "Someone Else's
Problem" status. I've thrown the 'hey, we can build the ssl/ directory
too' patch into the PR I submitted this morning, and that'll suffice.

But I at *least* wanted to test what I was doing on Windows. So I went
to the trouble of firing up a Windows VM and dealing with it (which
probably hasn't helped my frustration level right from the start, to be
honest), and found that the build had apparently been broken for 2
weeks without anyone noticing. Which makes me wonder why I'm
bothering... but OK, I'll not only 

Re: [edk2] EDK2 Staging Proposal 3rd draft, final?

[David Woodhouse]

On Thu, 2016-03-17 at 21:28 +, Kinney, Michael D wrote:
> 
> Yes.  Use of developer github forks is supported.  I had summarized
> 3 development methods earlier in this thread.
> 
> 1) PR emails send to edk2-devel.  There is a Wiki page that details process
> for developers and maintainer.  
> 
> 2) Branch on developer owned github fork of edk2.  There have been 
> discussions on
> how to support a pull request.  This is still under investigation.  In the 
> meantime, a link to the branch and pull request can be included in
> the PR emails to edk2-devel.
> 
> 3) Branch on edk2-staging (Specific process being discussed in this thread)

Let's *test* my assertion that github PRs should be sufficient even to
cover #3.

They're discoverable — at https://github.com/tianocore/edk2/pulls you
can see that I've just filed a couple of tickets. I believe they
satisfies all the other criteria you've mentioned, but let's have an
experiment and find out.

I think these two test cases are precisely the kind of submission which
want to be handled via 'staging', aren't they?

The first — https://github.com/tianocore/edk2/pull/70 — is the update
to build against OpenSSL 1.1. I've thrown in a "line note" on the code,
so you can see how that works as well as the general discussion in the
ticket.

The second — https://github.com/tianocore/edk2/pull/71 — is a test of
converting the internal representation of all files to use git's native
LF line endings. This should mean that the tools can do the right thing
and check out the working files according to the norms of the platform
— you'll get CRLF on Windows, and LF on Linux. It would be good to test
that, iron out any problems and get it fixed. But again, definitely
something that wants staging for review and testing first.

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2] OvmfPkg/PlatformPei: Fix VS2008 build breakage

[David Woodhouse]

On Sat, 2016-03-19 at 01:03 +0100, Laszlo Ersek wrote:
> > So yes, I'm interested in the bug because it should be fixed. But
> > basically, you brought it upon yourself by operating in a mode that is
> > *known* to invite such errors, and was abandoned by most other people a
> > *long* time ago.
> 
> (Hm, didn't I just predict exactly this argument above?...)

Yes, and for good reason.

> > And you're pushing that choice into the EDK2 history
> > so that others are exposed to the same class of bugs. :(
> 
> The facility I'm using is a documented feature of git; git will
> transcode Michał's name to UTF-8 for others (or whatever they have in
> logOutputEncoding).

Except when it predictably breaks, as you demonstrated.

As I said, we knew *years* ago that charset labelling doesn't work
reliably, and the only good answer is to be consistent so that it
doesn't matter (which basically means UTF-8. Don't even get me started
on UTF-16.)

And you've intentionally pushed an inconsistent set of character sets
to the EDK2 repository, you've *seen* the git tools fail to deal with
those labels properly, you've *predicted* that you'd be told "that's
what you get for mixing charsets"... and yet you're still absolutely
happy that you're pushing this to the EDK2 repository for everyone to
enjoy the potential bugs?

I'm still not yelling. But I'm not smiling either.

I really do think that if we can have a policy which eschews merge
commits, we should *definitely* have a policy which eschews non-UTF8
objects in the tree.

Isn't that i18n.commitencoding=latin2 config on your part somewhat
gratuitous? The idea is that the tools are converting everything on the
way in and out, and that *only* affects the commit objects that are
actually encoded in the repository, right? So letting that be UTF-8 to
match the default shouldn't even be *visible* to you, should it? Except
that you probably wouldn't have suffered that git-send-email bug you
mentioned?

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] EDK2 Staging Repository 2nd Draft

[David Woodhouse]

On Tue, 2016-03-15 at 00:16 +, Kinney, Michael D wrote:
> 
> 
> Can you provide some revised text you would like to see in step 6.
> 
> I agree that we need to use the tools in ways that help make this easy, 
> prevent
> errors, and preserve history.  Given that step 6 describes promoting a 
> feature 
> from edk2-staging repo to edk2 repo, what set of git operations you would 
> recommend?

Literally, 'git pull'. So my change in patch form would be:

@@
 6) Process to promote an edk2-staging branch to edk2 trunk
 a) Request sent to edk2-devel that describes the feature, design, 
testing, etc.
 b) Stewards evaluate request and determine if the feature meets edk2 
criteria.
-c) If approved, use edk2 patch review/commit process on edk2-devel 
mailing list
+    c) If approved, use 'git pull' to merge the branch into edk2 master, 
adding
+   any final 'Reviewed-by:' tags to the merge commit as appropriate.
 d) Remove feature branch from edk2-staging (maybe archived 
elsewhere?). 


I'm still interested in what benefit we gain from a centralised
edk2-staging repository though, over the normal process of contributors
just pushing their work to github repositories and submitting PRs.

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] EDK2 Staging Repository 2nd Draft

[David Woodhouse]

On Mon, 2016-03-14 at 22:38 +, Mangefeste, Tony wrote:
> Yes there is work that is substantial enough.

I'm not sure what that's an answer to; you didn't leave a specific part
of my email above it as context.

I was saying that *when* work is substantial enough to warrant such
staging, that is *precisely* the kind of work where we want to be using
the tools properly and *pull* it in rather than rebasing it.

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] EDK2 Staging Repository 2nd Draft

[David Woodhouse]

On Sat, 2016-03-12 at 00:25 +, Mangefeste, Tony wrote:
> 
> 6) Process to promote an edk2-staging branch to edk2 trunk
> a) Request sent to edk2-devel that describes the feature, design, 
> testing, etc.
> b) Stewards evaluate request and determine if the feature meets edk2 
> criteria.
> c) If approved, use edk2 patch review/commit process on edk2-devel 
> mailing list
> d) Remove feature branch from edk2-staging (maybe archived 
> elsewhere?). 

Anything which is substantial enough to warrant this kind of 'staging'
definitely wants to be *pulled* into the main tree, rather than
rebasing it onto the current master, losing accurate history and
invalidating the testing that's been done.

The 'Reviewed-by' and similar tags could be collected by the submitter
and added in their own tree (leaving them in control of whether they
rebase, and responsible for subsequent retesting everything).

Perhaps better still, the Reviewed-by: could be included in the *merge*
commit.

Either way, let's not further entrench incorrect rebase behaviour.

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] EDK2 Staging Repository 2nd Draft

[David Woodhouse]

On Sat, 2016-03-12 at 00:25 +, Mangefeste, Tony wrote:
> 
> Problem statement
> =
> Need place on tianocore.org where new features that are not ready for
> product integration can be checked in for evaluation by the EDK II
> community prior to adding to the edk2 trunk.  This serves several
> purposes:
> 
> * Encourage source code to be shared earlier in the development
> process
> * Allow source code to be shared that does not yet meet all edk2
> required quality criteria
> * Allow source code to be shared so the EDK II community can help
> finish and validate new features
> * Provide a location to hold new features until they are deemed ready
> for product integration
> * Provide a location to hold new features until there is a natural
> point in edk2 release cycle to fully validate the new feature.
> * Not intended to be used for bug fixes.
> * Not intended to be used for small, simple, or low risk features.

Surely all of this is covered by the normal process of contributors
publishing their work in github repositories of their own?

Why do we need to do something different?

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch 2/6] CryptoPkg: Add OpensslTlsLib module to enable 'openssl\ssl'

[David Woodhouse]

On Fri, 2016-03-11 at 09:45 -0800, James Bottomley wrote:
> 
> > Did you *really* enable the building of openssl-1.0.2x/ssl/*.c in the
> > above builds? Or are you talking about something different?
> 
> No, and since I'm not using it, separating it is fine with me.

And here's where I came in... is there any need for it to be separate?

If the objects corresponding to ssl/*.c are present in the library
archive, doesn't that just mean that they'll get pulled in if they're
*referenced*, and not if they're not? So why separate it out into
OpensslTlsLib at all?

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch 2/6] CryptoPkg: Add OpensslTlsLib module to enable 'openssl\ssl'

[David Woodhouse]

On Fri, 2016-03-11 at 09:25 -0800, James Bottomley wrote:
> 
> > With the ssl/ directory enabled?
> 
> Yes, if you crack the package, this is the contents:
> 
> /usr/include/edk2
> /usr/include/edk2/Base.h
> /usr/include/edk2/Guid
> /usr/include/edk2/Guid/GlobalVariable.h
> /usr/include/edk2/Guid/ImageAuthentication.h
> /usr/include/edk2/Library
> /usr/include/edk2/Library/BaseCryptLib.h
> /usr/include/edk2/ProcessorBind.h
> /usr/include/edk2/Protocol
> /usr/include/edk2/Protocol/Hash.h
> /usr/include/edk2/Protocol/Pkcs7Verify.h
> /usr/include/edk2/Uefi
> /usr/include/edk2/Uefi/UefiBaseType.h
> /usr/lib64/edk2
> /usr/lib64/edk2/OpensslLib.lib
> 
> It's the OpensslLib.lib that allows you to link all openssl functions
> in EFI.  It's cheating quite a bit because the headers aren't present,
> so you use the Linux headers from openssl-devel when you compile.

That's dangerous and likely to break; various structures change.

But still, the OpensslLib in EDK2 only builds libcrypto; the contents
of the crypto/ directory of OpenSSL.

The discussion here is about building libssl, from ssl/.

Jiaxin proposed building it as a separate library. I asked why not just
build it into the *same* OpensslLib.

Did you *really* enable the building of openssl-1.0.2x/ssl/*.c in the
above builds? Or are you talking about something different?

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 4/9] CryptoPkg/OpensslLib: Fix GCC unused-value warnings with HOST_c2l() (RT#4347)

[David Woodhouse]

On Fri, 2016-03-11 at 17:30 +, David Woodhouse wrote:
> If we actually allow GCC to produce warnings, we'll see a lot of these:
> …/crypto/md5/md5_dgst.c:109:56: error: right-hand operand of comma expression 
> has no effect [-Werror=unused-value]
> 
> These were fixed in OpenSSL 1.1; backport the fix to our 1.0.2 tree too.

I also pushed this to my OpenSSL-1.0.2g-EDK2 branch from which the
EDKII_openssl patches are now autogenerated:

https://github.com/dwmw2/openssl/commits/OpenSSL-1.0.2g-EDK2

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH 9/9] CryptoPkg/OpensslLib: Enable building of ssl/ subdirectory of OpenSSL

[David Woodhouse]

Since it's just a library archive, let's just build the ssl/ parts
unconditionally. If they're referenced, they'll get pulled in. If not
then they won't.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: David Woodhouse <david.woodho...@intel.com>
---
An alternative approach, which might be simpler. qv.

 CryptoPkg/Library/OpensslLib/OpensslLib.inf   | 39 +++
 CryptoPkg/Library/OpensslLib/process_files.sh | 12 -
 2 files changed, 44 insertions(+), 7 deletions(-)

diff --git a/CryptoPkg/Library/OpensslLib/OpensslLib.inf 
b/CryptoPkg/Library/OpensslLib/OpensslLib.inf
index d48c8f1..50f256d 100644
--- a/CryptoPkg/Library/OpensslLib/OpensslLib.inf
+++ b/CryptoPkg/Library/OpensslLib/OpensslLib.inf
@@ -477,6 +477,45 @@
   $(OPENSSL_PATH)/crypto/kdf/tls1_prf.c
   $(OPENSSL_PATH)/crypto/kdf/kdf_err.c
   $(OPENSSL_PATH)/crypto/kdf/hkdf.c
+  $(OPENSSL_PATH)/ssl/pqueue.c
+  $(OPENSSL_PATH)/ssl/statem/statem_srvr.c
+  $(OPENSSL_PATH)/ssl/statem/statem_clnt.c
+  $(OPENSSL_PATH)/ssl/s3_lib.c
+  $(OPENSSL_PATH)/ssl/s3_enc.c
+  $(OPENSSL_PATH)/ssl/record/rec_layer_s3.c
+  $(OPENSSL_PATH)/ssl/statem/statem_lib.c
+  $(OPENSSL_PATH)/ssl/s3_cbc.c
+  $(OPENSSL_PATH)/ssl/s3_msg.c
+  $(OPENSSL_PATH)/ssl/methods.c
+  $(OPENSSL_PATH)/ssl/t1_lib.c
+  $(OPENSSL_PATH)/ssl/t1_enc.c
+  $(OPENSSL_PATH)/ssl/t1_ext.c
+  $(OPENSSL_PATH)/ssl/d1_lib.c
+  $(OPENSSL_PATH)/ssl/record/rec_layer_d1.c
+  $(OPENSSL_PATH)/ssl/d1_msg.c
+  $(OPENSSL_PATH)/ssl/statem/statem_dtls.c
+  $(OPENSSL_PATH)/ssl/d1_srtp.c
+  $(OPENSSL_PATH)/ssl/ssl_lib.c
+  $(OPENSSL_PATH)/ssl/ssl_cert.c
+  $(OPENSSL_PATH)/ssl/ssl_sess.c
+  $(OPENSSL_PATH)/ssl/ssl_ciph.c
+  $(OPENSSL_PATH)/ssl/ssl_stat.c
+  $(OPENSSL_PATH)/ssl/ssl_rsa.c
+  $(OPENSSL_PATH)/ssl/ssl_asn1.c
+  $(OPENSSL_PATH)/ssl/ssl_txt.c
+  $(OPENSSL_PATH)/ssl/ssl_init.c
+  $(OPENSSL_PATH)/ssl/ssl_conf.c
+  $(OPENSSL_PATH)/ssl/ssl_mcnf.c
+  $(OPENSSL_PATH)/ssl/bio_ssl.c
+  $(OPENSSL_PATH)/ssl/ssl_err.c
+  $(OPENSSL_PATH)/ssl/t1_reneg.c
+  $(OPENSSL_PATH)/ssl/tls_srp.c
+  $(OPENSSL_PATH)/ssl/t1_trce.c
+  $(OPENSSL_PATH)/ssl/ssl_utst.c
+  $(OPENSSL_PATH)/ssl/record/ssl3_buffer.c
+  $(OPENSSL_PATH)/ssl/record/ssl3_record.c
+  $(OPENSSL_PATH)/ssl/record/dtls1_bitmap.c
+  $(OPENSSL_PATH)/ssl/statem/statem.c
 
 # Autogenerated files list ends here
 
diff --git a/CryptoPkg/Library/OpensslLib/process_files.sh 
b/CryptoPkg/Library/OpensslLib/process_files.sh
index 8ab0deb..90fa6a1 100755
--- a/CryptoPkg/Library/OpensslLib/process_files.sh
+++ b/CryptoPkg/Library/OpensslLib/process_files.sh
@@ -70,13 +70,11 @@ function filelist ()
    ;;
    LIBSRC=*)
    LIBSRC=$(echo "$LINE" | sed s/^LIBSRC=//)
-   if [ "$RELATIVE_DIRECTORY" != "ssl" ]; then
-   for FILE in $LIBSRC; do
-   if [ "$FILE" != "b_print.c" ]; then
-   echo -e '  
$(OPENSSL_PATH)/'$RELATIVE_DIRECTORY/$FILE\\r\\
-   fi
-   done
-   fi
+   for FILE in $LIBSRC; do
+   if [ "$FILE" != "b_print.c" ]; then
+   echo -e '  
$(OPENSSL_PATH)/'$RELATIVE_DIRECTORY/$FILE\\r\\
+   fi
+   done
    ;;
    esac
 done
-- 
2.5.0

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation


smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH 8/9] CryptoPkg: Abuse internal headers to make OpenSSL HEAD build work

[David Woodhouse]

More stuff got hidden. Some of this is tolerable. Other bits are
horrid, but given that we expose *requires* that we know the size
of the data structure, it's hard to see how we can avoid it.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: David Woodhouse <david.woodho...@intel.com>
---
Really need to sort this one out properly...

 CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c  | 7 ---
 CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c | 6 --
 CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Sign.c  | 1 +
 CryptoPkg/Library/BaseCryptLib/Pk/CryptTs.c | 2 ++
 CryptoPkg/Library/BaseCryptLib/Pk/CryptX509.c   | 1 +
 5 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c 
b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c
index 693cd32..93c2bcb 100644
--- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c
+++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c
@@ -14,7 +14,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 
 #include "InternalCryptLib.h"
 #include 
-
+#include <../hmac/hmac_lcl.h>
 /**
   Retrieves the size, in bytes, of the context buffer required for HMAC-MD5 
operations.
 
@@ -65,7 +65,8 @@ HmacMd5Init (
   //
   // OpenSSL HMAC-MD5 Context Initialization
   //
-  HMAC_CTX_init (HmacMd5Context);
+  memset(HmacMd5Context, 0, sizeof(HMAC_CTX));
+  HMAC_CTX_reset (HmacMd5Context);
   HMAC_Init_ex (HmacMd5Context, Key, (UINT32) KeySize, EVP_md5(), NULL);
 
   return TRUE;
@@ -191,7 +192,7 @@ HmacMd5Final (
   // OpenSSL HMAC-MD5 digest finalization
   //
   HMAC_Final (HmacMd5Context, HmacValue, );
-  HMAC_CTX_cleanup (HmacMd5Context);
+  HMAC_CTX_reset (HmacMd5Context);
 
   return TRUE;
 }
diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c 
b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c
index 881d26c..5710f26 100644
--- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c
+++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c
@@ -14,6 +14,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 
 #include "InternalCryptLib.h"
 #include 
+#include <../hmac/hmac_lcl.h>
 
 /**
   Retrieves the size, in bytes, of the context buffer required for HMAC-SHA1 
operations.
@@ -65,7 +66,8 @@ HmacSha1Init (
   //
   // OpenSSL HMAC-SHA1 Context Initialization
   //
-  HMAC_CTX_init (HmacSha1Context);
+  memset(HmacSha1Context, 0, sizeof(HMAC_CTX));
+  HMAC_CTX_reset (HmacSha1Context);
   HMAC_Init_ex (HmacSha1Context, Key, (UINT32) KeySize, EVP_sha1(), NULL);
 
   return TRUE;
@@ -191,7 +193,7 @@ HmacSha1Final (
   // OpenSSL HMAC-SHA1 digest finalization
   //
   HMAC_Final (HmacSha1Context, HmacValue, );
-  HMAC_CTX_cleanup (HmacSha1Context);
+  HMAC_CTX_reset (HmacSha1Context);
 
   return TRUE;
 }
diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Sign.c 
b/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Sign.c
index 704eb4e..8e0d896 100644
--- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Sign.c
+++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Sign.c
@@ -17,6 +17,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 #include 
 #include 
 #include 
+#include 
 
 
 /**
diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptTs.c 
b/CryptoPkg/Library/BaseCryptLib/Pk/CryptTs.c
index d495812..c6799ae 100644
--- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptTs.c
+++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptTs.c
@@ -23,6 +23,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 #include 
 #include 
 #include 
+#include <../evp/evp_locl.h>
 
 //
 // OID ASN.1 Value for SPC_RFC3161_OBJID ("1.3.6.1.4.1.311.3.3.1")
@@ -285,6 +286,7 @@ CheckTSTInfo (
   if (HashedMsg == NULL) {
 goto _Exit;
   }
+  memset(, 0, sizeof(MdCtx));
   EVP_DigestInit (, Md);
   EVP_DigestUpdate (, TimestampedData, DataSize);
   EVP_DigestFinal (, HashedMsg, NULL);
diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptX509.c 
b/CryptoPkg/Library/BaseCryptLib/Pk/CryptX509.c
index 7dc4596..d392bed 100644
--- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptX509.c
+++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptX509.c
@@ -15,6 +15,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 #include "InternalCryptLib.h"
 #include 
 #include 
+#include 
 
 /**
   Construct a X509 object from DER-encoded certificate data.
-- 
2.5.0

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation


smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH 6/9] CryptoPkg: Fix time(NULL) crash

[David Woodhouse]

From: Jiaxin Wu <jiaxin...@intel.com>

The POSIX time() function can be called with a NULL pointer, but our
implementation in BaseCryptLib didn't cope with that. Fix it, since
we want to start building the ssl/ directory of OpenSSL too, and it
does precisely this.

Cc: Long Qin <qin.l...@intel.com>
Cc: Ye Ting <ting...@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jiaxin Wu <jiaxin...@intel.com>
Reviewed-by: David Woodhouse <david.woodho...@intel.com>
---
I took the liberty of changing the commit comment a little.

 .../Library/BaseCryptLib/SysCall/TimerWrapper.c| 29 +-
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/CryptoPkg/Library/BaseCryptLib/SysCall/TimerWrapper.c 
b/CryptoPkg/Library/BaseCryptLib/SysCall/TimerWrapper.c
index 6422d61..93e487d 100644
--- a/CryptoPkg/Library/BaseCryptLib/SysCall/TimerWrapper.c
+++ b/CryptoPkg/Library/BaseCryptLib/SysCall/TimerWrapper.c
@@ -2,7 +2,7 @@
   C Run-Time Libraries (CRT) Time Management Routines Wrapper Implementation
   for OpenSSL-based Cryptographic Library (used in DXE & RUNTIME).
 
-Copyright (c) 2010 - 2011, Intel Corporation. All rights reserved.
+Copyright (c) 2010 - 2016, Intel Corporation. All rights reserved.
 This program and the accompanying materials
 are licensed and made available under the terms and conditions of the BSD 
License
 which accompanies this distribution.  The full text of the license may be 
found at
@@ -73,6 +73,7 @@ UINTN CumulativeDays[2][14] = {
 time_t time (time_t *timer)
 {
   EFI_TIME  Time;
+  time_tCalTime;
   UINTN Year;
 
   //
@@ -84,22 +85,26 @@ time_t time (time_t *timer)
   // Years Handling
   // UTime should now be set to 00:00:00 on Jan 1 of the current year.
   //
-  for (Year = 1970, *timer = 0; Year != Time.Year; Year++) {
-*timer = *timer + (time_t)(CumulativeDays[IsLeap(Year)][13] * SECSPERDAY);
+  for (Year = 1970, CalTime = 0; Year != Time.Year; Year++) {
+CalTime = CalTime + (time_t)(CumulativeDays[IsLeap(Year)][13] * 
SECSPERDAY);
   }
 
   //
   // Add in number of seconds for current Month, Day, Hour, Minute, Seconds, 
and TimeZone adjustment
   //
-  *timer = *timer + 
-   (time_t)((Time.TimeZone != EFI_UNSPECIFIED_TIMEZONE) ? 
(Time.TimeZone * 60) : 0) +
-   (time_t)(CumulativeDays[IsLeap(Time.Year)][Time.Month] * 
SECSPERDAY) + 
-   (time_t)(((Time.Day > 0) ? Time.Day - 1 : 0) * SECSPERDAY) + 
-   (time_t)(Time.Hour * SECSPERHOUR) + 
-   (time_t)(Time.Minute * 60) + 
-   (time_t)Time.Second;
-
-  return *timer;
+  CalTime = CalTime + 
+(time_t)((Time.TimeZone != EFI_UNSPECIFIED_TIMEZONE) ? 
(Time.TimeZone * 60) : 0) +
+(time_t)(CumulativeDays[IsLeap(Time.Year)][Time.Month] * 
SECSPERDAY) + 
+(time_t)(((Time.Day > 0) ? Time.Day - 1 : 0) * SECSPERDAY) + 
+(time_t)(Time.Hour * SECSPERHOUR) + 
+(time_t)(Time.Minute * 60) + 
+(time_t)Time.Second;
+
+  if (timer != NULL) {
+*timer = CalTime;
+  }
+
+  return CalTime;
 }
 
 //
-- 
2.5.0

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation


smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH 4/9] CryptoPkg/OpensslLib: Fix GCC unused-value warnings with HOST_c2l() (RT#4347)

[David Woodhouse]

If we actually allow GCC to produce warnings, we'll see a lot of these:
…/crypto/md5/md5_dgst.c:109:56: error: right-hand operand of comma expression 
has no effect [-Werror=unused-value]

These were fixed in OpenSSL 1.1; backport the fix to our 1.0.2 tree too.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: David Woodhouse <david.woodho...@intel.com>
---
 .../Library/OpensslLib/EDKII_openssl-1.0.2g.patch  | 168 +
 1 file changed, 168 insertions(+)

diff --git a/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2g.patch 
b/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2g.patch
index 25dbebc..58565b6 100644
--- a/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2g.patch
+++ b/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2g.patch
@@ -915,6 +915,79 @@ index 5be9e33..63c8866 100644
  
  int EVP_BytesToKey(const EVP_CIPHER *type, const EVP_MD *md,
 const unsigned char *salt, const unsigned char *data,
+diff --git a/crypto/md5/md5_dgst.c b/crypto/md5/md5_dgst.c
+index 2b51946..4ec1719 100644
+--- a/crypto/md5/md5_dgst.c
 b/crypto/md5/md5_dgst.c
+@@ -106,52 +106,52 @@ void md5_block_data_order(MD5_CTX *c, const void *data_, 
size_t num)
+ D = c->D;
+ 
+ for (; num--;) {
+-HOST_c2l(data, l);
++(void)HOST_c2l(data, l);
+ X(0) = l;
+-HOST_c2l(data, l);
++(void)HOST_c2l(data, l);
+ X(1) = l;
+ /* Round 0 */
+ R0(A, B, C, D, X(0), 7, 0xd76aa478L);
+-HOST_c2l(data, l);
++(void)HOST_c2l(data, l);
+ X(2) = l;
+ R0(D, A, B, C, X(1), 12, 0xe8c7b756L);
+-HOST_c2l(data, l);
++(void)HOST_c2l(data, l);
+ X(3) = l;
+ R0(C, D, A, B, X(2), 17, 0x242070dbL);
+-HOST_c2l(data, l);
++(void)HOST_c2l(data, l);
+ X(4) = l;
+ R0(B, C, D, A, X(3), 22, 0xc1bdceeeL);
+-HOST_c2l(data, l);
++(void)HOST_c2l(data, l);
+ X(5) = l;
+ R0(A, B, C, D, X(4), 7, 0xf57c0fafL);
+-HOST_c2l(data, l);
++(void)HOST_c2l(data, l);
+ X(6) = l;
+ R0(D, A, B, C, X(5), 12, 0x4787c62aL);
+-HOST_c2l(data, l);
++(void)HOST_c2l(data, l);
+ X(7) = l;
+ R0(C, D, A, B, X(6), 17, 0xa8304613L);
+-HOST_c2l(data, l);
++(void)HOST_c2l(data, l);
+ X(8) = l;
+ R0(B, C, D, A, X(7), 22, 0xfd469501L);
+-HOST_c2l(data, l);
++(void)HOST_c2l(data, l);
+ X(9) = l;
+ R0(A, B, C, D, X(8), 7, 0x698098d8L);
+-HOST_c2l(data, l);
++(void)HOST_c2l(data, l);
+ X(10) = l;
+ R0(D, A, B, C, X(9), 12, 0x8b44f7afL);
+-HOST_c2l(data, l);
++(void)HOST_c2l(data, l);
+ X(11) = l;
+ R0(C, D, A, B, X(10), 17, 0x5bb1L);
+-HOST_c2l(data, l);
++(void)HOST_c2l(data, l);
+ X(12) = l;
+ R0(B, C, D, A, X(11), 22, 0x895cd7beL);
+-HOST_c2l(data, l);
++(void)HOST_c2l(data, l);
+ X(13) = l;
+ R0(A, B, C, D, X(12), 7, 0x6b901122L);
+-HOST_c2l(data, l);
++(void)HOST_c2l(data, l);
+ X(14) = l;
+ R0(D, A, B, C, X(13), 12, 0xfd987193L);
+-HOST_c2l(data, l);
++(void)HOST_c2l(data, l);
+ X(15) = l;
+ R0(C, D, A, B, X(14), 17, 0xa679438eL);
+ R0(B, C, D, A, X(15), 22, 0x49b40821L);
 diff --git a/crypto/opensslconf.h.in b/crypto/opensslconf.h.in
 index 7a1c85d..7162c0f 100644
 --- a/crypto/opensslconf.h.in
@@ -1221,6 +1294,101 @@ index 4e06218..ddead3d 100644
  
  const EVP_PKEY_ASN1_METHOD rsa_asn1_meths[] = {
  {
+diff --git a/crypto/sha/sha256.c b/crypto/sha/sha256.c
+index 72a1159..64702cd 100644
+--- a/crypto/sha/sha256.c
 b/crypto/sha/sha256.c
+@@ -184,7 +184,7 @@ static void sha256_block_data_order(SHA256_CTX *ctx, const 
void *in,
+ h = ctx->h[7];
+ 
+ for (i = 0; i < 16; i++) {
+-HOST_c2l(data, l);
++(void)HOST_c2l(data, l);
+ T1 = X[i] = l;
+ T1 += h + Sigma1(e) + Ch(e, f, g) + K256[i];
+ T2 = Sigma0(a) + Maj(a, b, c);
+@@ -308,52 +308,52 @@ static void sha256_block_data_order(SHA256_CTX *ctx, 
const void *in,
+ } else {
+ SHA_LONG l;
+ 
+-HOST_c2l(data, l);
++(void)HOST_c2l(data, l);
+ T1 = X[0] = l;
+ ROUND_00_15(0, a, b, c, d, e, f, g, h);
+-HOST_c2l(data, l);
++(void)HOST_c2l(data, l);
+ T1 = X[1] = l;
+ ROUND_00_15(1, h, a, b, c, d, e, f, g);
+-HOST_c2l(data, l);
++(void)HOST_c2l(data, l);
+ T1 = X[2] = l;
+ ROUND_00_15(2, g, h, a, b, c, d, e, f);
+-HOST_c2l(data, l);
++(void)HOST_c2l(data, l);
+ T1 = X[3] = l;
+ ROUND_00_15(3, f, g, h, a, b, c, d, e);
+-HOST_c2l(data, l);
++(void)H

[edk2] [PATCH 5/9] CryptoPkg/OpensslLib: Enable warnings in GCC builds

[David Woodhouse]

[This space intentionally left blank, in case I accidentally venture
 an opinion about the fact that we *ever* added '-w' to the build
 flags of a security-sensitive piece of code.]

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: David Woodhouse <david.woodho...@intel.com>
---
 CryptoPkg/Library/OpensslLib/OpensslLib.inf | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/CryptoPkg/Library/OpensslLib/OpensslLib.inf 
b/CryptoPkg/Library/OpensslLib/OpensslLib.inf
index 8757100..9e5897f 100644
--- a/CryptoPkg/Library/OpensslLib/OpensslLib.inf
+++ b/CryptoPkg/Library/OpensslLib/OpensslLib.inf
@@ -508,11 +508,11 @@
   INTEL:*_*_X64_CC_FLAGS= -U_WIN32 -U_WIN64 -U_MSC_VER -U__ICC 
$(OPENSSL_FLAGS) /w
   INTEL:*_*_IPF_CC_FLAGS= -U_WIN32 -U_WIN64 -U_MSC_VER -U__ICC 
$(OPENSSL_FLAGS) /w
 
-  GCC:*_*_IA32_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -w
-  GCC:*_*_X64_CC_FLAGS  = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -w 
-UNO_BUILTIN_VA_FUNCS
-  GCC:*_*_IPF_CC_FLAGS  = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) -w
-  GCC:*_*_ARM_CC_FLAGS  = $(OPENSSL_FLAGS) -w
-  GCC:*_*_AARCH64_CC_FLAGS  = $(OPENSSL_FLAGS) -w
+  GCC:*_*_IA32_CC_FLAGS = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS)
+  GCC:*_*_X64_CC_FLAGS  = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS) 
-UNO_BUILTIN_VA_FUNCS
+  GCC:*_*_IPF_CC_FLAGS  = -U_WIN32 -U_WIN64 $(OPENSSL_FLAGS)
+  GCC:*_*_ARM_CC_FLAGS  = $(OPENSSL_FLAGS)
+  GCC:*_*_AARCH64_CC_FLAGS  = $(OPENSSL_FLAGS)
 
   # suppress the following warnings in openssl so we don't break the build 
with warnings-as-errors:
   # 1295: Deprecated declaration  - give arg types
-- 
2.5.0

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation


smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH 3/9] CryptoPkg/OpensslLib: Fix handling of function pointers

[David Woodhouse]

In a couple of places, OpenSSL code uses the address of the strcmp()
function, and assigns it to another comparator function pointer.

Unfortunately, this falls foul of the inconsistent function ABI that we
use in EDKII. We '#define strcmp AsciiStrCmp' but AsciiStrCmp is an
EFIAPI function with the Microsoft ABI. And we're assigning its address
to a non-EFIAPI function, which may well have a different ABI.

The compiler *should* have complained about this error, thus:

…/crypto/objects/o_names.c: In function ‘OBJ_NAME_new_index’:
…/crypto/objects/o_names.c:94:30: error: assignment from incompatible pointer 
type [-Werror=incompatible-pointer-types]
 name_funcs->cmp_func = OPENSSL_strcmp;
  ^

Unfortunately, all warnings are disabled when building OpenSSL code.

There's another one in crypto/lhash/lhash.c::lh_new() which has an
explicit cast so even with compiler warnings we wouldn't have seen it.

Fix this by providing an actual strcmp() function in the default ABI.
We already *had* a prototype for it in OpenSslSupport.h, which was then
superseded by the #define strcmp AsciiStrCmp.

Now, OpenSSL code *can* use  without problems.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: David Woodhouse <david.woodho...@intel.com>
---
 CryptoPkg/Include/OpenSslSupport.h| 1 -
 CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c | 6 ++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/CryptoPkg/Include/OpenSslSupport.h 
b/CryptoPkg/Include/OpenSslSupport.h
index 239ae8b..8bb4277 100644
--- a/CryptoPkg/Include/OpenSslSupport.h
+++ b/CryptoPkg/Include/OpenSslSupport.h
@@ -261,7 +261,6 @@ extern FILE  *stdout;
 #define memchr(buf,ch,count)  
ScanMem8(buf,(UINTN)(count),(UINT8)ch)
 #define memcmp(buf1,buf2,count)   
(int)(CompareMem(buf1,buf2,(UINTN)(count)))
 #define memmove(dest,source,count)CopyMem(dest,source,(UINTN)(count))
-#define strcmpAsciiStrCmp
 #define strncmp(string1,string2,count)
(int)(AsciiStrnCmp(string1,string2,(UINTN)(count)))
 #define strcpy(strDest,strSource) 
AsciiStrCpyS(strDest,MAX_STRING_SIZE,strSource)
 #define strncpy(strDest,strSource,count)  
AsciiStrnCpyS(strDest,MAX_STRING_SIZE,strSource,(UINTN)count)
diff --git a/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c 
b/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
index 9d6867e..f559da0 100644
--- a/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
+++ b/CryptoPkg/Library/IntrinsicLib/MemoryIntrinsics.c
@@ -15,6 +15,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 
 #include 
 #include 
+#include 
 
 /* OpenSSL will use floating point support, and C compiler produces the 
_fltused
symbol by default. Simply define this symbol here to satisfy the linker. */
@@ -44,3 +45,8 @@ void * memset (void *dest, char ch, unsigned int count)
   
   return dest;
 }
+
+int strcmp (const char *s1, const char *s2)
+{
+  return AsciiStrCmp(s1, s2);
+}
-- 
2.5.0

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation


smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH 2/9] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2g

[David Woodhouse]

From: Qin Long <qin.l...@intel.com>

OpenSSL 1.0.2g was released with several severity fixes at
01-Mar-2016(https://www.openssl.org/news/secadv/20160301.txt).
Upgrade the supported OpenSSL version in CryptoPkg/OpensslLib to
catch the latest release 1.0.2g.
(NOTE: RT4175 from David Woodhouse was included in 1.0.2g. The
   new-generated patch will remove this part. And the line
   endings were still kept as before in this version for
   consistency)

CC: Ting Ye <ting...@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Qin Long <qin.l...@intel.com>
Reviewed-by: David Woodhouse <david.woodho...@intel.com>
---
 CryptoPkg/CryptoPkg.dec|  2 +-
 ...ssl-1.0.2f.patch => EDKII_openssl-1.0.2g.patch} | 95 --
 CryptoPkg/Library/OpensslLib/Install.cmd   |  2 +-
 CryptoPkg/Library/OpensslLib/Install.sh|  2 +-
 CryptoPkg/Library/OpensslLib/OpensslLib.inf|  2 +-
 CryptoPkg/Library/OpensslLib/Patch-HOWTO.txt   | 26 +++---
 CryptoPkg/Library/OpensslLib/opensslconf.h |  6 ++
 7 files changed, 56 insertions(+), 79 deletions(-)
 rename CryptoPkg/Library/OpensslLib/{EDKII_openssl-1.0.2f.patch => 
EDKII_openssl-1.0.2g.patch} (94%)

diff --git a/CryptoPkg/CryptoPkg.dec b/CryptoPkg/CryptoPkg.dec
index 82d24f5..e1cdb8e 100644
--- a/CryptoPkg/CryptoPkg.dec
+++ b/CryptoPkg/CryptoPkg.dec
@@ -24,7 +24,7 @@
 
 [Includes]
   Include
-  Library/OpensslLib/openssl-1.0.2f/include
+  Library/OpensslLib/openssl-1.0.2g/include
 
 [LibraryClasses]
   ##  @libraryclass  Provides basic library functions for cryptographic 
primitives.
diff --git a/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2f.patch 
b/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2g.patch
similarity index 94%
rename from CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2f.patch
rename to CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2g.patch
index b799bf2..25dbebc 100644
--- a/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2f.patch
+++ b/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2g.patch
@@ -1,8 +1,8 @@
 diff --git a/Configure b/Configure
-index 4a715dc..b4a4781 100755
+index c98107a..c122709 100755
 --- a/Configure
 +++ b/Configure
-@@ -605,6 +605,9 @@ my %table=(
+@@ -609,6 +609,9 @@ my %table=(
  # with itself, Applink is never engaged and can as well be omitted.
  "mingw64", "gcc:-mno-cygwin -DL_ENDIAN -O3 -Wall -DWIN32_LEAN_AND_MEAN 
-DUNICODE -D_UNICODE::-D_MT:MINGW64:-lws2_32 -lgdi32 -lcrypt32:SIXTY_FOUR_BIT 
RC4_CHUNK_LL DES_INT 
EXPORT_VAR_AS_FN:${x86_64_asm}:mingw64:win32:cygwin-shared:-D_WINDLL:-mno-cygwin:.dll.a",
  
@@ -12,7 +12,7 @@ index 4a715dc..b4a4781 100755
  # UWIN 
  "UWIN", "cc:-DTERMIOS -DL_ENDIAN -O -Wall:::UWIN::BN_LLONG ${x86_gcc_des} 
${x86_gcc_opts}:${no_asm}:win32",
  
-@@ -1082,7 +1085,7 @@ if (defined($disabled{"tls1"}))
+@@ -1088,7 +1091,7 @@ if (defined($disabled{"tls1"}))
    }
  
  if (defined($disabled{"ec"}) || defined($disabled{"dsa"})
@@ -22,7 +22,7 @@ index 4a715dc..b4a4781 100755
    $disabled{"gost"} = "forced";
    }
 diff --git a/apps/apps.c b/apps/apps.c
-index 2e77805..e21e759 100644
+index b1dd970..8278c28 100644
 --- a/apps/apps.c
 +++ b/apps/apps.c
 @@ -2374,6 +2374,8 @@ int args_verify(char ***pargs, int *pargc,
@@ -462,7 +462,7 @@ index c042cf2..a25b636 100644
  }
  
 diff --git a/crypto/cryptlib.c b/crypto/cryptlib.c
-index c9f674b..39ead7f 100644
+index 1925428..da4b34d 100644
 --- a/crypto/cryptlib.c
 +++ b/crypto/cryptlib.c
 @@ -263,7 +263,7 @@ int CRYPTO_get_new_dynlockid(void)
@@ -525,7 +525,7 @@ index c9f674b..39ead7f 100644
  }
 +#endif
  
- int CRYPTO_memcmp(const void *in_a, const void *in_b, size_t len)
+ int CRYPTO_memcmp(const volatile void *in_a, const volatile void *in_b, 
size_t len)
  {
 diff --git a/crypto/cryptlib.h b/crypto/cryptlib.h
 index fba180a..3e3ea5e 100644
@@ -542,7 +542,7 @@ index fba180a..3e3ea5e 100644
  
  #ifdef  __cplusplus
 diff --git a/crypto/crypto.h b/crypto/crypto.h
-index c450d7a..063d78e 100644
+index 6c644ce..bea4ca1 100644
 --- a/crypto/crypto.h
 +++ b/crypto/crypto.h
 @@ -235,15 +235,15 @@ typedef struct openssl_item_st {
@@ -656,7 +656,7 @@ index 46fa5ac..cc366ec 100644
  dh_kdf.o: ../../include/openssl/crypto.h ../../include/openssl/dh.h
  dh_kdf.o: ../../include/openssl/e_os2.h ../../include/openssl/ec.h
 diff --git a/crypto/dh/dh.h b/crypto/dh/dh.h
-index 5498a9d..4a5c665 100644
+index a5bd901..6488879 100644
 --- a/crypto/dh/dh.h
 +++ b/crypto/dh/dh.h
 @@ -240,11 +240,13 @@ DH *DH_get_1024_160(void);
@@ -1021,7 +1021,7 @@ index 5747c73..fe465cc 100644
   * These functions write a private key in PKCS#8 format: it is a "drop in"
   * replacement for PEM_write_bio_PrivateKey() and friends. As usual if 'enc'
 diff --git a/crypto/pkcs7/pk7_smime.c b/crypto/pkcs7/pk7_smime.c
-index c4d3724..0bc3d43 

[edk2] [PATCH 1/9] CryptoPkg/OpensslLib: Convert saved opensslconf.h to DOS line endings

[David Woodhouse]

Until we fix the git repository to store line endings properly and then
just check them out in the appropriate form for the platform, let's make
process_files.sh convert the opensslconf.h to DOS line endings when it
creates it.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: David Woodhouse <david.woodho...@intel.com>
Reviewed-by: Qin Long <qin.l...@intel.com>
---
v2: Add missing '-n' arg.

 CryptoPkg/Library/OpensslLib/process_files.sh | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/CryptoPkg/Library/OpensslLib/process_files.sh 
b/CryptoPkg/Library/OpensslLib/process_files.sh
index bb33c8a..885adf3 100755
--- a/CryptoPkg/Library/OpensslLib/process_files.sh
+++ b/CryptoPkg/Library/OpensslLib/process_files.sh
@@ -93,5 +93,6 @@ function filelist ()
 filelist < "${OPENSSL_PATH}/MINFO" |  sed -n -f - -i OpensslLib.inf
 
 # We can tell Windows users to put this back manually if they can't run
-# Configure.
-cp "${OPENSSL_PATH}/crypto/opensslconf.h" .
+# Configure. For now, until the git repository is fixed to store things
+# sanely, also convert to DOS line-endings
+unix2dos -n "${OPENSSL_PATH}/crypto/opensslconf.h" opensslconf.h
-- 
2.5.0

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation


smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] More OpenSSL fun...

[David Woodhouse]

At git//, https://git.infradead.org/users/dwmw2/edk2.git and in
following emails you can find the following:

As before, the ones which update to OpenSSL HEAD, and after that, are
for comment only.

David Woodhouse (7):
      CryptoPkg/OpensslLib: Convert saved opensslconf.h to DOS line endings
  CryptoPkg/OpensslLib: Fix handling of  function pointers
  CryptoPkg/OpensslLib: Fix GCC unused-value warnings with HOST_c2l() 
(RT#4347)
  CryptoPkg/OpensslLib: Enable warnings in GCC builds
  CryptoPkg: Support building with OpenSSL HEAD (1.1.0-devel)
  CryptoPkg: Abuse internal headers to make OpenSSL HEAD build work
  CryptoPkg/OpensslLib: Enable building of ssl/ subdirectory of OpenSSL

Jiaxin Wu (1):
  CryptoPkg: Fix time(NULL) crash

Qin Long (1):
  CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2g


-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch 2/6] CryptoPkg: Add OpensslTlsLib module to enable 'openssl\ssl'

[David Woodhouse]

On Fri, 2016-03-11 at 07:54 -0800, James Bottomley wrote:
> 
> I package it here:
> 
> https://build.opensuse.org/package/show/home:jejb1:UEFI/OVMF
> 
> in edk2-devel

With the ssl/ directory enabled?

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch 2/6] CryptoPkg: Add OpensslTlsLib module to enable 'openssl\ssl'

[David Woodhouse]

On Fri, 2016-03-11 at 09:39 +, Long, Qin wrote:
> 
> > It looks like the libraries are built into an archive and then
> linked
> > statically. So only those objects which are *referenced* are
> actually
> > pulled into the build. Which means that if we just *add* the ssl/
> > directory to the OpensslLib build, it will only be pulled in if
> > something *uses* it. Doesn't it?
> > 
> 
> Yes, it's feasible to archive two libraries into one, and only
> referenced symbols will be included. 
> The current design (separated libraries) is try to keep the original
> openssl layout (libcrypto and libssl). Different library serve as
> different scopes. Of cause, the name of OpensslLib.inf looks
> confusing, which should be one crypto library only. 
> 
> I agree the proposal looks also valuable. We should ever discuss this
> internally. Let me try and get some size data for evaluations (I
> think the total symbols / functions in image still highly depend on
> the capabilities of the compiler / linker).

Yeah. With GCC we seem to have function granularity — if a function
isn't actually called, it gets completely dropped out of the image.

With MSVC it seems to be object file granularity. So if *one* function
in a given .obj file is called, that whole .obj file is included.

That's why we had additional problems with the MSVC build and needed
extra cleanups in the OpenSSL code (and I could reproduce them on Linux
by adding -fno-function-sections to the CFLAGS).

But I think that in both cases, we should have confidence that if you
don't *use* anything from the objects in the ssl/ directory, you won't
get anything else pulled in.

I'll try just fixing the process_files.sh script to include the ssl/*.c
files in the build. If I'm right, the resulting image will be
identical.

I'm slightly concerned by all the other duplication in the
OpensslTlsLib.inf file — the CFLAGS and other things. Merging them into
one, if it's technically feasible, does seem cleaner.

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2g

[David Woodhouse]

On Fri, 2016-03-11 at 00:20 -0800, Qin Long wrote:
> OpenSSL 1.0.2g was released with several severity fixes at
> 01-Mar-2016(https://www.openssl.org/news/secadv/20160301.txt).
> Upgrade the supported OpenSSL version in CryptoPkg/OpensslLib to
> catch the latest release 1.0.2g.
> (NOTE: RT4175 from David Woodhouse was included in 1.0.2g. The
>    new-generated patch will remove this part. And the line
>    endings were still kept as before in this version for
>    consistency)
> 
> CC: David Woodhouse <david.woodho...@intel.com>
> CC: Ting Ye <ting...@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Qin Long <qin.l...@intel.com>

Secure boot seems to work in OVMF using Laszlo's instructions (thanks
again). As does the Cryptest applications.

Reviewed-by: David Woodhouse <david.woodho...@intel.com>

-- 
-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch 2/6] CryptoPkg: Add OpensslTlsLib module to enable 'openssl\ssl'

[David Woodhouse]

On Wed, 2016-02-24 at 16:15 +0800, Jiaxin Wu wrote:
> --- a/CryptoPkg/CryptoPkg.dsc
> +++ b/CryptoPkg/CryptoPkg.dsc
> @@ -48,10 +48,11 @@
>    
> UefiDriverEntryPoint|MdePkg/Library/UefiDriverEntryPoint/UefiDriverEntryPoint.inf
>    
> UefiApplicationEntryPoint|MdePkg/Library/UefiApplicationEntryPoint/UefiApplicationEntryPoint.inf
>  
>    IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf
>    OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLib.inf
> +  OpensslTlsLib|CryptoPkg/Library/OpensslLib/OpensslTlsLib.inf

One more thing... does this *need* to be a separate library?

It looks like the libraries are built into an archive and then linked
statically. So only those objects which are *referenced* are actually
pulled into the build. Which means that if we just *add* the ssl/
directory to the OpensslLib build, it will only be pulled in if
something *uses* it. Doesn't it?

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation




smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg: Fix the potential system hang issue

[David Woodhouse]

On Fri, 2016-03-11 at 12:35 +0800, Jiaxin Wu wrote:
> This patch is used to fix the potential system hang
> caused by the NULL 'time' parameter usage.

Looks good. Thanks.

> Cc: David Woodhouse <dw...@infradead.org>
> Cc: Long Qin <qin.l...@intel.com>
> Cc: Ye Ting <ting...@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jiaxin Wu <jiaxin...@intel.com>

Reviewed-by: David Woodhouse <david.woodho...@intel.com>

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch 2/6] CryptoPkg: Add OpensslTlsLib module to enable 'openssl\ssl'

[David Woodhouse]

On Thu, 2016-03-10 at 17:12 +, Long, Qin wrote:
> 
> This patch series should be based on the old version, before the
> back-porting of upstreaming patch was done.
> We should have no need to add the extra patches on OpenSSL now for
> OpensslTlsLib build now.

Do you have a simple test case for OpensslTlsLib? I have it building
the library against 1.0.2, and I'll make sure it builds for 1.1 too
(although today was the last day for making significant changes in 1.1
so I hope we aren't missing anything!).

However, a simple UEFI application which can make a TLS connection
would be useful... or at least a git tree I can pull the HTTPS work
from; applying patches is kind of painful until we get the CRLF
nonsense fixed.

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] Tianocore Community Update 2016 #1

[David Woodhouse]

On Thu, 2016-03-10 at 13:58 +0100, Laszlo Ersek wrote:
> 
> > Sure, actually getting vendor buy-in for that is a completely different
> > story. But let's not design the system to make it hard :)
> 
> I couldn't buy in.

That's fine. I'm not asking you to. I'm just asking that we don't make
it *harder*.

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH] CryptoPkg/OpensslLib: Convert saved opensslconf.h to DOS line endings

[David Woodhouse]

Until we fix the git repository to store line endings properly and then
just check them out in the appropriate form for the platform, let's make
process_files.sh convert the opensslconf.h to DOS line endings when it
creates it.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: David Woodhouse <david.woodho...@intel.com>
---
(Resent from list-subscribed email address)

 CryptoPkg/Library/OpensslLib/process_files.sh | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/CryptoPkg/Library/OpensslLib/process_files.sh 
b/CryptoPkg/Library/OpensslLib/process_files.sh
index bb33c8a..1094fd4 100755
--- a/CryptoPkg/Library/OpensslLib/process_files.sh
+++ b/CryptoPkg/Library/OpensslLib/process_files.sh
@@ -93,5 +93,6 @@ function filelist ()
 filelist < "${OPENSSL_PATH}/MINFO" |  sed -n -f - -i OpensslLib.inf
 
 # We can tell Windows users to put this back manually if they can't run
-# Configure.
-cp "${OPENSSL_PATH}/crypto/opensslconf.h" .
+# Configure. For now, until the git repository is fixed to store things
+# sanely, also convert to DOS line-endings
+unix2dos "${OPENSSL_PATH}/crypto/opensslconf.h" opensslconf.h
-- 
2.5.0

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch 2/6] CryptoPkg: Add OpensslTlsLib module to enable 'openssl\ssl'

[David Woodhouse]

On Wed, 2016-02-24 at 16:15 +0800, Jiaxin Wu wrote:
> diff --git a/CryptoPkg/Library/OpensslLib/OpensslTlsLib.uni 
> b/CryptoPkg/Library/OpensslLib/OpensslTlsLib.uni
> new file mode 100644
> index 
> ..384f0245db8d1d3a1d758f6db58f85f5fc155211
> GIT binary patch
> literal 1792
> zcmd6oNpBND5QXcE#D5q$7eI*(95^6^u<-yRLdNo
> z1hRUq>Q}F-Uyr|ity#?y+9Q66y|RhTY;I$_Z}-@ht!o$TZI~@=Wh1+Edtz%VSZ}e7
> z@RvPjZ){8J@H=H4#u^Bx%oF;V4LH@OU9+BnxOTLKpDVGH?5@D5?il=!OkTrrO%4TY
> zr_`q;n+G<uhm23{u|2V8cFa5@lak!#%yp*Vl=}TV6RTsFM?_OK3$N;!am#E(MP69L

These should be UTF-8 now, shouldn't they? It seems wrong to be adding
new UTF-16 files while other people are submitting patches to convert
*from* UTF-16 to UTF-8.

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] Tianocore Community Update 2016 #1

[David Woodhouse]

On Thu, 2016-03-10 at 11:33 +0100, Laszlo Ersek wrote:
> 
> > * Considering tying the Bugzilla login to GitHub using GitHub as the
> > provider.  This would mean that anyone wishing to submit an item into
> > BZ would require a GitHub account.
> 
> I vote against this. I find 3rd party authentication providers insecure.

I concur.

The end goal should be a coherent bug tracking system where a user can
file a bug, and it can be reassigned to TianoCore or to a specific
vendor's "value subtract", and its *whole* lifetime can be tracked
until a fix is released for the specific instance that the user has
reported it with.

For that, we *are* going to need the thing to live under the auspices
of the UEFI Forum, and we are going to need to be able to mark things
as private — using an account system that is *directly* under our
control.

Sure, actually getting vendor buy-in for that is a completely different
story. But let's not design the system to make it hard :)

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch 2/6] CryptoPkg: Add OpensslTlsLib module to enable 'openssl\ssl'

[David Woodhouse]

On Wed, 2016-02-24 at 16:15 +0800, Jiaxin Wu wrote:
> diff --git a/CryptoPkg/Library/BaseCryptLib/SysCall/CrtWrapper.c
> b/CryptoPkg/Library/BaseCryptLib/SysCall/CrtWrapper.c
> index c0ccc0e..e68bfb8 100644
> --- a/CryptoPkg/Library/BaseCryptLib/SysCall/CrtWrapper.c
> +++ b/CryptoPkg/Library/BaseCryptLib/SysCall/CrtWrapper.c
> @@ -446,5 +446,10 @@ void syslog (int a, const char *c, ...)
>  
>  ssize_t write (int f, const void *b, size_t l)
>  {
>    return 0;
>  }
> +
> +int printf (char const *fmt, ...)
> +{
> +  return 0;
> +}

I'm assuming this is to work around a stray printf() in OpenSSL code
that we weren't building before?

The correct fix is to file a ticket upstream, submit a *fix* upstream
to be included in OpenSSL, and then to add the corresponding backported
patch to our EDKII_openssl patch.

Please don't add more workarounds like the above; we're trying to clean
those up not accumulate more.

And again... even if this was the right thing to do, it lives in a
*separate* standalone commit. It's OK if it's gratuitous, and the
commit comment simply says "we *will* want this because...".

See some of the OpenSSL API cleanups, for example, which make things
*ready* for OpenSSL 1.1 even while we're still using 1.0.2.

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch 2/6] CryptoPkg: Add OpensslTlsLib module to enable 'openssl\ssl'

[David Woodhouse]

On Wed, 2016-02-24 at 16:15 +0800, Jiaxin Wu wrote:
> diff --git a/CryptoPkg/Library/OpensslLib/OpensslTlsLib.inf 
> b/CryptoPkg/Library/OpensslLib/OpensslTlsLib.inf
> new file mode 100644
> index 000..fbd2b08
> --- /dev/null
> +++ b/CryptoPkg/Library/OpensslLib/OpensslTlsLib.inf
> @@ -0,0 +1,110 @@
 ...
> +[Sources]
> +  $(OPENSSL_PATH)/e_os.h
> +  $(OPENSSL_PATH)/ssl/s2_meth.c
> +  $(OPENSSL_PATH)/ssl/s2_srvr.c
> +  $(OPENSSL_PATH)/ssl/s2_clnt.c
> +  $(OPENSSL_PATH)/ssl/s2_lib.c
> +  $(OPENSSL_PATH)/ssl/s2_enc.c

These file lists are auto-generated now. You'll need to extend the
process_files.sh script to do this for OpensslTlsLib.inf just like it
does for OpensslLib.inf.

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch 2/6] CryptoPkg: Add OpensslTlsLib module to enable 'openssl\ssl'

[David Woodhouse]

On Wed, 2016-02-24 at 16:15 +0800, Jiaxin Wu wrote:
> 
> diff --git a/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2f.patch 
> b/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2f.patch
> index c42b776..f2d8f1a 100644
> --- a/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2f.patch
> +++ b/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2f.patch
> @@ -11,10 +11,19 @@ diff U3 crypto/bio/bio.h crypto/bio/bio.h
>   BIO *BIO_new_fp(FILE *stream, int close_flag);
>  +# ifndef OPENSSL_NO_FP_API
>   #  define BIO_s_file_internal    BIO_s_file
>   # endif
>   BIO *BIO_new(BIO_METHOD *type);
> +@@ -655,6 +655,8 @@
> + BIO *BIO_new_file(const char *filename, const char *mode);
> + BIO *BIO_new_fp(FILE *stream, int close_flag);
> + #  define BIO_s_file_internal    BIO_s_file
> ++# else
> ++#  define BIO_s_file_internal()  NULL
> + # endif
> + BIO *BIO_new(BIO_METHOD *type);
> + int BIO_set(BIO *a, BIO_METHOD *type);
>  diff U3 crypto/bio/bss_file.c crypto/bio/bss_file.c
>  --- crypto/bio/bss_file.c  Thu Jan 28 21:38:30 2016
>  +++ crypto/bio/bss_file.c  Wed Feb 17 16:01:02 2016
>  @@ -467,6 +467,23 @@
>   return (ret);

As a general rule, you should never make have been making changes to
this OpenSSL patch without ensuring that a ticket is filed upstream.

As of this week, there is *nothing* in the EDKII_openssl patch which
isn't a backport of a commit from OpenSSL 1.1. The patch is
autogenerated from a 1.0.2+backports git tree.

Adding to it like this was *never* acceptable. Sure, you were only
making it a little bit worse at a time, but please don't. It just isn't
the way to do things.

In this case, perhaps the *only* thing missing was the fact that this
should have been in its own separate commit, with a commit comment
*identifying* the upstream ticket (and OpenSSL 1.1 commit) in which it
was fixed. But that's important to get right too.

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] OpenSSL 1.1 status, and a worked example of why you should *NEVER* rebase

[David Woodhouse]

On Wed, 2016-03-09 at 09:58 +0100, Laszlo Ersek wrote:
> I disagree. A v1 can be (and very frequently is) applied from patches,
> if it passes review at the first try. When the patches are picked up
> from the list, and all patches get R-b's, then it's the maintainer that
> adds the tags, so the contributor need not do anything else.

OK. So v1 is not to be applied as-is from patches; it needs the
Reviewed-by tags added. Sometimes the submitter rounds them up when
submitting v2; sometimes the maintainer adds them.

> Whereas, for a purely merge-oriented workflow, even if the first
> submission passed review fully, it would be the submitter's
> responsibility to append the tags, and send a separate PULL with that
> version of the series.

Right. So in the optional case where the submitter wants to do that,
they can add the Reviewed-by and other tags for themselves before
submitting a v2. Or otherwise, the maintainer can add them.

Note that is perfectly feasible for the maintainer to add the R-b tags
even if he/she pulls the tree from the submitter.

How the commits get from A to B is mostly irrelevant — the important
thing for preserving history is that the commits are applied to the
right base, and the resulting code is identical.

(In the general case it's *also* frowned upon to edit commits after
they've been made public. It's mostly important when there are lots of
derivative trees which occasionally depend on each others' work. If you
base your work on the early commits in *my* repository, and then I mess
with them, then they're no longer the same commit. But in the EDK2
world I think that's less common, so editing commits is probably OK.)


> I was only trying to reconstruct the argument for this claim, from the
> kernel docs:
> 
> Note, however, that pulling patches from a developer requires a
> higher degree of trust than taking patches from a mailing list.
> 
> It is perfectly possible that my attempt to retrofit a security argument
> to this claim failed. But, in that case, can you please explain why the
> above claim is valid? Or are you suggesting that this statement of
> SubmittingPatches is bogus?

I think the trust bit is bogus these days yes. I'll go fix it (qv).

It might have had some historical relevance, when the tools were such
that applying a series of patches *did* actually mean you had to look
at them. But it's so easy to save a mailbox and use 'git am' these
days, that crap can slip in *just* as easily as with a 'git pull'.

> For several versions of the same patchset, I have a good workflow where
> I can incrementally review only the new stuff (the changes), without
> fully trusting the submitter. I compare the patch emails as well between
> each other, and I can git-diff the end results of the various versions.
> 
> The same seems possible with branches that are to be merged, sure, it's
> just that I don't yet have hands-on experience with that.

Yeah, it's basically the same. But again, this is why we shouldn't take
half-measures. You're getting used to a workflow that *doesn't*
represent how we should be using the tools correctly. And that means
the pain of the transition to git is multiplied.

> Thank you, but thus far noone has complained as vehemently as you about
> rebasing as a norm (for now), for the sake of a linear history. I've had
> series with 50+ patches, and I don't recall any rebase-incurred problems.

Excellent. That means we can fix it, and start keeping *accurate*
history in the version control system (which is, after all, what
version control systems are for), before our laxity ever caused you a
real problem.

> I don't want to dismiss this endeavor, but with QEMU and top Linux
> maintainers insisting on signed tag pulls, and you saying "nonsense" to
> it (or to my attempt anyway to interpret that requirement), I feel much
> less confident about a pull-based workflow.

Signed pulls are about personal trust. If you have signed emails or a
signed pull request from someone you trust sufficiently, you don't
*need* to look at the content. There is no difference between email and
pull requests, in this respect.

Likewise, if you are taking submissions from someone in whom you do not
have sufficient trust, then you *do* to need to look at the content.
There is no difference between email and pull request in this respect
either.

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] OpenSSL 1.1 status, and a worked example of why you should *NEVER* rebase

[David Woodhouse]

On Wed, 2016-03-09 at 07:49 +0700, Ard Biesheuvel wrote:
> 
> I agree that they should be allowed, but i share the concern that
> merging puts the burden of fixing up conflicts on the maintainer
> rather than the contributor, who is arguably in a worse position to
> assess any potential problems on a seemingly clean merge, especially
> since merges are much more forgiving than rebases.

For a not-yet-proficient maintainer to ask a submitter to perform the
merge for themselves, or to verify the result of a merge, seems
reasonable.

> On top of that, the current crop of Tianocore committers is not
> entirely on top of things yet as fas as git is concerned, and having
> non-linear history just because someone couldn't be bothered to do a
> pull beforehand should also be avoided imo.

Nah, history that is trivially non-linear is fine. There's absolutely
no harm in it. Even for backporting to SVN it's easy enough; you just
take a line through it which represents the state of the upstream tree
at any given time. Thus collapsing the merge commits into a single SVN
commit. But nobody cares because that's no worse than what SVN made
people do anyway.

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] OpenSSL 1.1 status, and a worked example of why you should *NEVER* rebase

[David Woodhouse]

On Wed, 2016-03-09 at 00:05 +0100, Laszlo Ersek wrote:
> (1) The submitter is himself/herself responsible for picking up review
> tags, and then for posting a final (fully reviewed) PULL that can be
> merged without *any* kind of rebase by the pulling maintainer.
> 
> Corollary: since the first submission never has any reviews, the first
> submission should never be a PULL.

If submissions *require* review, the first submission isn't intended to
be merged anyway. So it's meaningless to say *either* that the first
submission is to be merged by applying patches, *or* that it's to be
merged as a pull. This is fairly much a non-sequitur.

> It seems to me that the extra trust for a pull request is needed because
> it's possible to post patches to the list (for review) that *differ*
> from the commits that *actually* lead up to the hash that is presented
> in the pull request. An attacker can prepare two patch sets, an innocent
> and a malicious one; post one series for review, but include the final
> commit hash of the other series in the pull request.
>
> Whereas when the maintainer applies patches from the list (i.e., at that
> point: from his own mailbox) directly, then the maintainer can be
> (reasonably) sure that those are exactly the patches the community
> reviewed (not counting mailbox break-ins etc).

Nonsense. If I go through seven rounds of posting patches to the list,
and I gather a series of acks from the first six but you apply the
seventh, you have *no* good reason to believe that my final submission
doesn't contain the trojan horse. You have to *look* before you apply
it.

And it's *just* the same if my final submission is actually in the form
of a pull request. Sure, the signed tag means you know it comes from
*me*, which can be achieved by signed email too (like this one). But
you still have to trust *me* just the same.

Or actually do a final review on the code you're actually *merging*.
Whether it be in email patches or in a pull request, that's just the
same.

Really, from the trust point of view there is *not* a difference
between pull requests and patches. Look at what you merge, or trust the
submitter. It's as simple as that.

> ... So, I dunno what to do about this. I trust you, but I wouldn't want
> to open up the possibility of *any* maintainer pulling from *any*
> contributor.
>
> * How about an alternative.
> 
> (a) Contributors are encouraged to name the fork-off point (commit hash)
> of their topic branch, from which they post their patches.
> 
> (b) For a large series, the maintainer is expected to apply the patches
> on a local, temporary branch, forked off of exactly the named commit of
> the master branch. Review tags are also added on this branch.
> 
> (c) Maintainer merges topic into master locally.
> 
> (d) Maintainer pushes the merge commit to his or her personal repo.
> 
> (e) Submitter fetches maintainer's personal repo, and confirms (in
> email, stating the hash of the merge commit) that it's fine.
> 
> (f) Maintainer pushes the merge commit (and its dependencies of course)
> to upstream master.
> 
> Now, steps (d) and (e) add a round-trip that can make the merge commit
> prepared in step (c) obsolete, by the time the confirmation arrives. So
> perhaps we can drop (d) and (e).
> 
> The point is, your history would be precisely reconstructed, and you
> could get to say the final OK.

No, this is pointless complexity.

Again, just *look* at what you merge. Whether it means a proper read
through the seventh round of patches, or actually looking at the
contents of a pull request, there's *no* difference.

> * There is another alternative. You should become an official
> co-maintainer of CryptoPkg (using your Intel email address), and then
> you could push your merges directly, after review. As I said, I trust
> you, but I don't trust a setup where any maintainer can pull from any
> contributor.

That's a band-aid for the one issue. I'm trying to make things better
for *everyone*.

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] OpenSSL 1.1 status, and a worked example of why you should *NEVER* rebase

[David Woodhouse]

On Tue, 2016-03-08 at 19:00 +0100, Laszlo Ersek wrote:
> It is not about the branch that linus pulls from the subsystem
> maintainer.
> 
> It is about the patches that the subsystem maintainer picks up from
> emails of individual contributors.
> 
> Let me quote Linus's email back at you:

The better part to quote, I suspect, is :

    People can (and probably should) rebase their _private_ trees 
    (their own work). That's a _cleanup_. But never other peoples 
    code. That's a "destroy history"

> But we're not talking about the handling of pull requests. We're talking
> about patches that contributors send in email. Most individual
> contributors send patches in email *only*. This covers Linux (to my
> knowledge), QEMU (definitely), libvirt (I believe), and edk2.
> 
> Is your recommendation to *require* contirbutors to email pull requests,
> alongside their patches (which is "my" requirement)? 

Absolutely not. Especially for single patches, it doesn't make sense to
*require* a pull request. It's large submissions, especially new
features which interact with other things that might be in flux, and
especially where the new submission involves a large series of commits,
where pull requests are better.

And even then, if the submitter *chooses* to send their work as email
patches instead of a pull request — despite that being not such a good
idea — it should still be accepted as such.

That's OK. That history *is* correct — the submitter chose to use
email, and in doing so their work *was* broken from day 1 (in the cases
where it goes wrong).


> Or do you recommend that contributors be *allowed* to email pull
> requests (alongside their patches), and if they do, their pull requests
> be merged correctly?

Exactly this. They should be *allowed*, and for large submissions it
should be *recommended* but not mandatory.

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] OpenSSL 1.1 status, and a worked example of why you should *NEVER* rebase

[David Woodhouse]

On Tue, 2016-03-08 at 09:30 -0800, Jordan Justen wrote:
> It sounds like the issue was a lack or gap in testing after the
> rebase.
> 
> I don't see that possibility going away just because you instead used
> merge. Especially if you consider resolving merge conflicts or other
> subtle errors that can creep in during a merge. (3a01358bdb03)

Right. The possibility of error doesn't go away. But the point is that
the history is preserved, and you can work out what happened. Instead
of ending up with a series of commits that apparently *never* worked.

> I've worked on projects that generally rebase, and still regularly
> have patchsets larger than 50 patches contributed.

I've worked on projects that don't use version control at all. Or which
use SVN. That doesn't make it a good idea.

> EDK II has been forced to live in the rebase mindset for years due to
> svn, and rebasing has not been a problem for us either.

The point in moving to git was to make things better. :)

> At the subsystem level, I think even the kernel often relies on rebase
> along with format-patch/am to bring in patches rather than merges.

As I said, don't let that get close to Linus or he'll eat you.

> So I disagree with the statement that rebase should *NEVER* be used.
> 
> For EDK II, we decided to start with a rebase model as it was a
> smaller step (conceptually) from svn. I'm not sure what happened in
> your rebase, but I think issues with this model will be no more
> frequent than issues with merges.
> 
> I don't think anyone has ruled out considering using merges once
> everyone is more comfortable with git.

Shouldn't we just switch to a proper usage model in one hit instead of
prolonging the pain?

That means fixing the CRLF thing ASAP too.

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] OpenSSL 1.1 status, and a worked example of why you should *NEVER* rebase

[David Woodhouse]

On Tue, 2016-03-08 at 18:24 +0100, Laszlo Ersek wrote:
> Here again I can only point to people who I consider my betters -- are
> you suggesting that the QEMU workflow and the Linux workflow are utterly
> wrong?

It is not "the Linux workflow". Linus will *eat* you if you rebase
trees which you ask him to pull.

See http://www.mail-archive.com/dri-devel@lists.sourceforge.net/msg39091.html

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] OpenSSL 1.1 status, and a worked example of why you should *NEVER* rebase

[David Woodhouse]

On Tue, 2016-03-08 at 14:21 +0100, Laszlo Ersek wrote:
> As soon as Intel leadership signs off on a merge-oriented workflow,
> I'll seek to adopt it immediately.

What is this "Intel leadership" of which you speak? I thought the
direction from Intel was that we would move to git (at last).

Nobody made a statement that we should *NOT* use the git tools as they
were designed to be used, and should instead do things (rebasing) that
the git authors explicitly recommend *against*... did they?

Jordan?

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] OpenSSL 1.1 status, and a worked example of why you should *NEVER* rebase

[David Woodhouse]

As of yesterday, *all* my patches had been merged into OpenSSL HEAD, in
preparation for the OpenSSL 1.1 Beta 1 release this week.

There is only one more patch outstanding in my OpenSSL git tree.

Do those statements seem self-contradictory to you? Well, they're not
quite. The patches *were* merged, but one was then reverted because it
was utterly hosed and broke the build for fairly much *everyone* except
the UEFI target. Bad dwmw2. No biscuit.

Why was it so broken? Well, it wasn't because I'm a *complete* muppet —
it was correct when I first committed it, and I *tested* it in a native
Linux build at that point too, as well as within the EDK2 build:
http://git.infradead.org/users/dwmw2/openssl.git/commitdiff/929ae044cf7

However, a subsequent change in the upstream tree affected my patch:
http://git.infradead.org/users/dwmw2/openssl.git/commitdiff/8731a4fcd#patch7

At some point I stupidly did a 'git pull --rebase' or something else
that you should never do. In rebasing, I then failed to correctly fix
up the changes, leaving it broken. I did a quick smoke-test rebuild
(under EDK2 only, it seems) of the rebased tree, but didn't spot the
error.

Now, you can point out that this involved user error on my part when I
did the rebase — but there are a number of problems with putting it
down to that alone:

Firstly, it's a *predictable* error. You are *never* paying as much
attention (to each and every commit) when you rebase, as when you
create them in the first place. A process should not introduce
*predictable* user errors.

In fact, although in *this* case it needed user error, there are cases
where the tools *will* handle the merge automatically, but the code is
still broken — you're not handling a new enum value that someone else
has added in the upstream you're rebasing onto, or something like that.
So the patch *applies* OK, but doesn't work. Or works most of the time
but has broken corner cases.

And the most important thing is that when this happens during a rebase,
the history — which is the single most important thing that a version
control system exists to preserve! — is lost. When the breakage is
discovered, there's no way to go back and see "it works  and then
the merge  was broken". You just end up with a false history in
which a given commit *never* worked.

Now, this one is a trivial (and recent) example, and it's easy enough
to sort out and even *find* the original commit in my local tree which
was subsequently discarded in the rebase. I'm using it because it was
*me* who screwed up, and I can rant at length about the *practice* of
rebasing, without making some other poor sod feel bad. We all screw up.
The point is to use the tools to help us *cope*.

Although *this* one was simple, it *does* happen that more complex
submissions also end up broken even by the time they are merged. And
when that happens, if the tools are being used *properly*, you can
*see* that it actually worked on the commit just *before* it was
merged, and it was broken in the merge. And you can look for the cause
— what changed in the upstream, between the version that the work was
based on, and the tree it was merged into.

This isn't a purely theoretical concern. I've *done* this. And I've
done it as a third party with little familiarity with the specific code
in question, a *long* time after it was merged. Because it was only a
relatively esoteric corner case that was broken, so nobody noticed —
the kind of thing you think about while you're *writing* the code, and
never again. Especially not while rebasing.

(Which is also an argument for decent unit tests, mind you, but that's
a different rant and one on which I'm even *more* hypocritical.)

So please, DO NOT REBASE submissions onto the latest master. Use the
tools properly as they were designed to be used — put any non-trivial
work into a tree and send it as a pull request. Sure, send the patches
to the list for review *too*, but that shouldn't be how they actually
get in.

Note: using 'git rebase --interactive' just to add Reviewed-by: and
similar tags is acceptable. But don't actually rebase onto a different
base. Keep it where it was, and *merge* it.

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [GIT PULL] CryptoPkg/OpensslLib: Fix CRLF breakage in process_files.sh

[David Woodhouse]

On Sat, 2016-03-05 at 20:05 +0100, Ard Biesheuvel wrote:
> On 5 March 2016 at 17:54, David Woodhouse <dw...@infradead.org> wrote:
> > Please PULL this commit from:
> >
> > git://git.infradead.org/users/dwmw2/edk2.git fix-crlf-crap
> >
> 
> Fetched and pushed, thanks!

Thanks, Ard.

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [GIT PULL] CryptoPkg/OpensslLib: Fix CRLF breakage in process_files.sh

[David Woodhouse]

Please PULL this commit from:

git://git.infradead.org/users/dwmw2/edk2.git fix-crlf-crap



From 9353c60cea6eeedbbe4b336aea02646e2bf25f47 Mon Sep 17 00:00:00 2001
From: David Woodhouse <david.woodho...@intel.com>
Date: Sat, 5 Mar 2016 16:44:33 +
Subject: [PATCH] CryptoPkg/OpensslLib: Fix CRLF breakage in process_files.sh

This got broken in committing, due to a catalogue of broken practices.

Firstly, we should *pull* git submissions, never recommit them. You
preserve the correct history then, and don't risk rebasing to result in
a history which *never* worked in the form that gets preserved.

That would have kept the authorship attrbution correct too.

Secondly, we shouldn't be storing CRLF line endings in the objects that
git stores in its database. It is designed to store simple LF line endings,
and then check that out as appropriate for the system (resulting in CRLF
in the working tree for Windows users, as they expect). That would avoid
this problem, and all the other problems we have with patches being
exchanged.

Make it executable too, which also got lost in the commit mess.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: David Woodhouse <david.woodho...@intel.com>
---
 CryptoPkg/Library/OpensslLib/process_files.sh | 194 +-
 1 file changed, 97 insertions(+), 97 deletions(-)
 mode change 100644 => 100755 CryptoPkg/Library/OpensslLib/process_files.sh


-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 2/9] Linux: Ignore *.patch and *~ files

[David Woodhouse]

On Fri, 2016-03-04 at 08:56 -0800, Lee Leahy wrote:
> Have git ignore patch files and backup files (file names ending with
> ~).
> 
> TEST=git status no longer displays these files
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Lee Leahy <leroy.p.le...@intel.com>

Can't this wait until I have finished killing off the OpenSSL patch,
which it would also ignore?

OpenSSL 1.1 should be released next month, and shouldn't need
patching... 

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2] MdeModulePkg: RegularExpressionDxe: support free(NULL)

[David Woodhouse]

On Fri, 2016-02-26 at 16:43 +, Shia, Cinnamon wrote:
> 
> #define free(p) \
>   do {  \
>     if (p != NULL) { \
>   FreePool (p);  \
>    }   \
>  } while (FALSE)

Now consider a line of code which invokes this as 

  free(myPointer++);

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] MdeModulePkg: AcpiTableDxe: fix VS2008 build by merging adjacent if blocks

[David Woodhouse]

On Thu, 2016-02-25 at 17:25 +0100, Ard Biesheuvel wrote:
> The assignment of CurrentRsdtEntry and its subsequent dereference are
> subject to the same condition, but for some reason, VS2008 does not see
> that and warns about the dereference possibly involving an uninitialized
> pointer. Since the single statememt between the blocks is unrelated, we
> can just move it and merge the two conditional blocks together.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org>

This fixes the build for me; thanks.

Reviewed-by: David Woodhouse <david.woodho...@intel.com>

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v4 1/2] MdeModulePkg: AcpiTableDxe: make 4 GB table allocation limit optional

[David Woodhouse]

On Thu, 2016-02-25 at 17:16 +0100, Ard Biesheuvel wrote:
> 
> > This appears to break the build with VS2008 for me:
> >
> > e:\edk2\mdemodulepkg\universal\acpi\acpitabledxe\acpitableprotocol.c(98
> > 4) : warning C4701: potentially uninitialized local variable
> > 'CurrentRsdtEntry' used
> > LINK : fatal error LNK1257: code generation failed
> >
> 
> Apologies. I don't have access to that toolchain

I wish I didn't have to. It would be useful to have CI running
automatically with the supported toolchains and targets — ideally on
all pull requests, but at *least* on what's committed to HEAD.

Thanks for the quick patch. I will attempt to get it into the Windows
VM and apply it there.

> > You misspelled "4 GiB" in the commit comment too, btw.
> 
> I think the context of 32-bit addressable RAM and memory allocation
> kind of makes it obvious that this is 2base not 10base.

That's a very strange response. The same "it's obvious in context"
statement could be made of fairly much *any* typo or spelling mistake
you could make. That isn't really the point though; we strive to avoid
such errors anyway.

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v4 1/2] MdeModulePkg: AcpiTableDxe: make 4 GB table allocation limit optional

[David Woodhouse]

On Tue, 2016-02-23 at 18:35 +0100, Ard Biesheuvel wrote:
> AARCH64 systems never require compatibility with legacy ACPI OSes, and
> may not have any 32-bit addressable system RAM. To support ACPI on these
> systems, we need to be able to relax the 4 GB allocation restriction.
> 
> So add a PCD PcdAcpiExposedTableVersions containing a bitmask describing
> which ACPI versions are targeted, and wire it up it up to the memory
> allocation calls in AcpiTableDxe/AcpiTableProtocol.c. I.e., if ACPI v1.0b
> is not among the supported versions, the memory allocations are not limited
> to 4 GB, and only table types that carry 64-bit addresses are emitted.
> 
> Note that this will inhibit the publishing of any tables that carry only
> 32-bit addresses, i.e., RSDPv1, RSDTv1 and RSDTv3.

This appears to break the build with VS2008 for me:

e:\edk2\mdemodulepkg\universal\acpi\acpitabledxe\acpitableprotocol.c(98
4) : warning C4701: potentially uninitialized local variable
'CurrentRsdtEntry' used
LINK : fatal error LNK1257: code generation failed


You misspelled "4 GiB" in the commit comment too, btw.

-- 
dwmw2


smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 0/4] free(NULL) and realloc(NULL, size) conformance improvements

[David Woodhouse]

On Thu, 2016-02-25 at 11:01 +0100, Laszlo Ersek wrote:
> 
> > Anyway, I've rebased my tree on top of yours,
> 
> Thanks -- I'll push the first three patches to edk2 master in a minute,
> and I'll post a new version of the fourth.

You've committed the one I need. Thanks.

> > split up the patch
> > changes into separate bisectable commits, and pushed my tree out again
> > to http://git.infradead.org/users/dwmw2/edk2.git
> 
> I skimmed your fresh master -- the way the patch evolves looks
> excellent. I guess it took a lot of effort.

Actually it wasn't that much effort in the end — I'd already *done* the
fun part, which was identifying the various changes which were all
mashed together into our EDKII_openssl patch and getting them into
suitable shape for upstream.

And the "new" patch, which we end up with, is just generated from a
series of clean commits in a git tree. I'm not *completely* insane :)

So all I really needed to do was 

  git checkout 1.0.2f
  patch -p0 < EDKII_openssl patch
  git commit -m rt3992 crypto/x509v3/ext_dat.h
  git commit -m rt3951 crypto/x509/x509_vfy.[ch]
  ...etc.

In all but one case, it really was that simple because the separate
logical changes all touched *different* files. One time I had to use
'git commit --interactive' because two logical changes touched the same
file.

Then for each logical change, it was just a case of reverting the
original version I'd just committed from the EDKII_openssl patch, and
applying the backported version of the same change from upstream.
And 'git diff 1.0.2f.. > unix2dos > EDKII_openssl.patch at each stage.

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

[David Woodhouse]

On Wed, 2016-02-24 at 18:20 +0100, Laszlo Ersek wrote:
> 
> Now, in the edk2 build, OPENSSL_free() boils down to a FreePool().
> However, *unlike* the free() function of the standard C library,
> FreePool() does *not* handle a NULL argument transparently.

Well that's just utterly batshit insane, now isn't it?

I'm amazed that didn't bite us before. If we're providing a free()
function especially for OpenSSL because the NIH principle guiding UEFI
was *so* strong that we even had to eschew even such *fundamentals* of
the C environment, then the least we can do is provide a *correct* one:

diff --git a/CryptoPkg/Library/BaseCryptLib/SysCall/BaseMemAllocation.c 
b/CryptoPkg/Library/BaseCryptLib/SysCall/BaseMemAllocation.c
index 544f072..7c7818a 100644
--- a/CryptoPkg/Library/BaseCryptLib/SysCall/BaseMemAllocation.c
+++ b/CryptoPkg/Library/BaseCryptLib/SysCall/BaseMemAllocation.c
@@ -38,5 +38,6 @@ void *realloc (void *ptr, size_t size)
 /* De-allocates or frees a memory block */
 void free (void *ptr)
 {
-  FreePool (ptr);
+  if (ptr)
+FreePool (ptr);
 }

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

[David Woodhouse]

On Wed, 2016-02-24 at 15:46 +0100, Laszlo Ersek wrote:
> On 02/24/16 13:05, David Woodhouse wrote:
> > On Tue, 2016-02-23 at 21:57 +0100, Laszlo Ersek wrote:
> 
> > > First of all, I built it for:
> > > - OvmfPkg/OvmfIa32.dsc
> > > - OvmfPkg/OvmfIa32X64.dsc
> > > - OvmfPkg/OvmfX64.dsc
> > > - ArmVirtPkg/ArmVirtQemu.dsc (AARCH64 architecture)
> > > 
> > > The builds complete for the first three DSC files, but it fails
> > > for the last one:
> > > 
> > > > .../Build/ArmVirtQemu-
> > > > AARCH64/DEBUG_GCC48/AARCH64/CryptoPkg/Library/OpensslLib/Openss
> > > > lLib/OUTPUT/OpensslLib.lib(poly1305.obj): In function
> > > > `poly1305_blocks':
> > > > .../CryptoPkg/Library/OpensslLib/openssl/crypto/poly1305/poly13
> > > > 05.c:194: undefined reference to `__multi3'
> > > > .../CryptoPkg/Library/OpensslLib/openssl/crypto/poly1305/poly13
> > > > 05.c:195: undefined reference to `__multi3'
> > > > .../CryptoPkg/Library/OpensslLib/openssl/crypto/poly1305/poly13
> > > > 05.c:196: undefined reference to `__multi3'
> > > > .../CryptoPkg/Library/OpensslLib/openssl/crypto/poly1305/poly13
> > > > 05.c:197: undefined reference to `__multi3'
> > 
> > That's easily fixed by adding no-poly1305 to my process_files.sh
> > script. I've pushed a new version to my edk2.git tree with that
> > change.
> 
> Yes, with your master branch @ 55eb3465c077, the ArmVirtQemu build
> completes as well. Thanks.

Excellent; thanks for confirming that.

> (By the way, do you think that your series should revert, or at least
> customize, the following commit:
> 
> commit ffbb1b25402c38d25cbbfb059139e76900bdc843
> Author: Ard Biesheuvel <ard.biesheu...@linaro.org>
> Date:   Tue Jun 16 15:09:19 2015 +
> 
> CryptoPkg: add .gitignore for OpenSSL source files

Yes, it should. Thanks for spotting that. I'll roll that into the next
series when I respin it... hopefully after finding out why it doesn't
*work*.

In fact they might be related. If you're using the headers which were
previously copied into CryptoPkg/Include/openssl because that's on the
include path before CryptoPkg/Library/OpensslLib/openssl-1.0.2f/include
then that might explain some weird breakage.

Perhaps my updated Install.sh should *delete* those stale copied
headers if they exist.

> Interestingly, the failure reproduces even when I build OVMF at your
> commit a35e4359d; with identical symptoms.

Also useful to know; thanks.

> Please find the repro steps below. (Sorry that it took so long to write
> them up, but generally I use libvirt, and I had to create these
> instructions on the spot, and I also tested them.)

I'm happy enough using libvirt, FWIW. I'm running on Fedora 23 — which
means I have a reasonably recent qemu. And if it wasn't for the poxy
FAT driver and its broken licence, I assume I'd even have a fully
functional OVMF package supplied as part of the distribution, Secure
Boot and all. :(

I'll start working through your instructions. Thanks!

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

[David Woodhouse]

On Tue, 2016-02-23 at 21:57 +0100, Laszlo Ersek wrote:
> 
> I'm testing David's patches from his repo referenced above, master branch, 
> commits
> 
>  1  81009e3cff24 CryptoPkg: Use OpenSSL include directory directly
>  2  8a40ff734a1e CryptoPkg/OpensslLib: Include complete copy of 
> opensslconf.h
>  3  d8b5c31bed60 CryptoPkg/OpensslLib: Update OpenSSL patch
>  4  b68dc8e0bb53 CryptoPkg/OpensslLib: Automatically configure OpenSSL 
> and generate file list
>  5  61e047fb19dd CryptoPkg: Support building with OpenSSL HEAD 
> (1.1.0-devel)
>  6  1e89cb2399ba CryptoPkg: Abuse internal headers to make OpenSSL HEAD 
> build work
> 
> First of all, I built it for:
> - OvmfPkg/OvmfIa32.dsc
> - OvmfPkg/OvmfIa32X64.dsc
> - OvmfPkg/OvmfX64.dsc
> - ArmVirtPkg/ArmVirtQemu.dsc (AARCH64 architecture)
> 
> The builds complete for the first three DSC files, but it fails for the last 
> one:
> 
> > .../Build/ArmVirtQemu-AARCH64/DEBUG_GCC48/AARCH64/CryptoPkg/Library/OpensslLib/OpensslLib/OUTPUT/OpensslLib.lib(poly1305.obj):
> >  In function `poly1305_blocks':
> > .../CryptoPkg/Library/OpensslLib/openssl/crypto/poly1305/poly1305.c:194: 
> > undefined reference to `__multi3'
> > .../CryptoPkg/Library/OpensslLib/openssl/crypto/poly1305/poly1305.c:195: 
> > undefined reference to `__multi3'
> > .../CryptoPkg/Library/OpensslLib/openssl/crypto/poly1305/poly1305.c:196: 
> > undefined reference to `__multi3'
> > .../CryptoPkg/Library/OpensslLib/openssl/crypto/poly1305/poly1305.c:197: 
> > undefined reference to `__multi3'

That's easily fixed by adding no-poly1305 to my process_files.sh
script. I've pushed a new version to my edk2.git tree with that change.

I've also updated the 1.0.2f-based patch, now that the PKCS7_verify()
regression has actually been fixed upstream (for 1.0.2g).

We have now merged *everything* from our EDKII_openssl-1.0.2f patch
into upstream OpenSSL HEAD, and our own patch can be *entirely*
represented as backports of existing commits from 1.1.

> Anyway, for runtime testing, I used the OvmfIa32X64 build:
> 
> > (1a) Enroll keys, and confirm SB being active in the Fedora guest,
> >  using my current build.
> > (1b) Rebuild the firmware binary with your patches & instructions. Do
> >  not touch the VM's varstore.
> > (1c) Confirm SB is still active in the Fedora guest.
> 
> This step failed, with the OVMF debug output ending with:
> 
> > Booting Fedora
> > FSOpen: Open '\EFI\fedora\shim.efi' Success
> > 
> > ASSERT_EFI_ERROR (Status = Invalid Parameter)
> > ASSERT 
> > .../MdePkg/Library/UefiMemoryAllocationLib/MemoryAllocationLib.c(819): 
> > !EFI_ERROR (Status)
> 
> I didn't continue testing after this point.

OK, thanks very much for testing this.

It sounds like there's a new issue in OpenSSL HEAD that needs fixing,
and I'm going to need to reproduce that myself to see what's going on.

Would you mind talking me through the setup above, please? To enroll
keys, I assume I need to start with a version of qemu that can support
running OVMF from a writeable flash chip, so it can store NV vars? 

Also, if you could test just the antepenultimate commit a35e4359d in
what I've just pushed — which is all the build system improvements but
still using OpenSSL 1.0.2f — that would also be very much appreciated.

Thanks again!

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

[David Woodhouse]

On Fri, 2016-02-19 at 14:26 +0100, Laszlo Ersek wrote:
> 
> In file included from
> .../CryptoPkg/Library/OpensslLib/openssl/crypto/bn/bn_add.c:59:0:
> .../CryptoPkg/Library/OpensslLib/openssl/crypto/bn/bn_lcl.h:114:31:
> fatal error: internal/bn_conf.h: No such file or directory
>  # include "internal/bn_conf.h"
>    ^

Hm, that's a new autogenerated file. Screw it, please run
./process_files.sh in the OpensslLib directory and I believe that'll be
created for you. I'll work out the alternative build procedure with a
*clean* tree in the meantime...

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

[David Woodhouse]

On Fri, 2016-02-19 at 10:43 +0100, Laszlo Ersek wrote:
> 
> I can test this for you, if you give me precise instructions.
> 
> (I'm asking for instructions because CryptoPkg/Include/openssl/README is
> deleted in one of the early patches.)

It moved from Patch-HOWTO.txt to OpenSSL-HOWTO.txt since there was no
longer a patch involved. Or did I forget to add the new file?


 Introduction

  OpenSSL is a well-known open source implementation of SSL and TLS protocols.
The core library implements the basic cryptographic functions and provides 
various
utility functions. The OpenSSL library is widely used in variety of security
products development as base crypto provider. (See http://www.openssl.org/ for 
more
information on OpenSSL).
  UEFI (Unified Extensible Firmware Interface) is a specification detailing the
interfaces between OS and platform firmware. Several security features were
introduced (e.g. Authenticated Variable Service, Driver Signing, etc) from UEFI
2.2 (http://www.uefi.org/). These security features highly depend on the
cryptography. This HOWTO documents OpenSSL building under UEFI environment.



OpenSSL-Version

  EDKII supports building with the master branch of OpenSSL, which will
  eventually be released as OpenSSL 1.1.0. In additional, fixes for the
  following OpenSSL "Request Tracker" tickets are required:

   RT4175: Fix regression using PKCS7_verify() with Authenticode
   RT4309: Define PRIu64 for UEFI build

  A clone of the OpenSSL git repository with all the required fixes is
  available at git://git.infradead.org/users/dwmw2/openssl.git



  HOW to Install Openssl for UEFI Building


1. Clone the above-referenced git repository into the directory
  CryptoPkg/Library/OpensslLib/openssl/

2. Copy the opensslconf.h file from this directory into the newly-created
   CryptoPkg/Library/OpensslLib/openssl/include/openssl directory next to
   the other OpenSSL include files.

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

[David Woodhouse]

On Fri, 2016-02-19 at 09:19 +, Long, Qin wrote:
> 
> //nod
> Some old include / structure definitions were put there just to satisfy the 
> compiler in early phase, which looks like one rough style to make it work. 
> It's
> really the time to have one clean-up for our "out-of-the-box" integration.

Yeah, I might actually throw the whole lot away and start again, adding
only what's needed (and only then when I can't make OpenSSL *stop*
needing it)...

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

[David Woodhouse]

On Fri, 2016-02-19 at 07:59 +, Long, Qin wrote:
> I agree those changes really make sense for better alignment, under
> both 1.0.2xx and 1.1 HEAD. The final out-of-box support will be
> wonderful. 
> The updates (http://git.infradead.org/users/dwmw2/edk2.git) looks
> good to me. And I will follow more validations, and start the next
> integration step-by-step. 

Great, thanks. I should note that, as before, I haven't actually done
any real *testing* of this lot. Only build testing under Linux. I think
I did manage to create a Windows VM with a build environment for EDK2
at one point; I should at least dust that off.

> Yeah, also will do more follow-ups about the remaining opens...

Like using the OpenSSL TS support instead of our own? That would be
good. Likewise I think I remember a vague plan of making it possible to
disable the OCSP code, since we don't use it?

Note that the OpenSSL 1.1 Beta 1 release is scheduled in "about a
month"¹, and that is the feature/API freeze deadline. Anything we want
added (other than bug fixes, of course), needs to be in by then.

In the fullness of time, I would *also* like to clean up the litter of
include files we provide in CryptoPkg/Include purely for the benefit of
OpenSSL — removing stuff from OpenSslSupport.h as we go. If we really
want to stop OpenSSL from including/requiring those headers then that
makes sense to do before Beta 1 too.

I might start by looking for things which can be considered "obviously"
bugs — like including anything in netinet/ when configured with no-sock 
for example. And I really don't see why it needs syslog.h for the UEFI
build, or dirent.h for a no-stdio (which really means no file access)
build.

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation


¹ https://www.mail-archive.com/openssl-dev@openssl.org/msg42723.html



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

[David Woodhouse]

On Thu, 2016-02-18 at 17:36 +, Long, Qin wrote:
> I think I also need to apologize, that it's my decision to pending
> some part of Dave's patch series posted in last year (with simple
> sync-up with Dave), which includes some changes for include path,
> build process, etc.

I agreed with that then, and in retrospect it looks like the right
decision. It turns out that from 1.0.2e onwards, the symlinks are no
longer present in the include/openssl/ directory of the release
tarballs. So if you had actually merged my patch to use the OpenSSL
include directory, back in the 1.0.2d days, it would have broken with
the update to 1.0.2e.

I've revamped that patch so that we retain Install.sh even on POSIX
platforms. Basically we just copy the files to where OpenSSL normally
has them, instead of copying them to our *own* CryptoPkg/Include/
directory.

I've also rebuilt my OpenSSL_1_0_2-stable branch at
http://git.infradead.org/users/dwmw2/openssl.git with freshly cherry-
picked patches from HEAD... now that fairly much every change we had
*is* committed to OpenSSL HEAD.

My main motivation for doing this right now is because we need to
ensure that OpenSSL 1.1, when it comes out, *does* do everything we
need out of the box.

I won't send another patchbomb to the list right now, but I've updated
the tree at http://git.infradead.org/users/dwmw2/edk2.git

Again, up to and including the 'Automatically configure OpenSSL and
generate file list' patch are applicable even while we stick with
1.0.2. The final two commits need more work (and I'm hoping you follow
through on the discussion about the HMAC APIs), but are mostly useful
for ensuring that OpenSSL HEAD *stays* working as it approaches
release.

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

[David Woodhouse]

On Thu, 2016-02-18 at 14:27 +, David Woodhouse wrote:
> On Thu, 2016-02-18 at 14:58 +0100, Laszlo Ersek wrote:
> > 
> > Then I gave my R-b to this patch, admitting that I couldn't verify the
> > edk2-only customizations against the 1.0.2f release.
> > 
> > Turns out those customizations are indeed no longer correct, so my R-b
> > was in error.
> 
> Er, aren't they? Are you referring to my comment about the RT#3955 fix?
> 
> That isn't actually *less* correct than it ever was.

And, embarrassingly, it looks like OpenSSL 1.1 has a fix which looks
much more like the one in EDKII_openssl-1.0.2f.patch, than the one in
my own tree.

Is there room for me to come and crawl under that rock *with* you...?

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

[David Woodhouse]

On Thu, 2016-02-18 at 14:58 +0100, Laszlo Ersek wrote:
> 
> Then I gave my R-b to this patch, admitting that I couldn't verify the
> edk2-only customizations against the 1.0.2f release.
> 
> Turns out those customizations are indeed no longer correct, so my R-b
> was in error.

Er, aren't they? Are you referring to my comment about the RT#3955 fix?

That isn't actually *less* correct than it ever was.

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

[David Woodhouse]

On Thu, 2016-02-18 at 14:43 +0100, Laszlo Ersek wrote:
> 
> > Actually, in practice is *isn't* so bad. Given that we don't update our
> > EDKII_openssl-1.0.2X.patch very often (if at all) between OpenSSL
> > releases, what happens is that your build tree ends up with *multiple*
> > CryptoPkg/Library/OpensslLib/openssl-1.0.2[def] directories.
> 
> Interesting! The OPENSSL_PATH define in
> "CryptoPkg/Library/OpensslLib/OpensslLib.inf" would change, and the
> untracked files under the separate openssl-* directories would co-exist.
> I'm not sure about the untracked files created by Install.sh though --
> they come from the different openssl source trees, but go into a common
> location. Maybe Install.sh should be reexecuted at each step of the
> bisection.

Ah, good point. But I *killed* Install.sh, even for the 1.0.2 build,
many months ago. It temporarily escaped my notice that that fix wasn't
actually merged into EDK2 upstream yet. It's one of the set I sent
yesterday. Once that's done, *then* what I said is true because we
actually use the OpenSSL headers from within the OpenSSL source tree,
and don't stupidly *copy* them to our own Include/ directory.

> Do you think it will be possible to add openssl as a git submodule to
> the upstream edk2 repo? (I'd rather not explore that myself down-stream...)

I don't see why not. But let's concentrate on getting the even *more*
basic and obvious "you are completely insane if you aren't using git
properly" things fixed first... like using LF in the stored files.

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

[David Woodhouse]

On Thu, 2016-02-18 at 14:12 +0100, Laszlo Ersek wrote:
> 
> > And even if we've missed the chance to do it "in retrospect" for
> > historical commits in our canonical git repository, we could still make
> > a commit now which *changes* the line endings to what git expects, and
> > quite soon this whole nonsense would be a thing of the past.
> 
> Yes. I think that would take a commit that converted everything to
> LF-only. Then everybody on Windows would have to toggle some git knobs,
> correct? Would that be safe when moving between commits?

I'm not even sure if they'd have to toggle anything. Doesn't git
convert automatically if it finds LF in the files it checks out, on
Windows? And if things do get checked out with LF-only line endings on
Windows, isn't that *also* harmless?

I have fairly much reached my masochism quota for this week though; I'm
not about to attempt to boot a Windows machine and check :)

Either way, such a change will be *very* quickly lost in the dim and
distant past — rather than being a constant problem and a barrier to
contributions.

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

[David Woodhouse]

On Thu, 2016-02-18 at 13:14 +0100, Laszlo Ersek wrote:
> This patch seems right.

Please trim citations. You didn't have to cite the *entire* patch just
to say that it seems right!

You aren't one of the Outlook-afflicted. We need you to set an example
of how email is actually *supposed* to be used :)

> I also tried to diff the "openssl-1.0.2e" and "openssl-1.0.2f" trees
> against each other, to see if "no new source changes [...] introduced
> for 1.0.2f enabling" is indeed the right thing to do.
> 
> The result of that diffing is a 3000 line patch, with the following
> diffstat:
> 
>  144 files changed, 815 insertions(+), 476 deletions(-)

This is the 21st century. Nobody in their right mind deals with
software management except through git. More edifying would be to run:

 git log OpenSSL_1_0_2e..OpenSSL_1_0_2f  --stat

and/or

 gitk OpenSSL_1_0_2e..OpenSSL_1_0_2f

There are 60 commits. Browsing through them with gitk, they do mostly
seem like sane, simple bug fixes. Some are cosmetic but basically risk-
free, such as the example you gave — that commit touched 74 files, but
*only* in the comments, with fairly much zero chance of causing
problems.

> While I'm absolutely no crypto expert, I have a hard time believing
> that fixing the two major issues listed by the release notes required
> touching *144 files*!

There were plenty more issues fixed other than the "headline" issues.

> So, I hereby declare the OpenSSL updates practically un-review-able,
> even just for *scope* -- i.e., in order to see how far those changes
> extend. I also claim that the OpenSSL release strategy is not being
> implemented in reality -- the "letter releases" actually seem to be
> vulnerability-triggered *snapshots* of the 1.0.2 tree, where the code
> influx, albeit low volume, definitely meanders outside of bug and
> security fixes.

Yeah, there's a certain amount of truth in what you say. When a high-
priority vulnerability comes up, we need a new release and people need
to upgrade. If we fix a minor functionality issue which is fairly
esoteric and *doesn't* have security implications, there *isn't* an
immediate release the same day. We'd run out of letters very quickly if
we did that! :)

So yes, when the high-visibility issues trigger a release, a bunch of
less important fixes go along for the ride. I don't think that's a
problem.

But trust me — as someone who has occasionally wanted to get
improvements into a stable branch of OpenSSL when they have been
considered "new features" — when I say that they aren't adding whole
piles of exciting new stuff to the 1.0.2 branch :)

Take a look at the discussion about fixing ECDH for engines, on the
openssl-dev list at the moment. It would be fairly trivial to expose
what's necessary there, but because it's a new feature (although some
would want to consider a bug fix), the OpenSSL team really don't want
to do it.

> Now, one might ask why I care. I care because for some downstreams of
> edk2 at least, the situation that openssl has to be patched in before a
> secure boot enabled build is completely unacceptable. That makes it
> super-unwieldy to bisect a secure boot enabled build for example. It
> also requires all people who clone your tree to patch in OpenSSL
> manually.

Actually, in practice is *isn't* so bad. Given that we don't update our
EDKII_openssl-1.0.2X.patch very often (if at all) between OpenSSL
releases, what happens is that your build tree ends up with *multiple*
CryptoPkg/Library/OpensslLib/openssl-1.0.2[def] directories. And as you
flit back and forth between EDK2 commits in your bisect, you use the
different OpenSSL directories, as appropriate.

> Importing openssl should be a run-of-the-mill *commit* in the git
> history (and it is, for us).

Surely this is what git submodules are for?

> Then you can understand why I care about actual OpenSSL differences --
> because when the OpenSSL addition is an actual commit in your repo, the
> upgrade looks like this:
> 
> - revert the commit that captured the execution of the previous
>   Patch-HOWTO.txt
> - perform the current Patch-HOWTO.txt, commit the results as a new
>   commit
> - *squash* these two commits (unless you are happy with two, several MB
>   long patches -- good luck to your mailing list!)
> - now you have an incremental patch in your history that takes you from
>   1.0.2e to 1.0.2f
> 
> Except, of course, that patch is fully unreviewable -- it is
> no better than a binary-only code drop.

Again, not if it's done properly as a git submodule.

> Honestly, edk2 should either incorporate OpenSSL permanently, or build
> it 100% from an external, unmodified upstream tarball (I think this is
> what David has been working on, right?)

As noted yesterday, we're two trivial patches from being able to use
the next

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

[David Woodhouse]

On Thu, 2016-02-18 at 13:40 +0100, Laszlo Ersek wrote:
> > We should not be carrying patches which *differ* from the fixes that
> > went into OpenSSL upstream.
> 
> So yeah, I guess the recent irrelevance of this edk2-only hunk should
> have been "obvious" from the 3000-line diff between OpenSSL 1.0.2e and
> 1.0.2f -- a diff that no person not involved in day-to-day OpenSSL
> development can review.

Actually, that isn't fixed in OpenSSL 1.0.2f; it's fixed only in 1.1
and I've backported the fix.

> > Can we *please* keep native line endings in the git tree
>
> No, we can't. This issue has been discussed with Junio C Hamano, the git
> maintainer. Git can convert text files to platform-native line endings
> on checkout, but only if the internal (object) representation of the
> text files is LF only.
> 
> And, because edk2 started out with SVN, and the (now primary) git repo
> started out as a mirror of the SVN repo, the internal representation is
> CRLF, not LF. So git's auto-conversion doesn't apply.

Apologies, I misspoke. I didn't mean "native line endings in the git
tree", which is obviously impossible — I meant "sane line endings in
the git tree", which means LF — so git's auto-conversion *would* apply.

> Everyone else did not start with a git mirror of an SVN repo of text
> files that were created and edited exclusively on Windows. :(

A poor excuse. It wasn't hard to fix up the SVN->git mirror, and we
knew we needed to do this *long* before git actually became the
"primary" system.

And even if we've missed the chance to do it "in retrospect" for
historical commits in our canonical git repository, we could still make
a commit now which *changes* the line endings to what git expects, and
quite soon this whole nonsense would be a thing of the past.

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

[David Woodhouse]

On Thu, 2016-02-18 at 00:33 +0800, Qin Long wrote:
> 
>  crypto/pkcs7/pk7_smime.c   Thu Jun 11 21:01:06 2015
> -+++ crypto/pkcs7/pk7_smime.c   Fri Jun 12 11:23:38 2015
> +--- crypto/pkcs7/pk7_smime.c   Thu Jan 28 21:56:08 2016
>  crypto/pkcs7/pk7_smime.c   Wed Feb 17 16:22:45 2016
>  @@ -254,7 +254,8 @@
>   STACK_OF(PKCS7_SIGNER_INFO) *sinfos;
>   PKCS7_SIGNER_INFO *si;
> @@ -114,20 +114,19 @@ diff U3 crypto/pkcs7/pk7_smime.c
> crypto/pkcs7/pk7_smime.c
>   if (i <= 0)
>   break;
>   if (tmpout)
> -@@ -394,6 +394,10 @@
> +@@ -394,6 +394,9 @@
>   }
>   BIO_free_all(p7bio);
>   sk_X509_free(signers);
> -+
>  +    if (buf != NULL) {
> -+  OPENSSL_free(buf);
> ++    OPENSSL_free(buf);
>  +    }
>   return ret;
>   }
>  

This bit of code addresses OpenSSL RT#3955, although you don't actually
*mention* that fact anywhere. A different fix has been committed to
OpenSSL to close that RT.

We should not be carrying patches which *differ* from the fixes that
went into OpenSSL upstream.

That's why part of my patch series (qv) actually *replaces* this whole
EDKII_openssl-1.0.2X.patch with a cleanly generated one from a 1.0.2-
based git tree, *with* its full changelog:

http://git.infradead.org/users/dwmw2/edk2.git/commitdiff/cf8dd4aee409

I mention this just to reinforce the need for that change, even before
we make the switch to OpenSSL 1.1.

FWIW I was unable to apply the patch from your email; if there was ever
a trick to managing the bogus line endings, I've forgotten it. Can we
*please* keep native line endings in the git tree and let it be checked
out into the native form — like everyone else does?

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation





smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

[David Woodhouse]

On Thu, 2016-02-18 at 09:50 +0100, Laszlo Ersek wrote:
> On 02/18/16 09:44, Long, Qin wrote:
> > Thanks for raising this, Laszlo.
> > 
> > Exactly, the posted patch series from David also included one
> 1.0.2f enabling. The patch series will bring one direct / smooth
> supports for EDKII-CryptoPkg with some patch integration in both
> EDKII and OpenSSL sides, and also introduce some source generation
> mechanism for more native build support. 
> > 
> > I will work on more validations based on David's post, and also
> work with David on other possible updates (e.g. include file issue).
> This may need some extra times.
> > 
> > Before all patches were integrated, my plan is to have one 1.0.2f
> upgrade firstly based on my last patch, which will not change any
> build process, and just to catch the latest release for some
> requirements. 
> > 
> > (David, apology for my late feedback to your patch post.)
> > 
> > Let me know if any concerns. 
> 
> Works for me if it works for David.

Yeah, that's fine. I'm happy to rebase my tree and put the upgrade to
1.0.2f first.

Although I *did* like having "it's easy now..." as the commit comment.

That exercise has highlighted one more potential improvement — the
upgrade from 1.0.2e to 1.0.2f did require changing about 18 instances
of the string "1.0.2e" to "1.0.2f". I'll see if I can cut that down.

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH 7/7] CryptoPkg: Abuse internal headers to make OpenSSL HEAD build work

[David Woodhouse]

More stuff got hidden. Some of this is tolerable. Other bits are
horrid, but given that we expose *requires* that we know the size
of the data structure, it's hard to see how we can avoid it.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: David Woodhouse <david.woodho...@intel.com>
---
 CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c  | 7 ---
 CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c | 6 --
 CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Sign.c  | 1 +
 CryptoPkg/Library/BaseCryptLib/Pk/CryptTs.c | 2 ++
 CryptoPkg/Library/BaseCryptLib/Pk/CryptX509.c   | 1 +
 5 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c 
b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c
index 693cd32..93c2bcb 100644
--- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c
+++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c
@@ -14,7 +14,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 
 #include "InternalCryptLib.h"
 #include 
-
+#include <../hmac/hmac_lcl.h>
 /**
   Retrieves the size, in bytes, of the context buffer required for HMAC-MD5 
operations.
 
@@ -65,7 +65,8 @@ HmacMd5Init (
   //
   // OpenSSL HMAC-MD5 Context Initialization
   //
-  HMAC_CTX_init (HmacMd5Context);
+  memset(HmacMd5Context, 0, sizeof(HMAC_CTX));
+  HMAC_CTX_reset (HmacMd5Context);
   HMAC_Init_ex (HmacMd5Context, Key, (UINT32) KeySize, EVP_md5(), NULL);
 
   return TRUE;
@@ -191,7 +192,7 @@ HmacMd5Final (
   // OpenSSL HMAC-MD5 digest finalization
   //
   HMAC_Final (HmacMd5Context, HmacValue, );
-  HMAC_CTX_cleanup (HmacMd5Context);
+  HMAC_CTX_reset (HmacMd5Context);
 
   return TRUE;
 }
diff --git a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c 
b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c
index 881d26c..5710f26 100644
--- a/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c
+++ b/CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c
@@ -14,6 +14,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 
 #include "InternalCryptLib.h"
 #include 
+#include <../hmac/hmac_lcl.h>
 
 /**
   Retrieves the size, in bytes, of the context buffer required for HMAC-SHA1 
operations.
@@ -65,7 +66,8 @@ HmacSha1Init (
   //
   // OpenSSL HMAC-SHA1 Context Initialization
   //
-  HMAC_CTX_init (HmacSha1Context);
+  memset(HmacSha1Context, 0, sizeof(HMAC_CTX));
+  HMAC_CTX_reset (HmacSha1Context);
   HMAC_Init_ex (HmacSha1Context, Key, (UINT32) KeySize, EVP_sha1(), NULL);
 
   return TRUE;
@@ -191,7 +193,7 @@ HmacSha1Final (
   // OpenSSL HMAC-SHA1 digest finalization
   //
   HMAC_Final (HmacSha1Context, HmacValue, );
-  HMAC_CTX_cleanup (HmacSha1Context);
+  HMAC_CTX_reset (HmacSha1Context);
 
   return TRUE;
 }
diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Sign.c 
b/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Sign.c
index 704eb4e..8e0d896 100644
--- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Sign.c
+++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Sign.c
@@ -17,6 +17,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 #include 
 #include 
 #include 
+#include 
 
 
 /**
diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptTs.c 
b/CryptoPkg/Library/BaseCryptLib/Pk/CryptTs.c
index d495812..c6799ae 100644
--- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptTs.c
+++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptTs.c
@@ -23,6 +23,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 #include 
 #include 
 #include 
+#include <../evp/evp_locl.h>
 
 //
 // OID ASN.1 Value for SPC_RFC3161_OBJID ("1.3.6.1.4.1.311.3.3.1")
@@ -285,6 +286,7 @@ CheckTSTInfo (
   if (HashedMsg == NULL) {
 goto _Exit;
   }
+  memset(, 0, sizeof(MdCtx));
   EVP_DigestInit (, Md);
   EVP_DigestUpdate (, TimestampedData, DataSize);
   EVP_DigestFinal (, HashedMsg, NULL);
diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptX509.c 
b/CryptoPkg/Library/BaseCryptLib/Pk/CryptX509.c
index 7dc4596..d392bed 100644
--- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptX509.c
+++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptX509.c
@@ -15,6 +15,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 #include "InternalCryptLib.h"
 #include 
 #include 
+#include 
 
 /**
   Construct a X509 object from DER-encoded certificate data.
-- 
2.5.0

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation


smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH 2/7] CryptoPkg/OpensslLib: Include complete copy of opensslconf.h

[David Woodhouse]

This can be an auto-generated file, and it *isn't* in the OpenSSL git tree;
it's only in the generated tarballs. So rather than including it in our
OpenSSL patch, just have the user copy it into place.

This makes it easier to manage changes, and is a step towards better
integration.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: David Woodhouse <david.woodho...@intel.com>
---
 CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2e.patch | 323 -
 CryptoPkg/Library/OpensslLib/Patch-HOWTO.txt|   6 +-
 CryptoPkg/Library/OpensslLib/opensslconf.h  | 497 

 3 files changed, 501 insertions(+), 325 deletions(-)

diff --git a/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2e.patch 
b/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2e.patch
index e4eaff6..65d61e2 100644
--- a/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2e.patch
+++ b/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2e.patch
@@ -352,329 +352,6 @@ diff U3 crypto/crypto.h crypto/crypto.h
  "Low level API call to cipher " #alg " forbidden in FIPS 
mode!")
  
  # else
-diff U3 crypto/opensslconf.h crypto/opensslconf.h
 crypto/opensslconf.h   Thu Jun 11 21:55:38 2015
-+++ crypto/opensslconf.h   Fri Jun 12 10:28:27 2015
-@@ -5,15 +5,72 @@
- extern "C" {
- #endif
- /* OpenSSL was configured with the following options: */
-+#ifndef OPENSSL_SYSNAME_UEFI
-+# define OPENSSL_SYSNAME_UEFI
-+#endif
- #ifndef OPENSSL_DOING_MAKEDEPEND
- 
- 
-+#ifndef OPENSSL_NO_BF
-+# define OPENSSL_NO_BF
-+#endif
-+#ifndef OPENSSL_NO_CAMELLIA
-+# define OPENSSL_NO_CAMELLIA
-+#endif
-+#ifndef OPENSSL_NO_CAPIENG
-+# define OPENSSL_NO_CAPIENG
-+#endif
-+#ifndef OPENSSL_NO_CAST
-+# define OPENSSL_NO_CAST
-+#endif
-+#ifndef OPENSSL_NO_CMS
-+# define OPENSSL_NO_CMS
-+#endif
-+#ifndef OPENSSL_NO_DEPRECATED
-+# define OPENSSL_NO_DEPRECATED
-+#endif
-+#ifndef OPENSSL_NO_DGRAM
-+# define OPENSSL_NO_DGRAM
-+#endif
-+#ifndef OPENSSL_NO_DSA
-+# define OPENSSL_NO_DSA
-+#endif
-+#ifndef OPENSSL_NO_DYNAMIC_ENGINE
-+# define OPENSSL_NO_DYNAMIC_ENGINE
-+#endif
-+#ifndef OPENSSL_NO_EC
-+# define OPENSSL_NO_EC
-+#endif
- #ifndef OPENSSL_NO_EC_NISTP_64_GCC_128
- # define OPENSSL_NO_EC_NISTP_64_GCC_128
- #endif
-+#ifndef OPENSSL_NO_ECDH
-+# define OPENSSL_NO_ECDH
-+#endif
-+#ifndef OPENSSL_NO_ECDSA
-+# define OPENSSL_NO_ECDSA
-+#endif
-+#ifndef OPENSSL_NO_ENGINE
-+# define OPENSSL_NO_ENGINE
-+#endif
-+#ifndef OPENSSL_NO_ENGINES
-+# define OPENSSL_NO_ENGINES
-+#endif
-+#ifndef OPENSSL_NO_FILENAMES
-+# define OPENSSL_NO_FILENAMES
-+#endif
-+#ifndef OPENSSL_NO_FP_API
-+# define OPENSSL_NO_FP_API
-+#endif
- #ifndef OPENSSL_NO_GMP
- # define OPENSSL_NO_GMP
- #endif
-+#ifndef OPENSSL_NO_GOST
-+# define OPENSSL_NO_GOST
-+#endif
-+#ifndef OPENSSL_NO_IDEA
-+# define OPENSSL_NO_IDEA
-+#endif
- #ifndef OPENSSL_NO_JPAKE
- # define OPENSSL_NO_JPAKE
- #endif
-@@ -23,30 +80,90 @@
- #ifndef OPENSSL_NO_LIBUNBOUND
- # define OPENSSL_NO_LIBUNBOUND
- #endif
-+#ifndef OPENSSL_NO_LOCKING
-+# define OPENSSL_NO_LOCKING
-+#endif
- #ifndef OPENSSL_NO_MD2
- # define OPENSSL_NO_MD2
- #endif
-+#ifndef OPENSSL_NO_MDC2
-+# define OPENSSL_NO_MDC2
-+#endif
-+#ifndef OPENSSL_NO_POSIX_IO
-+# define OPENSSL_NO_POSIX_IO
-+#endif
-+#ifndef OPENSSL_NO_RC2
-+# define OPENSSL_NO_RC2
-+#endif
- #ifndef OPENSSL_NO_RC5
- # define OPENSSL_NO_RC5
- #endif
-+#ifndef OPENSSL_NO_RCS
-+# define OPENSSL_NO_RCS
-+#endif
- #ifndef OPENSSL_NO_RFC3779
- # define OPENSSL_NO_RFC3779
- #endif
-+#ifndef OPENSSL_NO_RIPEMD
-+# define OPENSSL_NO_RIPEMD
-+#endif
-+#ifndef OPENSSL_NO_SCRYPT
-+# define OPENSSL_NO_SCRYPT
-+#endif
-+#ifndef OPENSSL_NO_SCT
-+# define OPENSSL_NO_SCT
-+#endif
- #ifndef OPENSSL_NO_SCTP
- # define OPENSSL_NO_SCTP
- #endif
-+#ifndef OPENSSL_NO_SEED
-+# define OPENSSL_NO_SEED
-+#endif
-+#ifndef OPENSSL_NO_SHA0
-+# define OPENSSL_NO_SHA0
-+#endif
-+#ifndef OPENSSL_NO_SOCK
-+# define OPENSSL_NO_SOCK
-+#endif
-+#ifndef OPENSSL_NO_SRP
-+# define OPENSSL_NO_SRP
-+#endif
- #ifndef OPENSSL_NO_SSL_TRACE
- # define OPENSSL_NO_SSL_TRACE
- #endif
-+#ifndef OPENSSL_NO_SSL2
-+# define OPENSSL_NO_SSL2
-+#endif
-+#ifndef OPENSSL_NO_SSL3
-+# define OPENSSL_NO_SSL3
-+#endif
-+#ifndef OPENSSL_NO_STDIO
-+# define OPENSSL_NO_STDIO
-+#endif
- #ifndef OPENSSL_NO_STORE
- # define OPENSSL_NO_STORE
- #endif
-+#ifndef OPENSSL_NO_UI
-+# define OPENSSL_NO_UI
-+#endif
- #ifndef OPENSSL_NO_UNIT_TEST
- # define OPENSSL_NO_UNIT_TEST
- #endif
-+#ifndef OPENSSL_NO_WHIRLPOOL
-+# define OPENSSL_NO_WHIRLPOOL
-+#endif
- 
- #endif /* OPENSSL_DOING_MAKEDEPEND */
- 
-+#ifndef OPENSSL_NO_ASM
-+# define OPENSSL_NO_ASM
-+#endif
-+#ifndef OPENSSL_NO_ERR
-+# define OPENSSL_NO_ERR
-+#endif
-+#ifndef OPENSSL_NO_HW
-+# define OPENSSL_NO_HW
-+#endif
- #ifndef OPENSSL_NO_DYNAMIC_ENGINE
- # define OPENSSL_NO_DYNAMIC_ENGINE
- #endif
-@@ -56,12 +173,66 @@
-who haven't had the time to do the appropriate changes in their
-applications.  *

[edk2] [PATCH 1/7] CryptoPkg: Use OpenSSL include directory directly

[David Woodhouse]

In OpenSSL 1.1, all the public header files will reside directly in the
include/openssl/ directory of the source tree, rather than being symbolic
links. So we can just add that directory to our include path and not have
to worry about copying files around.

In fact, that *already* works on POSIX-compliant systems, because the
existing source release tarballs contain the required symlinks — they're
not created by the configuration. So we can switch our own include setup
now and kill the Install.sh for Linux, and change the Windows Install.cmd
script to copy the files there too.

When we update to 1.1, we can just kill Install.cmd completely (as well
as the patching step too, hopefully.)

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: David Woodhouse <david.woodho...@intel.com>
---
 CryptoPkg/CryptoPkg.dec  |   1 +
 CryptoPkg/Include/openssl/README |   1 -
 CryptoPkg/Library/OpensslLib/Install.cmd | 150 ++--
 CryptoPkg/Library/OpensslLib/Install.sh  |  79 ---
 CryptoPkg/Library/OpensslLib/Patch-HOWTO.txt |   8 +-
 5 files changed, 81 insertions(+), 158 deletions(-)

diff --git a/CryptoPkg/CryptoPkg.dec b/CryptoPkg/CryptoPkg.dec
index 4561f3f..05aad24 100644
--- a/CryptoPkg/CryptoPkg.dec
+++ b/CryptoPkg/CryptoPkg.dec
@@ -24,6 +24,7 @@
 
 [Includes]
   Include
+  Library/OpensslLib/openssl-1.0.2e/include
 
 [LibraryClasses]
   ##  @libraryclass  Provides basic library functions for cryptographic 
primitives.
diff --git a/CryptoPkg/Include/openssl/README b/CryptoPkg/Include/openssl/README
deleted file mode 100644
index 1594010..000
--- a/CryptoPkg/Include/openssl/README
+++ /dev/null
@@ -1 +0,0 @@
-This directory contains all the public include files from the OpenSSL project.
diff --git a/CryptoPkg/Library/OpensslLib/Install.cmd 
b/CryptoPkg/Library/OpensslLib/Install.cmd
index b9b6fc6..e441ce5 100755
--- a/CryptoPkg/Library/OpensslLib/Install.cmd
+++ b/CryptoPkg/Library/OpensslLib/Install.cmd
@@ -1,77 +1,77 @@
 cd openssl-1.0.2e
-copy e_os2.h..\..\..\Include\openssl
-copy crypto\crypto.h..\..\..\Include\openssl
-copy crypto\opensslv.h  ..\..\..\Include\openssl
-copy crypto\opensslconf.h   ..\..\..\Include\openssl
-copy crypto\ebcdic.h..\..\..\Include\openssl
-copy crypto\symhacks.h  ..\..\..\Include\openssl
-copy crypto\ossl_typ.h  ..\..\..\Include\openssl
-copy crypto\objects\objects.h   ..\..\..\Include\openssl
-copy crypto\objects\obj_mac.h   ..\..\..\Include\openssl
-copy crypto\md4\md4.h   ..\..\..\Include\openssl
-copy crypto\md5\md5.h   ..\..\..\Include\openssl
-copy crypto\sha\sha.h   ..\..\..\Include\openssl
-copy crypto\mdc2\mdc2.h ..\..\..\Include\openssl
-copy crypto\hmac\hmac.h ..\..\..\Include\openssl
-copy crypto\ripemd\ripemd.h ..\..\..\Include\openssl
-copy crypto\whrlpool\whrlpool.h ..\..\..\Include\openssl
-copy crypto\des\des.h   ..\..\..\Include\openssl
-copy crypto\des\des_old.h   ..\..\..\Include\openssl
-copy crypto\aes\aes.h   ..\..\..\Include\openssl
-copy crypto\rc2\rc2.h   ..\..\..\Include\openssl
-copy crypto\rc4\rc4.h   ..\..\..\Include\openssl
-copy crypto\idea\idea.h ..\..\..\Include\openssl
-copy crypto\bf\blowfish.h   ..\..\..\Include\openssl
-copy crypto\cast\cast.h ..\..\..\Include\openssl
-copy crypto\camellia\camellia.h ..\..\..\Include\openssl
-copy crypto\seed\seed.h ..\..\..\Include\openssl
-copy crypto\modes\modes.h   ..\..\..\Include\openssl
-copy crypto\bn\bn.h ..\..\..\Include\openssl
-copy crypto\ec\ec.h ..\..\..\Include\openssl
-copy crypto\rsa\rsa.h   ..\..\..\Include\openssl
-copy crypto\dsa\dsa.h   ..\..\..\Include\openssl
-copy crypto\ecdsa\ecdsa.h   ..\..\..\Include\openssl
-copy crypto\dh\dh.h ..\..\..\Include\openssl
-copy crypto\ecdh\ecdh.h ..\..\..\Include\openssl
-copy crypto\dso\dso.h   ..\..\..\Include\openssl
-copy crypto\engine\engine.h ..\..\..\Include\openssl
-copy crypto\buffer\buffer.h ..\..\..\Include\openssl
-copy crypto\bio\bio.h   ..\..\..\Include\openssl
-copy crypto\stack\stack.h   ..\..\..\Include\openssl
-copy crypto\stack\safestack.h   ..\..\..\Include\openssl
-copy crypto\lhash\lhash.h   ..\..\..\Include\openssl
-copy crypto\rand\rand.h ..\..\..\Include\openssl
-copy crypto\err\err.h   ..\..\..\Include\openssl
-copy crypto\evp\evp.h   ..\..\..\Include\openssl
-copy crypto\asn1\asn1.h ..\..\..\Include\openssl
-copy crypto\asn1\asn1_mac.h ..\..\..\Include\openssl
-copy crypto\asn1\asn1t.h..\..\..\Include\openssl
-copy crypto\pem\pem.h   ..\..\..\Include\openssl
-copy crypto\pem\pem2.h  ..\..\..\Include\openssl
-copy crypto\x509\x509.h ..\..\..\Include\openssl
-copy crypto\x509\x509

[edk2] EDK2 vs. OpenSSL HEAD update

[David Woodhouse]

As of today, there are only two remaining fixes that still need to be
merged into OpenSSL HEAD to allow building for EDK2 out of the box.

These are RT4309, a trivial one-liner to provide a correct definition
of the PRIu64 macro for the EDK2 environment, and RT4175 to fix the
regression (also introduced in 1.0.2e) with verifying Authenticode
PKCS#7 signatures.

Hopefully these will be merged shortly.

In the meantime, here's another dump of the patch series to clean up
the OpenSSL integration and then switch over. Patches 1-5 in this
sequence do *not* require switching to OpenSSL HEAD, and stand alone.

The last patch in the series stands alone for reasons which will be
obvious when you see it.

David Woodhouse (7):
  CryptoPkg: Use OpenSSL include directory directly
  CryptoPkg/OpensslLib: Include complete copy of opensslconf.h
  CryptoPkg/OpensslLib: Update OpenSSL patch
  CryptoPkg/OpensslLib: Automatically configure OpenSSL and generate file 
list
  CryptoPkg/OpensslLib: Update to OpenSSL 1.0.2f
  CryptoPkg: Support building with OpenSSL HEAD (1.1.0-devel)
  CryptoPkg: Abuse internal headers to make OpenSSL HEAD build work

 CryptoPkg/CryptoPkg.dec |   2 +
 CryptoPkg/Include/OpenSslSupport.h  |   4 +
 CryptoPkg/Include/openssl/README|   1 -
 CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacMd5.c  |   7 +-
 CryptoPkg/Library/BaseCryptLib/Hmac/CryptHmacSha1.c |   6 +-
 CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Sign.c  |   1 +
 CryptoPkg/Library/BaseCryptLib/Pk/CryptTs.c |   2 +
 CryptoPkg/Library/BaseCryptLib/Pk/CryptX509.c   |   1 +
 CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2e.patch | 707 

 CryptoPkg/Library/OpensslLib/Install.cmd|  77 ---
 CryptoPkg/Library/OpensslLib/Install.sh |  79 ---
 CryptoPkg/Library/OpensslLib/OpenSSL-HOWTO.txt  |  44 ++
 CryptoPkg/Library/OpensslLib/OpensslLib.inf | 466 ++---
 CryptoPkg/Library/OpensslLib/Patch-HOWTO.txt|  61 --
 CryptoPkg/Library/OpensslLib/opensslconf.h  | 262 
 CryptoPkg/Library/OpensslLib/process_files.sh   |  75 +++
 16 files changed, 447 insertions(+), 1348 deletions(-)



-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 3/4] CryptoPkg/OpensslLib: comment out unused code

[David Woodhouse]

On Thu, 2015-12-03 at 12:32 +0100, Ard Biesheuvel wrote:
> 
> 
> ... or maybe not (I hit send too soon)
> 
> It does not appear that there are any tests for those #defines
> anywhere, and the pqueue and ts_* source files are built
> unconditionally by the standard Makefiles.

That might be OK. I think there's magic, and if your no-XXX matches a
directory name crypto/XXX then that directory just gets dropped from
the build. Nobody ever needs to check the resulting OPENSSL_NO_XXX
define, as long as there isn't code that *references* things in those
directories, which needs an #ifdef. And if there were, then I imagine
your original patch would have broken...

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 3/4] CryptoPkg/OpensslLib: comment out unused code

[David Woodhouse]

On Thu, 2015-12-03 at 11:50 +0100, Ard Biesheuvel wrote:
> This comments out the pqueue and ts_* source files from the
> OpensslLib build, since they have no users.

These are going to be auto-generated from the OpenSSL build system (see
the process_files.sh script in the patches I've been submitting).

Is it just a case of adding 'no-pqueue no-ts' to the OpenSSL Configure
invocation in process_files.sh, to make the generated result continue
to match?

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel