Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Fixes #507 in LibTomCrypt #3320

Merged
merged 1 commit into from
Oct 9, 2019
Merged

Conversation

jbech-linaro
Copy link
Contributor

@jbech-linaro jbech-linaro commented Oct 8, 2019

@werew FYI and btw, we tend to use "Signed-off-by" tags in patches in our tree, so I wonder, is it OK if I add the following to the commit message?

Signed-off-by: Luigi Coniglio <[email protected]>>

Fix a vulnerability in der_decode_utf8_string as specified here:
libtom/libtomcrypt#507

Patch created by @werew and manually picked from:
libtom/libtomcrypt@25c26a3

@jenswi-linaro
Copy link
Contributor

It would at least be preferable to have a Signed-off-by.
@werew, can you confirm that
Signed-off-by: Luigi Coniglio <[email protected]>
is fine with you?

@jforissier
Copy link
Contributor

I'd rather have a clearer subject, too, such as "libtomcrypt: fix vulnerability in der_decode_utf8_string()".

+1 for the S-o-b, although I think having Joakim as the author and first S-o-b could be OK, since it is clearly mentioned in the commit text who the original author is (and the S-o-b exactly means that you acknowledge that the patch is suitable for inclusion in OP-TEE).

Last thing: we will want that to be merged in the import branch first (well, order does not really matter actually).

@werew
Copy link
Contributor

werew commented Oct 8, 2019

@jbech-linaro @jenswi-linaro yes, having a Signed-off-by tag is fine with me, go ahead.

@jbech-linaro
Copy link
Contributor Author

@jforissier wrote: Last thing: we will want that to be merged in the import branch first (well, order does not really matter actually).

I can do that, but why is that necessary?

@werew wrote: @jbech-linaro @jenswi-linaro yes, having a Signed-off-by tag is fine with me, go ahead.

Thanks!

@jbech-linaro
Copy link
Contributor Author

I'd rather have a clearer subject, too, such as "libtomcrypt: fix vulnerability in der_decode_utf8_string()".

I wanted to keep it close to the original message, so it should be somewhat easy to identify. I can extend the subject a bit.

@jenswi-linaro
Copy link
Contributor

@jforissier wrote: Last thing: we will want that to be merged in the import branch first (well, order does not really matter actually).

I can do that, but why is that necessary?

Actually I think it's best to do it on the import branch first in order to refer to that commit when cherry-picking for master. That's how we keep track of local modifications.

@jforissier
Copy link
Contributor

I wanted to keep it close to the original message, so it should be somewhat easy to identify. I can extend the subject a bit.

Fair enough, I would also keep the original text in general but TBH this commit subject is quite useless when seen by itself (such as when browsing git log --oneline). That's why I'd rather have a better subject and keep the original subject / link to LTC issue etc. in the commit text.

@jbech-linaro
Copy link
Contributor Author

jbech-linaro commented Oct 8, 2019

Fix a vulnerability in der_decode_utf8_string as specified here:
libtom/libtomcrypt#507

Patch manually picked from:
  libtom/libtomcrypt@25c26a3

Signed-off-by: Luigi Coniglio <[email protected]>
[Joakim Bech: Extended commit message]
Signed-off-by: Joakim Bech <[email protected]>
Acked-by: Joakim Bech <[email protected]>
Tested-by: Joakim Bech <[email protected]> (QEMU v7)
Acked-by: Jerome Forissier <[email protected]>
@jbech-linaro
Copy link
Contributor Author

Same patch (force) pushed here as we just merged to the import branch.

@jforissier jforissier merged commit c4108ef into OP-TEE:master Oct 9, 2019
@jbech-linaro jbech-linaro deleted the ltc_issue_507 branch October 9, 2019 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants