|
@@ -0,0 +1,334 @@
|
|
|
+# Fix CVE-2025-31115 in XZ Utils 5.3.3alpha to 5.8.0
|
|
|
+# This applies to all affected releases.
|
|
|
+# https://tukaani.org/xz/threaded-decoder-early-free.html
|
|
|
+
|
|
|
+From 831b55b971cf579ee16a854f177c36b20d3c6999 Mon Sep 17 00:00:00 2001
|
|
|
+From: Lasse Collin <[email protected]>
|
|
|
+Date: Thu, 3 Apr 2025 14:34:42 +0300
|
|
|
+Subject: [PATCH 1/4] liblzma: mt dec: Fix a comment
|
|
|
+
|
|
|
+Reviewed-by: Sebastian Andrzej Siewior <[email protected]>
|
|
|
+Thanks-to: Sam James <[email protected]>
|
|
|
+---
|
|
|
+ src/liblzma/common/stream_decoder_mt.c | 2 +-
|
|
|
+ 1 file changed, 1 insertion(+), 1 deletion(-)
|
|
|
+
|
|
|
+diff --git a/src/liblzma/common/stream_decoder_mt.c b/src/liblzma/common/stream_decoder_mt.c
|
|
|
+index 22c9375f..812b745d 100644
|
|
|
+--- a/src/liblzma/common/stream_decoder_mt.c
|
|
|
++++ b/src/liblzma/common/stream_decoder_mt.c
|
|
|
+@@ -347,7 +347,7 @@ worker_enable_partial_update(void *thr_ptr)
|
|
|
+
|
|
|
+
|
|
|
+ /// Things do to at THR_STOP or when finishing a Block.
|
|
|
+-/// This is called with thr->mutex locked.
|
|
|
++/// This is called with thr->coder->mutex locked.
|
|
|
+ static void
|
|
|
+ worker_stop(struct worker_thread *thr)
|
|
|
+ {
|
|
|
+--
|
|
|
+2.49.0
|
|
|
+
|
|
|
+
|
|
|
+From c0c835964dfaeb2513a3c0bdb642105152fe9f34 Mon Sep 17 00:00:00 2001
|
|
|
+From: Lasse Collin <[email protected]>
|
|
|
+Date: Thu, 3 Apr 2025 14:34:42 +0300
|
|
|
+Subject: [PATCH 2/4] liblzma: mt dec: Simplify by removing the THR_STOP state
|
|
|
+
|
|
|
+The main thread can directly set THR_IDLE in threads_stop() which is
|
|
|
+called when errors are detected. threads_stop() won't return the stopped
|
|
|
+threads to the pool or free the memory pointed by thr->in anymore, but
|
|
|
+it doesn't matter because the existing workers won't be reused after
|
|
|
+an error. The resources will be cleaned up when threads_end() is
|
|
|
+called (reinitializing the decoder always calls threads_end()).
|
|
|
+
|
|
|
+Reviewed-by: Sebastian Andrzej Siewior <[email protected]>
|
|
|
+Thanks-to: Sam James <[email protected]>
|
|
|
+---
|
|
|
+ src/liblzma/common/stream_decoder_mt.c | 75 ++++++++++----------------
|
|
|
+ 1 file changed, 29 insertions(+), 46 deletions(-)
|
|
|
+
|
|
|
+diff --git a/src/liblzma/common/stream_decoder_mt.c b/src/liblzma/common/stream_decoder_mt.c
|
|
|
+index 812b745d..82962c64 100644
|
|
|
+--- a/src/liblzma/common/stream_decoder_mt.c
|
|
|
++++ b/src/liblzma/common/stream_decoder_mt.c
|
|
|
+@@ -23,15 +23,10 @@ typedef enum {
|
|
|
+ THR_IDLE,
|
|
|
+
|
|
|
+ /// Decoding is in progress.
|
|
|
+- /// Main thread may change this to THR_STOP or THR_EXIT.
|
|
|
++ /// Main thread may change this to THR_IDLE or THR_EXIT.
|
|
|
+ /// The worker thread may change this to THR_IDLE.
|
|
|
+ THR_RUN,
|
|
|
+
|
|
|
+- /// The main thread wants the thread to stop whatever it was doing
|
|
|
+- /// but not exit. Main thread may change this to THR_EXIT.
|
|
|
+- /// The worker thread may change this to THR_IDLE.
|
|
|
+- THR_STOP,
|
|
|
+-
|
|
|
+ /// The main thread wants the thread to exit.
|
|
|
+ THR_EXIT,
|
|
|
+
|
|
|
+@@ -346,27 +341,6 @@ worker_enable_partial_update(void *thr_ptr)
|
|
|
+ }
|
|
|
+
|
|
|
+
|
|
|
+-/// Things do to at THR_STOP or when finishing a Block.
|
|
|
+-/// This is called with thr->coder->mutex locked.
|
|
|
+-static void
|
|
|
+-worker_stop(struct worker_thread *thr)
|
|
|
+-{
|
|
|
+- // Update memory usage counters.
|
|
|
+- thr->coder->mem_in_use -= thr->in_size;
|
|
|
+- thr->in_size = 0; // thr->in was freed above.
|
|
|
+-
|
|
|
+- thr->coder->mem_in_use -= thr->mem_filters;
|
|
|
+- thr->coder->mem_cached += thr->mem_filters;
|
|
|
+-
|
|
|
+- // Put this thread to the stack of free threads.
|
|
|
+- thr->next = thr->coder->threads_free;
|
|
|
+- thr->coder->threads_free = thr;
|
|
|
+-
|
|
|
+- mythread_cond_signal(&thr->coder->cond);
|
|
|
+- return;
|
|
|
+-}
|
|
|
+-
|
|
|
+-
|
|
|
+ static MYTHREAD_RET_TYPE
|
|
|
+ worker_decoder(void *thr_ptr)
|
|
|
+ {
|
|
|
+@@ -397,17 +371,6 @@ next_loop_unlocked:
|
|
|
+ return MYTHREAD_RET_VALUE;
|
|
|
+ }
|
|
|
+
|
|
|
+- if (thr->state == THR_STOP) {
|
|
|
+- thr->state = THR_IDLE;
|
|
|
+- mythread_mutex_unlock(&thr->mutex);
|
|
|
+-
|
|
|
+- mythread_sync(thr->coder->mutex) {
|
|
|
+- worker_stop(thr);
|
|
|
+- }
|
|
|
+-
|
|
|
+- goto next_loop_lock;
|
|
|
+- }
|
|
|
+-
|
|
|
+ assert(thr->state == THR_RUN);
|
|
|
+
|
|
|
+ // Update progress info for get_progress().
|
|
|
+@@ -510,7 +473,22 @@ next_loop_unlocked:
|
|
|
+ && thr->coder->thread_error == LZMA_OK)
|
|
|
+ thr->coder->thread_error = ret;
|
|
|
+
|
|
|
+- worker_stop(thr);
|
|
|
++ // Return the worker thread to the stack of available
|
|
|
++ // threads.
|
|
|
++ {
|
|
|
++ // Update memory usage counters.
|
|
|
++ thr->coder->mem_in_use -= thr->in_size;
|
|
|
++ thr->in_size = 0; // thr->in was freed above.
|
|
|
++
|
|
|
++ thr->coder->mem_in_use -= thr->mem_filters;
|
|
|
++ thr->coder->mem_cached += thr->mem_filters;
|
|
|
++
|
|
|
++ // Put this thread to the stack of free threads.
|
|
|
++ thr->next = thr->coder->threads_free;
|
|
|
++ thr->coder->threads_free = thr;
|
|
|
++ }
|
|
|
++
|
|
|
++ mythread_cond_signal(&thr->coder->cond);
|
|
|
+ }
|
|
|
+
|
|
|
+ goto next_loop_lock;
|
|
|
+@@ -544,17 +522,22 @@ threads_end(struct lzma_stream_coder *coder, const lzma_allocator *allocator)
|
|
|
+ }
|
|
|
+
|
|
|
+
|
|
|
++/// Tell worker threads to stop without doing any cleaning up.
|
|
|
++/// The clean up will be done when threads_exit() is called;
|
|
|
++/// it's not possible to reuse the threads after threads_stop().
|
|
|
++///
|
|
|
++/// This is called before returning an unrecoverable error code
|
|
|
++/// to the application. It would be waste of processor time
|
|
|
++/// to keep the threads running in such a situation.
|
|
|
+ static void
|
|
|
+ threads_stop(struct lzma_stream_coder *coder)
|
|
|
+ {
|
|
|
+ for (uint32_t i = 0; i < coder->threads_initialized; ++i) {
|
|
|
++ // The threads that are in the THR_RUN state will stop
|
|
|
++ // when they check the state the next time. There's no
|
|
|
++ // need to signal coder->threads[i].cond.
|
|
|
+ mythread_sync(coder->threads[i].mutex) {
|
|
|
+- // The state must be changed conditionally because
|
|
|
+- // THR_IDLE -> THR_STOP is not a valid state change.
|
|
|
+- if (coder->threads[i].state != THR_IDLE) {
|
|
|
+- coder->threads[i].state = THR_STOP;
|
|
|
+- mythread_cond_signal(&coder->threads[i].cond);
|
|
|
+- }
|
|
|
++ coder->threads[i].state = THR_IDLE;
|
|
|
+ }
|
|
|
+ }
|
|
|
+
|
|
|
+@@ -1941,7 +1924,7 @@ stream_decoder_mt_init(lzma_next_coder *next, const lzma_allocator *allocator,
|
|
|
+ // accounting from scratch, too. Changes in filter and block sizes may
|
|
|
+ // affect number of threads.
|
|
|
+ //
|
|
|
+- // FIXME? Reusing should be easy but unlike the single-threaded
|
|
|
++ // Reusing threads doesn't seem worth it. Unlike the single-threaded
|
|
|
+ // decoder, with some types of input file combinations reusing
|
|
|
+ // could leave quite a lot of memory allocated but unused (first
|
|
|
+ // file could allocate a lot, the next files could use fewer
|
|
|
+--
|
|
|
+2.49.0
|
|
|
+
|
|
|
+
|
|
|
+From d5a2ffe41bb77b918a8c96084885d4dbe4bf6480 Mon Sep 17 00:00:00 2001
|
|
|
+From: Lasse Collin <[email protected]>
|
|
|
+Date: Thu, 3 Apr 2025 14:34:42 +0300
|
|
|
+Subject: [PATCH 3/4] liblzma: mt dec: Don't free the input buffer too early
|
|
|
+ (CVE-2025-31115)
|
|
|
+
|
|
|
+The input buffer must be valid as long as the main thread is writing
|
|
|
+to the worker-specific input buffer. Fix it by making the worker
|
|
|
+thread not free the buffer on errors and not return the worker thread to
|
|
|
+the pool. The input buffer will be freed when threads_end() is called.
|
|
|
+
|
|
|
+With invalid input, the bug could at least result in a crash. The
|
|
|
+effects include heap use after free and writing to an address based
|
|
|
+on the null pointer plus an offset.
|
|
|
+
|
|
|
+The bug has been there since the first committed version of the threaded
|
|
|
+decoder and thus affects versions from 5.3.3alpha to 5.8.0.
|
|
|
+
|
|
|
+As the commit message in 4cce3e27f529 says, I had made significant
|
|
|
+changes on top of Sebastian's patch. This bug was indeed introduced
|
|
|
+by my changes; it wasn't in Sebastian's version.
|
|
|
+
|
|
|
+Thanks to Harri K. Koskinen for discovering and reporting this issue.
|
|
|
+
|
|
|
+Fixes: 4cce3e27f529 ("liblzma: Add threaded .xz decompressor.")
|
|
|
+Reported-by: Harri K. Koskinen <[email protected]>
|
|
|
+Reviewed-by: Sebastian Andrzej Siewior <[email protected]>
|
|
|
+Thanks-to: Sam James <[email protected]>
|
|
|
+---
|
|
|
+ src/liblzma/common/stream_decoder_mt.c | 31 ++++++++++++++++++--------
|
|
|
+ 1 file changed, 22 insertions(+), 9 deletions(-)
|
|
|
+
|
|
|
+diff --git a/src/liblzma/common/stream_decoder_mt.c b/src/liblzma/common/stream_decoder_mt.c
|
|
|
+index 82962c64..98aabcff 100644
|
|
|
+--- a/src/liblzma/common/stream_decoder_mt.c
|
|
|
++++ b/src/liblzma/common/stream_decoder_mt.c
|
|
|
+@@ -435,8 +435,7 @@ next_loop_unlocked:
|
|
|
+ }
|
|
|
+
|
|
|
+ // Either we finished successfully (LZMA_STREAM_END) or an error
|
|
|
+- // occurred. Both cases are handled almost identically. The error
|
|
|
+- // case requires updating thr->coder->thread_error.
|
|
|
++ // occurred.
|
|
|
+ //
|
|
|
+ // The sizes are in the Block Header and the Block decoder
|
|
|
+ // checks that they match, thus we know these:
|
|
|
+@@ -444,16 +443,30 @@ next_loop_unlocked:
|
|
|
+ assert(ret != LZMA_STREAM_END
|
|
|
+ || thr->out_pos == thr->block_options.uncompressed_size);
|
|
|
+
|
|
|
+- // Free the input buffer. Don't update in_size as we need
|
|
|
+- // it later to update thr->coder->mem_in_use.
|
|
|
+- lzma_free(thr->in, thr->allocator);
|
|
|
+- thr->in = NULL;
|
|
|
+-
|
|
|
+ mythread_sync(thr->mutex) {
|
|
|
++ // Block decoder ensures this, but do a sanity check anyway
|
|
|
++ // because thr->in_filled < thr->in_size means that the main
|
|
|
++ // thread is still writing to thr->in.
|
|
|
++ if (ret == LZMA_STREAM_END && thr->in_filled != thr->in_size) {
|
|
|
++ assert(0);
|
|
|
++ ret = LZMA_PROG_ERROR;
|
|
|
++ }
|
|
|
++
|
|
|
+ if (thr->state != THR_EXIT)
|
|
|
+ thr->state = THR_IDLE;
|
|
|
+ }
|
|
|
+
|
|
|
++ // Free the input buffer. Don't update in_size as we need
|
|
|
++ // it later to update thr->coder->mem_in_use.
|
|
|
++ //
|
|
|
++ // This step is skipped if an error occurred because the main thread
|
|
|
++ // might still be writing to thr->in. The memory will be freed after
|
|
|
++ // threads_end() sets thr->state = THR_EXIT.
|
|
|
++ if (ret == LZMA_STREAM_END) {
|
|
|
++ lzma_free(thr->in, thr->allocator);
|
|
|
++ thr->in = NULL;
|
|
|
++ }
|
|
|
++
|
|
|
+ mythread_sync(thr->coder->mutex) {
|
|
|
+ // Move our progress info to the main thread.
|
|
|
+ thr->coder->progress_in += thr->in_pos;
|
|
|
+@@ -474,8 +487,8 @@ next_loop_unlocked:
|
|
|
+ thr->coder->thread_error = ret;
|
|
|
+
|
|
|
+ // Return the worker thread to the stack of available
|
|
|
+- // threads.
|
|
|
+- {
|
|
|
++ // threads only if no errors occurred.
|
|
|
++ if (ret == LZMA_STREAM_END) {
|
|
|
+ // Update memory usage counters.
|
|
|
+ thr->coder->mem_in_use -= thr->in_size;
|
|
|
+ thr->in_size = 0; // thr->in was freed above.
|
|
|
+--
|
|
|
+2.49.0
|
|
|
+
|
|
|
+
|
|
|
+From 8188048854e8d11071b8a50d093c74f4c030acc9 Mon Sep 17 00:00:00 2001
|
|
|
+From: Lasse Collin <[email protected]>
|
|
|
+Date: Thu, 3 Apr 2025 14:34:42 +0300
|
|
|
+Subject: [PATCH 4/4] liblzma: mt dec: Don't modify thr->in_size in the worker
|
|
|
+ thread
|
|
|
+
|
|
|
+Don't set thr->in_size = 0 when returning the thread to the stack of
|
|
|
+available threads. Not only is it useless, but the main thread may
|
|
|
+read the value in SEQ_BLOCK_THR_RUN. With valid inputs, it made
|
|
|
+no difference if the main thread saw the original value or 0. With
|
|
|
+invalid inputs (when worker thread stops early), thr->in_size was
|
|
|
+no longer modified after the previous commit with the security fix
|
|
|
+("Don't free the input buffer too early").
|
|
|
+
|
|
|
+So while the bug appears harmless now, it's important to fix it because
|
|
|
+the variable was being modified without proper locking. It's trivial
|
|
|
+to fix because there is no need to change the value. Only main thread
|
|
|
+needs to set the value in (in SEQ_BLOCK_THR_INIT) when starting a new
|
|
|
+Block before the worker thread is activated.
|
|
|
+
|
|
|
+Fixes: 4cce3e27f529 ("liblzma: Add threaded .xz decompressor.")
|
|
|
+Reviewed-by: Sebastian Andrzej Siewior <[email protected]>
|
|
|
+Thanks-to: Sam James <[email protected]>
|
|
|
+---
|
|
|
+ src/liblzma/common/stream_decoder_mt.c | 6 ++++--
|
|
|
+ 1 file changed, 4 insertions(+), 2 deletions(-)
|
|
|
+
|
|
|
+diff --git a/src/liblzma/common/stream_decoder_mt.c b/src/liblzma/common/stream_decoder_mt.c
|
|
|
+index 98aabcff..1fa92220 100644
|
|
|
+--- a/src/liblzma/common/stream_decoder_mt.c
|
|
|
++++ b/src/liblzma/common/stream_decoder_mt.c
|
|
|
+@@ -491,8 +491,6 @@ next_loop_unlocked:
|
|
|
+ if (ret == LZMA_STREAM_END) {
|
|
|
+ // Update memory usage counters.
|
|
|
+ thr->coder->mem_in_use -= thr->in_size;
|
|
|
+- thr->in_size = 0; // thr->in was freed above.
|
|
|
+-
|
|
|
+ thr->coder->mem_in_use -= thr->mem_filters;
|
|
|
+ thr->coder->mem_cached += thr->mem_filters;
|
|
|
+
|
|
|
+@@ -1554,6 +1552,10 @@ stream_decode_mt(void *coder_ptr, const lzma_allocator *allocator,
|
|
|
+ }
|
|
|
+
|
|
|
+ // Return if the input didn't contain the whole Block.
|
|
|
++ //
|
|
|
++ // NOTE: When we updated coder->thr->in_filled a few lines
|
|
|
++ // above, the worker thread might by now have finished its
|
|
|
++ // work and returned itself back to the stack of free threads.
|
|
|
+ if (coder->thr->in_filled < coder->thr->in_size) {
|
|
|
+ assert(*in_pos == in_size);
|
|
|
+ return LZMA_OK;
|
|
|
+--
|
|
|
+2.49.0
|
|
|
+
|