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

Fix out of bounds read in slarrv #625

Merged

Conversation

Keno
Copy link
Contributor

@Keno Keno commented Sep 30, 2021

This was originally reported as JuliaLang/LinearAlgebra.jl#873.
I've tracked this down to an out of bounds read on the following line:

DO 170 JBLK = 1, IBLOCK( M )

In the crashing example, M is 0, causing slarrv to read uninitialized
memory from the work array. I believe the 0 for M is correct and indeed,
the documentation above supports that M may be zero:

lapack/SRC/slarrv.f

Lines 113 to 116 in 44ecb6a

*> \param[in] M
*> \verbatim
*> M is INTEGER
*> The total number of input eigenvalues. 0 <= M <= N.

I believe it may be sufficient to early-out this function as suggested
in this PR. However, I have limited context for the full routine here,
so I would appreciate a sanity check.

@andreasnoack
Copy link
Contributor

It looks right to but it might make sense to move this check to

lapack/SRC/slarrv.f

Lines 353 to 355 in 44ecb6a

IF( N.LE.0 ) THEN
RETURN
END IF
where there is already an early exit check.

This issue was found in JuliaLang/LinearAlgebra.jl#873, i.e. the original call was from sstemr and this seems to happen when requesting eigenvectors corresponding to an interval that doesn't contain any eigenvalues.

This PR only fixes the s version of xlarrv so if the fix is considered correct then similar changes should be made to the d, c, and z versions.

@codecov
Copy link

codecov bot commented Sep 30, 2021

Codecov Report

Merging #625 (0631b6b) into master (44ecb6a) will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     JuliaLang/julia#625   +/-   ##
=======================================
  Coverage    0.00%    0.00%           
=======================================
  Files        1894     1894           
  Lines      184021   184021           
=======================================
  Misses     184021   184021           
Impacted Files Coverage Δ
SRC/clarrv.f 0.00% <0.00%> (ø)
SRC/dlarrv.f 0.00% <0.00%> (ø)
SRC/slarrv.f 0.00% <0.00%> (ø)
SRC/zlarrv.f 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 44ecb6a...0631b6b. Read the comment docs.

langou
langou previously approved these changes Sep 30, 2021
Copy link
Collaborator

@martin-frbg martin-frbg 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 (with the change suggested by andreasnoack, to move the exit to the start of the function where we already check for N.LE.0 )

@weslleyspereira
Copy link
Collaborator

I agree with @martin-frbg and @andreasnoack. Could you do this slightly modification, @Keno? Thanks a lot.

This was originally reported as https://github.com/JuliaLang/julia/issues/42415.
I've tracked this down to an our of bounds read on the following line:

https://github.com/Reference-LAPACK/lapack/blob/44ecb6a5ff821b1cbb39f8cc2166cb098e060b4d/SRC/slarrv.f#L423

In the crashing example, `M` is `0`, causing `slarrv` to read uninitialized
memory from the work array. I believe the `0` for `M` is correct and indeed,
the documentation above supports that `M` may be zero:

https://github.com/Reference-LAPACK/lapack/blob/44ecb6a5ff821b1cbb39f8cc2166cb098e060b4d/SRC/slarrv.f#L113-L116

I believe it may be sufficient to early-out this function as suggested
in this PR. However, I have limited context for the full routine here,
so I would appreciate a sanity check.
@Keno
Copy link
Contributor Author

Keno commented Oct 1, 2021

I updated the PR.

@weslleyspereira weslleyspereira merged commit 38f3eee into Reference-LAPACK:master Oct 1, 2021
@weslleyspereira
Copy link
Collaborator

Thanks!! I've just merged it.

@langou
Copy link
Contributor

langou commented Oct 1, 2021

Thanks @Keno. Excellent!

DilumAluthge added a commit to JuliaPackaging/Yggdrasil that referenced this pull request Oct 7, 2021
DilumAluthge added a commit to JuliaPackaging/Yggdrasil that referenced this pull request Oct 7, 2021
DilumAluthge added a commit to JuliaPackaging/Yggdrasil that referenced this pull request Oct 7, 2021
DilumAluthge added a commit to JuliaPackaging/Yggdrasil that referenced this pull request Oct 7, 2021
DilumAluthge added a commit to JuliaPackaging/Yggdrasil that referenced this pull request Oct 8, 2021
DilumAluthge added a commit to JuliaPackaging/Yggdrasil that referenced this pull request Oct 8, 2021
@thoger
Copy link

thoger commented Dec 6, 2021

We got an info that VulnDB is flagging this fix as a security issue. The relevant VulnDB link should be:

https://vulndb.cyberriskanalytics.com/vulnerabilities/270365

and should include the following description:

OpenBLAS contains an out-of-bounds read error in the zlarrv.f library that occurs when user input is not validated properly. This could allow a remote attacker to crash the process associated with the library, or potentially expose the contents of memory by executing arbitrary code.

Note that it's flagging OpenBLAS as affected, not lapack directly, but the relevant file is the lapack code bundled in OpenBLAS.

However, this information is part of the VulnDB subscriber-only content and I do not have access to it, we only got this information indirectly.

AFAICT, there was no CVE assigned for the issue. I'd like to ask if you have any concerns with getting a CVE assigned here as the issue is already getting flagged as a vulnerability. Any thoughts on how likely this issue may be triggered in a real-world use cases via untrusted inputs to a non-malicious applications?

@Keno
Copy link
Contributor Author

Keno commented Dec 6, 2021

It is an out of bounds read, so it does qualify as the kind of vulnerability CVEs are issued for if someone wanted to write that up and submit it. Not sure how likely it is that someone is taking untrusted matrices and running a symmetric tridiag eigensolve on them, which is a bit of a niche thing to do, but if you are, crafting a malicious input is pretty easy.

@thoger
Copy link

thoger commented Dec 10, 2021

weslleyspereira added a commit to weslleyspereira/lapack that referenced this pull request Apr 27, 2022
weslleyspereira added a commit to weslleyspereira/lapack that referenced this pull request Apr 27, 2022
weslleyspereira added a commit to weslleyspereira/lapack that referenced this pull request Apr 27, 2022
@julielangou julielangou added this to the LAPACK 3.10.1 milestone Nov 12, 2022
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.

7 participants