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 unsoundness of AtomicCell<*64> arithmetics on 32-bit targets that support Atomic*64 #781

Merged
merged 1 commit into from
Feb 5, 2022

Conversation

taiki-e
Copy link
Member

@taiki-e taiki-e commented Feb 5, 2022

The alignment of u64/i64 on a 32-bit target can be smaller than AtomicU64/AtomicI64.

32-bit targets without Atomic*64 (#767) and 64-bit targets are not affected by this issue.

@taiki-e
Copy link
Member Author

taiki-e commented Feb 5, 2022

bors r+

bors bot added a commit that referenced this pull request Feb 5, 2022
781: Fix unsoundness of AtomicCell<*64> arithmetics on 32-bit targets that support Atomic*64 r=taiki-e a=taiki-e

The alignment of u64/i64 on a 32-bit target can be smaller than AtomicU64/AtomicI64.

32-bit targets without Atomic*64 (#767) and 64-bit targets are not affected by this issue.

Co-authored-by: Taiki Endo <[email protected]>
@bors
Copy link
Contributor

bors bot commented Feb 5, 2022

Canceled.

@taiki-e
Copy link
Member Author

taiki-e commented Feb 5, 2022

bors r+

@bors
Copy link
Contributor

bors bot commented Feb 5, 2022

Build succeeded:

@bors bors bot merged commit 924436c into master Feb 5, 2022
@bors bors bot deleted the atomic-cell-64-32 branch February 5, 2022 05:51
bors bot added a commit that referenced this pull request Feb 5, 2022
786: Prepare for the next release r=taiki-e a=taiki-e

- crossbeam-epoch 0.9.6 -> 0.9.7
  - Fix Miri error when `-Zmiri-check-number-validity` is enabled. (#779)
- crossbeam-queue 0.3.3 -> 0.3.4
  - Implement `IntoIterator` for `ArrayQueue` and `SegQueue`. (#772)
- crossbeam-utils 0.8.6 -> 0.8.7
  - Add `AtomicCell<i*,u*>::{fetch_max,fetch_min}`. (#785)
  - Add `AtomicCell<i*,u*,bool>::fetch_nand`. (#785)
  - Fix unsoundness of `AtomicCell<{i,u}64>` arithmetics on 32-bit targets that support `Atomic{I,U}64` (#781)


Co-authored-by: Taiki Endo <[email protected]>
bors bot added a commit that referenced this pull request Feb 5, 2022
786: Prepare for the next release r=taiki-e a=taiki-e

- crossbeam-epoch 0.9.6 -> 0.9.7
  - Fix Miri error when `-Zmiri-check-number-validity` is enabled. (#779)
- crossbeam-queue 0.3.3 -> 0.3.4
  - Implement `IntoIterator` for `ArrayQueue` and `SegQueue`. (#772)
- crossbeam-utils 0.8.6 -> 0.8.7
  - Add `AtomicCell<{i*,u*}>::{fetch_max,fetch_min}`. (#785)
  - Add `AtomicCell<{i*,u*,bool}>::fetch_nand`. (#785)
  - Fix unsoundness of `AtomicCell<{i,u}64>` arithmetics on 32-bit targets that support `Atomic{I,U}64` (#781)


Co-authored-by: Taiki Endo <[email protected]>
@jlebon
Copy link

jlebon commented Feb 11, 2022

Were the previous releases yanked because of this? It'd be good to note that in the CHANGELOG under each version yanked for folks looking for more context. Thanks!

@taiki-e
Copy link
Member Author

taiki-e commented Feb 13, 2022

Originally I was going to write a security advisory on this, but I was busy with work and it got delayed. (I requested CVE today, so it should be published in the next few days)

That said, I agree that mentioning the yanked version is a good idea (I usually do it on my projects), so I did that in #788.

@taiki-e
Copy link
Member Author

taiki-e commented Feb 14, 2022

Advisory published as GHSA-qc84-gqf4-9926.

kodiakhq bot pushed a commit to Azure/iotedge that referenced this pull request Feb 18, 2022
Resolves vulnerability in crossbeam-util. See crossbeam-rs/crossbeam#781 for details.

## Azure IoT Edge PR checklist:
kodiakhq bot pushed a commit to Azure/iotedge that referenced this pull request Feb 23, 2022
… versions (#6135)

crossbeam-utils recently patched an unsoundness bug in `AtomicCell` arithmetic: crossbeam-rs/crossbeam#781. However, this patch was not backported to the older versions required by tokio 0.1 and rayon. This PR updates tokio and rayon to the latest available versions to unify our crossbeam dependencies, in particular unifying crossbeam-util to version 0.7.2. In the process, we can verify that we are not affected by the unsoundness bug by attempting a build with crossbeam-util patched to remove AtomicCell arithmetic.

## Azure IoT Edge PR checklist:

This checklist is used to make sure that common guidelines for a pull request are followed.

### General Guidelines and Best Practices
- [x] I have read the [contribution guidelines](https://github.com/azure/iotedge#contributing).
- [x] Title of the pull request is clear and informative.
- [x] Description of the pull request includes a concise summary of the enhancement or bug fix.

### Testing Guidelines
- [ ] Pull request includes test coverage for the included changes.
- Description of the pull request includes 
	- [ ] concise summary of tests added/modified
	- [x] local testing done.
kodiakhq bot pushed a commit to Azure/iotedge that referenced this pull request Feb 24, 2022
… versions (#6136)

crossbeam-utils recently patched an unsoundness bug in `AtomicCell` arithmetic: crossbeam-rs/crossbeam#781. However, this patch was not backported to the older versions required by tokio 0.1 and rayon. This PR updates tokio and rayon to the latest available versions, allowing some dependencies to upgrade to a safe crossbeam version. However, tokio still remains on a vulnerable version, so manual verification with a patched build was still required to determine that are not affected by the bug.

The notary module had to be modified to remove the `tokio-process` dependency due to the latter's reliance on crossbeam-utils 0.6 (through crossbeam-queue). notary now uses `tokio_threadpool::blocking`.

## Azure IoT Edge PR checklist:

This checklist is used to make sure that common guidelines for a pull request are followed.

### General Guidelines and Best Practices
- [x] I have read the [contribution guidelines](https://github.com/azure/iotedge#contributing).
- [x] Title of the pull request is clear and informative.
- [x] Description of the pull request includes a concise summary of the enhancement or bug fix.

### Testing Guidelines
- [ ] Pull request includes test coverage for the included changes.
- Description of the pull request includes 
	- [ ] concise summary of tests added/modified
	- [x] local testing done.
@schopin-pro
Copy link

Hi!

Is there a reason why the 0.7.* versions haven't been yanked? I thought it was because they weren't affected by the issue but looking a bit more closely to the changelog for 0.8.0 I don't see why it would be affected and not the 0.7.* versions.

When browsing the reverse-dependency list on crates.io, there are still quite a few crates that specify a ^0.7.0 dependency. Rust 1.58.1, for instance, had both 0.8.5 and 0.7.2 in its vendored dependency tree.

@taiki-e
Copy link
Member Author

taiki-e commented Mar 15, 2022

@schopin-pro

I thought it was because they weren't affected by the issue but looking a bit more closely to the changelog for 0.8.0 I don't see why it would be affected and not the 0.7.* versions.

As "Affected versions" in the advisory says, 0.7.x releases are also affected. However, in 0.7.0 and older releases, the affected functionality requires the nightly feature. So the only release that is not yanked and still considered affected is 0.7.2.

For the same reason as #749 (comment), we no longer maintain 0.7.x releases. So no backport to 0.7.x has been made. However, if someone sends a backport to the 0.7 branch and also fixes the CI-related issues (which we expect will probably occur), then we can accept it and release a new 0.7.x release.

When browsing the reverse-dependency list on crates.io, there are still quite a few crates that specify a ^0.7.0 dependency.

As far as I know, many of them are crates that are not maintained, or use crossbeam-utils as a dev-dependency.

Rust 1.58.1, for instance, had both 0.8.5 and 0.7.2 in its vendored dependency tree.

That has already been fixed: rust-lang/rust@f3b8812
Also, the rustc's case (rustc-rayon) does not seem to be affected by this issue.

@schopin-pro
Copy link

schopin-pro commented Mar 15, 2022 via email

@taiki-e
Copy link
Member Author

taiki-e commented Mar 15, 2022

I feel like the 0.7.2 should have been yanked as well, so that people that are indirect consumers lagging a bit behind would have the yanked warning.

Unfortunately, yanking does not actually work that way.
If we yank 0.7.2, a crate that requires crossbeam-utils ^0.7.2 as a dependency will fail to resolve dependencies if Cargo.lock is not yet present.

damonbarry pushed a commit to damonbarry/iotedge that referenced this pull request Apr 15, 2022
…re#6137)

Resolves vulnerability in crossbeam-util. See crossbeam-rs/crossbeam#781 for details.

## Azure IoT Edge PR checklist:
bergwolf added a commit to bergwolf/nydus that referenced this pull request Apr 28, 2022
brayniac added a commit to brayniac/pelikan-twitter that referenced this pull request Jun 6, 2022
Updates crossbeam-utils to 0.8.8 to fix crossbeam-rs/crossbeam#781
Ryan1729 added a commit to Ryan1729/rote that referenced this pull request Jun 10, 2022
…-utils

    Specifically, we still rely on crossbeam-utils v0.7.2, which has a known vulnerability fixed in v0.8.7

    It does not seem worth putting more effort into this at this time for two reasons:

    * Using `rote` does not involve connecting to the internet.
    * The vulnerability only affects 32-bit targets according to crossbeam-rs/crossbeam#781 . I'm not running `rote` on any of those at the moment, anyway.

    If `rusttype` releases the fixed version to `crates.io` then running `cargo update` will fix it for me, so I plan to wait until some time after that is released to address this.
alevy added a commit to faasten/faasten that referenced this pull request Jun 19, 2022
Updates crossbeam-channel to a newer version that doesn't rely on a
buggy version of crossbeam-utils
(crossbeam-rs/crossbeam#781)
@pinkforest
Copy link

pinkforest commented Jul 31, 2022

Hey @taiki-e could you release the advisory as Public Domain so we can publish this in RustSec ?

rustsec/advisory-db#1203

Or please feel free to send PR to https://github.com/rustsec/advisory-db

EDIT: Oh wait there is CVE-2022-23639 - that we can use - perfect.

Thanks!

@taiki-e
Copy link
Member Author

taiki-e commented Jul 31, 2022

Sigh. I thought those were in the public domain.

I re-licensed all advisories in this repository as the public domain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants