Skip to content

Commit

Permalink
liblzma: Fix false Valgrind error report with GCC.
Browse files Browse the repository at this point in the history
With GCC and a certain combination of flags, Valgrind will falsely
trigger an invalid write. This appears to be due to the omission of
instructions to properly save, set up, and restore the frame pointer.

The IFUNC resolver is a leaf function since it only calls a function
that is inlined. So sometimes GCC omits the frame pointer instructions
in the resolver unless this optimization is explictly disabled.

This fixes https://bugzilla.redhat.com/show_bug.cgi?id=2267598.
  • Loading branch information
JiaT75 committed Mar 9, 2024
1 parent 3007e74 commit 82ecc53
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 10 deletions.
9 changes: 3 additions & 6 deletions src/liblzma/check/crc32_fast.c
Expand Up @@ -135,15 +135,12 @@ typedef uint32_t (*crc32_func_type)(
// This resolver is shared between all three dispatch methods. It serves as
// the ifunc resolver if ifunc is supported, otherwise it is called as a
// regular function by the constructor or first call resolution methods.
// The __no_profile_instrument_function__ attribute support is checked when
// determining if ifunc can be used, so it is safe to use here.
#ifdef CRC_USE_IFUNC
__attribute__((__no_profile_instrument_function__))
#endif
// The funcion attributes are needed for safe IFUNC resolver usage with GCC.
lzma_resolver_attributes
static crc32_func_type
crc32_resolve(void)
{
return is_arch_extension_supported()
return is_arch_extension_supported()
? &crc32_arch_optimized : &crc32_generic;
}

Expand Down
7 changes: 3 additions & 4 deletions src/liblzma/check/crc64_fast.c
Expand Up @@ -98,13 +98,12 @@ typedef uint64_t (*crc64_func_type)(
# pragma GCC diagnostic ignored "-Wunused-function"
#endif

#ifdef CRC_USE_IFUNC
__attribute__((__no_profile_instrument_function__))
#endif
// The funcion attributes are needed for safe IFUNC resolver usage with GCC.
lzma_resolver_attributes
static crc64_func_type
crc64_resolve(void)
{
return is_arch_extension_supported()
return is_arch_extension_supported()

This comment has been minimized.

Copy link
@Arinerron

Arinerron Mar 29, 2024

whyy

This comment has been minimized.

Copy link
@Sora1248

Sora1248 Mar 29, 2024

As this would break the sed-pattern that injects the backdoor: Haven't we all committed debug-changes by accident? ๐Ÿ˜„

? &crc64_arch_optimized : &crc64_generic;
}

Expand Down
25 changes: 25 additions & 0 deletions src/liblzma/check/crc_common.h
Expand Up @@ -128,6 +128,31 @@
# endif
#endif

#ifdef CRC_USE_IFUNC
// Two function attributes are needed to make IFUNC safe with GCC.
//
// no-omit-frame-pointer prevents false Valgrind issues when combined with
// a few other compiler flags. The optimize attribute is supported on
// GCC >= 4.4 and is not supported with Clang.
# if TUKLIB_GNUC_REQ(4,4) && !defined(__clang__)
# define no_omit_frame_pointer \
__attribute__((optimize("no-omit-frame-pointer")))
# else
# define no_omit_frame_pointer
# endif

// The __no_profile_instrument_function__ attribute support is checked when
// determining if ifunc can be used, so it is safe to use unconditionally.
// This attribute is needed because GCC can add profiling to the IFUNC
// resolver, which calls functions that have not yet been relocated leading
// to a crash on liblzma start up.
# define lzma_resolver_attributes \
__attribute__((__no_profile_instrument_function__)) \
no_omit_frame_pointer
#else
# define lzma_resolver_attributes
#endif

// For CRC32 use the generic slice-by-eight implementation if no optimized
// version is available.
#if !defined(CRC32_ARCH_OPTIMIZED) && !defined(CRC32_GENERIC)
Expand Down

9 comments on commit 82ecc53

@kloczek
Copy link

Choose a reason for hiding this comment

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

Looks like this commit fixes https://www.openwall.com/lists/oss-security/2024/03/29/4
Is it any plan to release ASAP new version because this commit? ๐Ÿค”

@eli-schwartz
Copy link

Choose a reason for hiding this comment

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

@kloczek have you read the writeup correctly? Because this commit is described in your link as being part of the backdoor.

... in addition to being backported as 651a154 and released in the "ASAP new version" from 3 weeks ago.

@kloczek
Copy link

@kloczek kloczek commented on 82ecc53 Mar 29, 2024

Choose a reason for hiding this comment

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

OK so my understanding is that in Phoronix article someone wrongly that 4.6.1 is affected? ๐Ÿค”

@mrbid
Copy link

@mrbid mrbid commented on 82ecc53 Mar 29, 2024

Choose a reason for hiding this comment

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

Someone contact Brian Krebs about this at: https://krebsonsecurity.com/about/
Link him to the source article: https://www.openwall.com/lists/oss-security/2024/03/29/4

Otherwise the threat actor might slip though the net, but if he gains mainstream attention it's more likely he will face scrutiney by an alphabet organisation or two and we might find out who funded the threat actor or what his intentions where.

@gamer191
Copy link

Choose a reason for hiding this comment

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

@kloczek itโ€™s a supply chain attack, meaning (at least) one of the projectโ€™s major contributors added malicious code

So it has not been fixed, and likely wonโ€™t be depending how high up that contributor is

@kloczek
Copy link

Choose a reason for hiding this comment

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

I've red one more time CVE and my understanding is that compromised is not content of the git repo but github tag assets where are distributed dist tar balls).
Am I right? ๐Ÿค”

@rugk
Copy link

@rugk rugk commented on 82ecc53 Mar 30, 2024

Choose a reason for hiding this comment

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

@kianmeng https://boehs.org/node/everything-i-know-about-the-xz-backdoor is one of the best summaries IMHO and worth a read.

@Buggem

This comment was marked as spam.

@Buggem

This comment was marked as spam.

Please sign in to comment.