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.

Sign up for GitHub

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

Jump to bottom

Expose SSL HW record acceleration error. #3310

Merged
merged 2 commits into from
May 11, 2020
Merged

Expose SSL HW record acceleration error. #3310

merged 2 commits into from
May 11, 2020

Conversation

sander-visser
Copy link
Contributor

Fix issue with variable shadowing.

Signed-off-by: sander-visser github@visser.se

Status

READY

Requires Backporting

NO

Migrations

If there is any API change, what's the incentive and logic for it.

YES | NO

Additional comments

Any additional information that could be of interest

Todos

  • Tests
  • Documentation
  • Changelog updated
  • Backported

Steps to test or reproduce

Outline the steps to test or reproduce the PR here.

@sander-visser
Expose SSL HW record acceleration error.
1abe8ee 
Fix issue with variable shadowing.

Signed-off-by: sander-visser <github@visser.se>
@mpg
Copy link
Contributor

mpg commented May 7, 2020

Hi @sander-visser and thanks for your contribution!

Could you please give use more details about the issue this PR is fixing and how you found it:

  • From the fix it looks like this is fixing a warning given by a compiler or static analyzer, not a bug in observable behaviour - is that correct ? (To be clear: we want to fix warnings just as well as functional bugs, we just want to be just what we're fixing.)

  • As it happens, we're planning on removing the MBEDTLS_SSL_HW_RECORD_ACCEL option in Mbed TLS 3.0. If you're using this option and don't want it to be removed in Mbed TLS 3.0, you'll probably want to join the discussion on our mailing list.

@mpg mpg added bug component-tls needs-backports Backports are missing or are pending review and approval. needs: changelog needs-review Every commit must be reviewed by at least two team members, labels May 7, 2020
@sander-visser
Copy link
Contributor Author

Hi @sander-visser and thanks for your contribution!

Could you please give use more details about the issue this PR is fixing and how you found it:

* From the fix it looks like this is fixing a warning given by a compiler or static analyzer, not a bug in observable behaviour - is that correct ? (To be clear: we want to fix warnings just as well as functional bugs, we just want to be just what we're fixing.)

* As it happens, we're [planning on removing the `MBEDTLS_SSL_HW_RECORD_ACCEL` option in Mbed TLS 3.0](https://lists.trustedfirmware.org/pipermail/mbed-tls/2020-April/000022.html). If you're using this option and don't want it to be removed in Mbed TLS 3.0, you'll probably want to join the discussion on our [mailing list](https://lists.trustedfirmware.org/mailman/listinfo/mbed-tls).

The issue is real as far as I have understood. Only the ret local to "if( mbedtls_ssl_hw_record_init != NULL )" would have seen the error and the shadowed ret that is returned from "end:" would still have been 0.

Im not a user of MBEDTLS_SSL_HW_RECORD_ACCEL. I just reacted to issue reported in Cppcheck 1.89 (I scanned the codebase since Im looking into optee that uses it (older version)).

sander-visser added a commit to sander-visser/mbedtls that referenced this pull request May 7, 2020
@sander-visser
Add changelog entry for Mbed-TLS#3310.
77e20d5
@gilles-peskine-arm gilles-peskine-arm mentioned this pull request May 7, 2020
Open
@gilles-peskine-arm
Copy link
Contributor

I just reacted to issue reported in Cppcheck 1.89

Oh, interesting, thanks for the information. Last I tried cppcheck it didn't find anything interesting… But it does have the advantage that it explores different compile-time configurations, whereas almost every other static analysis tool work on the preprocessor output. We should run it in our CI.

gilles-peskine-arm
gilles-peskine-arm previously approved these changes May 7, 2020
View reviewed changes
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. But please add a signoff line to the second commit.

@sander-visser
Add changelog entry for Mbed-TLS#3310.
a65fe0b 
Signed-off-by: sander-visser <github@visser.se>
@sander-visser sander-visser force-pushed the fix-wrong-return-with-ssl-hw-accel-if-init-fails branch from 77e20d5 to a65fe0b Compare May 7, 2020 20:07
mpg
mpg approved these changes May 11, 2020
View reviewed changes
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding a ChangeLog entry. Looks good to me.

@mpg
Copy link
Contributor

mpg commented May 11, 2020

I just checked that the issue was introduced in a refactoring that happened after 2.16, so it's not present in any of the LTS branches and doesn't need backporting.

@mpg mpg removed the needs-backports Backports are missing or are pending review and approval. label May 11, 2020
@gilles-peskine-arm gilles-peskine-arm removed the needs-review Every commit must be reviewed by at least two team members, label May 11, 2020
@gilles-peskine-arm gilles-peskine-arm merged commit f862d73 into Mbed-TLS:development May 11, 2020
yanesca pushed a commit that referenced this pull request Jul 1, 2020
@mpg
Merge branch 'development' into development-restricted
c859c21 
* development: (81 commits)
  Add changelog entry file
  Remove obsolete comment
  Changelog entry noting the behavior change and storage format change
  Update SE support to pass a location when registering a driver
  Update SE support to pass a location when registering a driver
  Update the SE interface to pass a location when registering a driver
  Fix macros
  Missing word
  Define a macro to construct a lifetime from persistence and location
  Document PSA_KEY_PERSISTENCE_xxx and PSA_KEY_LOCATION_xxx
  Rename and clarify the default persistent location and persistence
  PSA_KEY_LIFETIME_PERSISTENT is a lifetime, not just a storage area
  Shorten type and value names for lifetime parts
  Define some structure for lifetime values
  Fix typo in program benchmark.
  Add changelog entry for #3310.
  Add variable initialization to large SSL TLS function.
  Add Changelog entry for #3312
  Scope reduction to enable NULL check to protect dereferencing.
  Expose SSL HW record acceleration error.
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mpg mpg mpg approved these changes

@gilles-peskine-arm gilles-peskine-arm gilles-peskine-arm approved these changes

Assignees

@mpg mpg

Labels
bug component-tls
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sander-visser @mpg @gilles-peskine-arm