Use-after-free in arc header from dbuf code in ztest #15293
Labels
Component: Encryption
"native encryption" feature
Component: Memory Management
kernel memory management
Type: Defect
Incorrect behavior (e.g. crash, hang)
Comments
pcd1193182
added
Type: Defect
|
I had similar question about headers reallocation into crypto during my latest ARC tour. I haven't seen there any locking either. But since it does not explode all the time I guessed it may be at least partially protected by the header/buffer life cycle. Worth another look. Meanwhile looking on this specific panic I've got orthogonal question: should we really call arc_buf_access() when getting data for ZIL write? We do not want MFU promotions just from the fact the write was synchronous. |
|
Actually, thinking more of it, I recall b_evict_lock I dropped in 2.2 branch, since it was not protecting anything any more. It was used in arc_hdr_realloc_crypt() in such a chunk, that in my opinion did not really protect from the use-after-free: |
|
I told pcd this OOB, but this really just seems like #11679 to me from the trace. |
|
Looking more into this, I think b_evict_lock did protect from certain races around arc_hdr_realloc_crypt(), being taken inside arc_buf_access() and arc_release(). I just doubt it covered all the cases. For example calls like arc_referenced() were racy, even though with extremely small windows, or arc_release() read buf->b_hdr before taking b_evict_lock. I am not exactly happy to resurrect that additional lock in pretty hot paths. I am thinking about moving arc_hdr_realloc_crypt() from arc_write_ready() into higher level awcb_ready callbacks under db_mtx. I think it should be both safer and cheaper, just may be a bit less obvious. |
amotin
added a commit
to amotin/zfs
that referenced
this issue
Sep 29, 2023
As part of openzfs#14340 I've removed b_evict_lock, that played no role in ARC eviction process. But appears it played a secondary role of protecting b_hdr pointers in arc_buf_t during header reallocation by arc_hdr_realloc_crypt(), that, as found in openzfs#15293, may cause use after free races if some encrypted block is read while being synced after been writen. After closer look on b_evict_lock I still do not believe it covered all the possible races, so I am not eager to resurrect it. Instead this refactors arc_hdr_realloc_crypt() into arc_realloc_crypt() and moves its calls from arc_write_ready() into upper levels ready callbacks, like dbuf_write_ready(), where it is protected by existing db_mtx, protecting also all the arc buffer accesses. Signed-off-by: Alexander Motin <[email protected]> Sponsored by: iXsystems, Inc.
13 tasks
amotin
added a commit
to amotin/zfs
that referenced
this issue
Oct 3, 2023
To reduce memory usage ZFS crypto allocated bigger by 64 bytes ARC headers only when specific block was encrypted on disk. It was a nice optimization, except in some cases the code reallocated them on fly, that invalidated header pointers from the buffers. Since the buffers use different locking, it created number of races, that were originally covered (at least partially) by b_evict_lock, used also to protection evictions. But it has gone as part of openzfs#14340. As result, as was found in openzfs#15293, arc_hdr_realloc_crypt() ended up unprotected and causing use-after-free. Instead of introducing some even more elaborate locking, this patch just drops the difference between normal and protected headers. It cost us additional 64 bytes per header, but with couple patches saving 24 bytes, the net growth is only 40 bytes with total header size of 240 bytes on FreeBSD, that IMHO is acceptable price for simplicity. Additional locking would also end up consuming space, time or both. Signed-off-by: Alexander Motin <[email protected]> Sponsored by: iXsystems, Inc.
13 tasks
|
I gave up on attempt to use db_mtx lock. Instead in #15347 I decided to just drop the header reallocation. With growing ashifts and block sizes extra 32 bytes may be not so high price for simplicity these days. |
amotin
added a commit
to amotin/zfs
that referenced
this issue
Oct 4, 2023
To reduce memory usage ZFS crypto allocated bigger by 64 bytes ARC headers only when specific block was encrypted on disk. It was a nice optimization, except in some cases the code reallocated them on fly, that invalidated header pointers from the buffers. Since the buffers use different locking, it created number of races, that were originally covered (at least partially) by b_evict_lock, used also to protection evictions. But it has gone as part of openzfs#14340. As result, as was found in openzfs#15293, arc_hdr_realloc_crypt() ended up unprotected and causing use-after-free. Instead of introducing some even more elaborate locking, this patch just drops the difference between normal and protected headers. It cost us additional 64 bytes per header, but with couple patches saving 24 bytes, the net growth is only 40 bytes with total header size of 240 bytes on FreeBSD, that IMHO is acceptable price for simplicity. Additional locking would also end up consuming space, time or both. Signed-off-by: Alexander Motin <[email protected]> Sponsored by: iXsystems, Inc.
amotin
added a commit
to amotin/zfs
that referenced
this issue
Oct 4, 2023
To reduce memory usage ZFS crypto allocated bigger by 64 bytes ARC headers only when specific block was encrypted on disk. It was a nice optimization, except in some cases the code reallocated them on fly, that invalidated header pointers from the buffers. Since the buffers use different locking, it created number of races, that were originally covered (at least partially) by b_evict_lock, used also to protection evictions. But it has gone as part of openzfs#14340. As result, as was found in openzfs#15293, arc_hdr_realloc_crypt() ended up unprotected and causing use-after-free. Instead of introducing some even more elaborate locking, this patch just drops the difference between normal and protected headers. It cost us additional 64 bytes per header, but with couple patches saving 24 bytes, the net growth is only 40 bytes with total header size of 240 bytes on FreeBSD, that IMHO is acceptable price for simplicity. Additional locking would also end up consuming space, time or both. Signed-off-by: Alexander Motin <[email protected]> Sponsored by: iXsystems, Inc.
amotin
added a commit
to amotin/zfs
that referenced
this issue
Oct 5, 2023
To reduce memory usage ZFS crypto allocated bigger by 64 bytes ARC headers only when specific block was encrypted on disk. It was a nice optimization, except in some cases the code reallocated them on fly, that invalidated header pointers from the buffers. Since the buffers use different locking, it created number of races, that were originally covered (at least partially) by b_evict_lock, used also to protection evictions. But it has gone as part of openzfs#14340. As result, as was found in openzfs#15293, arc_hdr_realloc_crypt() ended up unprotected and causing use-after-free. Instead of introducing some even more elaborate locking, this patch just drops the difference between normal and protected headers. It cost us additional 64 bytes per header, but with couple patches saving 32 bytes, the net growth is only 32 bytes with total header size of 232 bytes on FreeBSD, that IMHO is acceptable price for simplicity. Additional locking would also end up consuming space, time or both. Signed-off-by: Alexander Motin <[email protected]> Sponsored by: iXsystems, Inc.
amotin
added a commit
to amotin/zfs
that referenced
this issue
Oct 5, 2023
To reduce memory usage ZFS crypto allocated bigger by 64 bytes ARC headers only when specific block was encrypted on disk. It was a nice optimization, except in some cases the code reallocated them on fly, that invalidated header pointers from the buffers. Since the buffers use different locking, it created number of races, that were originally covered (at least partially) by b_evict_lock, used also to protection evictions. But it has gone as part of openzfs#14340. As result, as was found in openzfs#15293, arc_hdr_realloc_crypt() ended up unprotected and causing use-after-free. Instead of introducing some even more elaborate locking, this patch just drops the difference between normal and protected headers. It cost us additional 56 bytes per header, but with couple patches saving 24 bytes, the net growth is only 32 bytes with total header size of 232 bytes on FreeBSD, that IMHO is acceptable price for simplicity. Additional locking would also end up consuming space, time or both. Signed-off-by: Alexander Motin <[email protected]> Sponsored by: iXsystems, Inc.
amotin
added a commit
to amotin/zfs
that referenced
this issue
Oct 5, 2023
To reduce memory usage ZFS crypto allocated bigger by 56 bytes ARC headers only when specific block was encrypted on disk. It was a nice optimization, except in some cases the code reallocated them on fly, that invalidated header pointers from the buffers. Since the buffers use different locking, it created number of races, that were originally covered (at least partially) by b_evict_lock, used also to protection evictions. But it has gone as part of openzfs#14340. As result, as was found in openzfs#15293, arc_hdr_realloc_crypt() ended up unprotected and causing use-after-free. Instead of introducing some even more elaborate locking, this patch just drops the difference between normal and protected headers. It cost us additional 56 bytes per header, but with couple patches saving 24 bytes, the net growth is only 32 bytes with total header size of 232 bytes on FreeBSD, that IMHO is acceptable price for simplicity. Additional locking would also end up consuming space, time or both. Signed-off-by: Alexander Motin <[email protected]> Sponsored by: iXsystems, Inc.
behlendorf
pushed a commit
to behlendorf/zfs
that referenced
this issue
Oct 6, 2023
To reduce memory usage ZFS crypto allocated bigger by 56 bytes ARC headers only when specific block was encrypted on disk. It was a nice optimization, except in some cases the code reallocated them on fly, that invalidated header pointers from the buffers. Since the buffers use different locking, it created number of races, that were originally covered (at least partially) by b_evict_lock, used also to protection evictions. But it has gone as part of openzfs#14340. As result, as was found in openzfs#15293, arc_hdr_realloc_crypt() ended up unprotected and causing use-after-free. Instead of introducing some even more elaborate locking, this patch just drops the difference between normal and protected headers. It cost us additional 56 bytes per header, but with couple patches saving 24 bytes, the net growth is only 32 bytes with total header size of 232 bytes on FreeBSD, that IMHO is acceptable price for simplicity. Additional locking would also end up consuming space, time or both. Reviewe-by: Brian Behlendorf <[email protected]> Signed-off-by: Alexander Motin <[email protected]> Sponsored by: iXsystems, Inc. Closes openzfs#15293 Closes openzfs#15347
behlendorf
pushed a commit
that referenced
this issue
Oct 7, 2023
To reduce memory usage ZFS crypto allocated bigger by 56 bytes ARC headers only when specific block was encrypted on disk. It was a nice optimization, except in some cases the code reallocated them on fly, that invalidated header pointers from the buffers. Since the buffers use different locking, it created number of races, that were originally covered (at least partially) by b_evict_lock, used also to protection evictions. But it has gone as part of #14340. As result, as was found in #15293, arc_hdr_realloc_crypt() ended up unprotected and causing use-after-free. Instead of introducing some even more elaborate locking, this patch just drops the difference between normal and protected headers. It cost us additional 56 bytes per header, but with couple patches saving 24 bytes, the net growth is only 32 bytes with total header size of 232 bytes on FreeBSD, that IMHO is acceptable price for simplicity. Additional locking would also end up consuming space, time or both. Reviewe-by: Brian Behlendorf <[email protected]> Signed-off-by: Alexander Motin <[email protected]> Sponsored by: iXsystems, Inc. Closes #15293 Closes #15347
lundman
pushed a commit
to openzfsonwindows/openzfs
that referenced
this issue
Dec 12, 2023
To reduce memory usage ZFS crypto allocated bigger by 56 bytes ARC headers only when specific block was encrypted on disk. It was a nice optimization, except in some cases the code reallocated them on fly, that invalidated header pointers from the buffers. Since the buffers use different locking, it created number of races, that were originally covered (at least partially) by b_evict_lock, used also to protection evictions. But it has gone as part of openzfs#14340. As result, as was found in openzfs#15293, arc_hdr_realloc_crypt() ended up unprotected and causing use-after-free. Instead of introducing some even more elaborate locking, this patch just drops the difference between normal and protected headers. It cost us additional 56 bytes per header, but with couple patches saving 24 bytes, the net growth is only 32 bytes with total header size of 232 bytes on FreeBSD, that IMHO is acceptable price for simplicity. Additional locking would also end up consuming space, time or both. Reviewe-by: Brian Behlendorf <[email protected]> Signed-off-by: Alexander Motin <[email protected]> Sponsored by: iXsystems, Inc. Closes openzfs#15293 Closes openzfs#15347
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
I got this report from a ztest run with ASAN enabled:
From my reading of the code, what happened here is fairly obvious; we have a dbuf with an arc buf storing its data. That arc buffer has dirty data in it, and we’re in the process of writing it out. Part of that is transforming the arc buffer into a protected buffer (because encryption is enabled in the dataset). When we do that, we replace the header with a new one, and then free it. Unfortunately, there is no synchronization happening here; any references to the existing header will remain outstanding. As a result, it’s possible to UAF the headers, with bad timing. This seems like a fairly obvious race, but there is no synchronization mechanism here. I believe fixing it would involve introducing a mutex that you have to hold around access to the arc header from the arc buf.
The text was updated successfully, but these errors were encountered: