From 2d262a490f3bc30c2524e30ab088d7d8ec0ac648 Mon Sep 17 00:00:00 2001 From: Ryan O'Hara Date: Mon, 16 Mar 2020 14:04:40 -0500 Subject: [PATCH] Fix invalid element address calculation (#1801109) --- ...threads-use-of-crypt-not-thread-safe.patch | 83 ------------ fix-invalid-addr-calc.patch | 118 ++++++++++++++++++ haproxy.spec | 8 +- 3 files changed, 125 insertions(+), 84 deletions(-) delete mode 100644 0001-BUG-MEDIUM-auth-threads-use-of-crypt-not-thread-safe.patch create mode 100644 fix-invalid-addr-calc.patch diff --git a/0001-BUG-MEDIUM-auth-threads-use-of-crypt-not-thread-safe.patch b/0001-BUG-MEDIUM-auth-threads-use-of-crypt-not-thread-safe.patch deleted file mode 100644 index a0149e5..0000000 --- a/0001-BUG-MEDIUM-auth-threads-use-of-crypt-not-thread-safe.patch +++ /dev/null @@ -1,83 +0,0 @@ -From a873c161d251abd025008034c0ddef8cd7f39511 Mon Sep 17 00:00:00 2001 -From: Willy Tarreau -Date: Mon, 29 Oct 2018 18:02:54 +0100 -Subject: BUG/MEDIUM: auth/threads: use of crypt() is not thread-safe - -It was reported here that authentication may fail when threads are -enabled : - - https://bugzilla.redhat.com/show_bug.cgi?id=1643941 - -While I couldn't reproduce the issue, it's obvious that there is a -problem with the use of the non-reentrant crypt() function there. -On Linux systems there's crypt_r() but not on the vast majority of -other ones. Thus a first approach consists in placing a lock around -this crypt() call. Another patch may relax it when crypt_r() is -available. - -This fix must be backported to 1.8. Thanks to Ryan O'Hara for the -quick notification. - -(cherry picked from commit 34d4b525a129baa6f52a930ae629ddb1ba4255c2) -Signed-off-by: Willy Tarreau ---- - include/common/hathreads.h | 2 ++ - src/auth.c | 7 +++++++ - 2 files changed, 9 insertions(+) - -diff --git a/include/common/hathreads.h b/include/common/hathreads.h -index 44bd66d1d..24fb1d1a7 100644 ---- a/include/common/hathreads.h -+++ b/include/common/hathreads.h -@@ -373,6 +373,7 @@ enum lock_label { - START_LOCK, - TLSKEYS_REF_LOCK, - PENDCONN_LOCK, -+ AUTH_LOCK, - LOCK_LABELS - }; - struct lock_stat { -@@ -495,6 +496,7 @@ static inline const char *lock_label(enum lock_label label) - case START_LOCK: return "START"; - case TLSKEYS_REF_LOCK: return "TLSKEYS_REF"; - case PENDCONN_LOCK: return "PENDCONN"; -+ case AUTH_LOCK: return "AUTH"; - case LOCK_LABELS: break; /* keep compiler happy */ - }; - /* only way to come here is consecutive to an internal bug */ -diff --git a/src/auth.c b/src/auth.c -index a2c689f76..e0fb13522 100644 ---- a/src/auth.c -+++ b/src/auth.c -@@ -28,6 +28,7 @@ - #include - #include - #include -+#include - - #include - #include -@@ -37,6 +38,10 @@ - - struct userlist *userlist = NULL; /* list of all existing userlists */ - -+#ifdef CONFIG_HAP_CRYPT -+__decl_hathreads(static HA_SPINLOCK_T auth_lock); -+#endif -+ - /* find targets for selected gropus. The function returns pointer to - * the userlist struct ot NULL if name is NULL/empty or unresolvable. - */ -@@ -245,7 +250,9 @@ check_user(struct userlist *ul, const char *user, const char *pass) - - if (!(u->flags & AU_O_INSECURE)) { - #ifdef CONFIG_HAP_CRYPT -+ HA_SPIN_LOCK(AUTH_LOCK, &auth_lock); - ep = crypt(pass, u->pass); -+ HA_SPIN_UNLOCK(AUTH_LOCK, &auth_lock); - #else - return 0; - #endif --- -2.14.4 - diff --git a/fix-invalid-addr-calc.patch b/fix-invalid-addr-calc.patch new file mode 100644 index 0000000..d893b91 --- /dev/null +++ b/fix-invalid-addr-calc.patch @@ -0,0 +1,118 @@ +From 3285ced5cbec5d8a0c3464f1762d34439280f8eb Mon Sep 17 00:00:00 2001 +From: Willy Tarreau +Date: Wed, 11 Mar 2020 11:54:04 +0100 +Subject: [PATCH] BUG/MAJOR: list: fix invalid element address calculation + +Ryan O'Hara reported that haproxy breaks on fedora-32 using gcc-10 +(pre-release). It turns out that constructs such as: + + while (item != head) { + item = LIST_ELEM(item.n); + } + +loop forever, never matching to despite a printf there +showing them equal. In practice the problem is that the LIST_ELEM() +macro is wrong, it assigns the subtract of two pointers (an integer) +to another pointer through a cast to its pointer type. And GCC 10 now +considers that this cannot match a pointer and silently optimizes the +comparison away. A tested workaround for this is to build with +-fno-tree-pta. Note that older gcc versions even with -ftree-pta do +not exhibit this rather surprizing behavior. + +This patch changes the test to instead cast the null-based address to +an int to get the offset and subtract it from the pointer, and this +time it works. There were just a few places to adjust. Ideally +offsetof() should be used but the LIST_ELEM() API doesn't make this +trivial as it's commonly called with a typeof(ptr) and not typeof(ptr*) +thus it would require to completely change the whole API, which is not +something workable in the short term, especially for a backport. + +With this change, the emitted code is subtly different even on older +versions. A code size reduction of ~600 bytes and a total executable +size reduction of ~1kB are expected to be observed and should not be +taken as an anomaly. Typically this loop in dequeue_proxy_listeners() : + + while ((listener = MT_LIST_POP(...))) + +used to produce this code where the comparison is performed on RAX +while the new offset is assigned to RDI even though both are always +identical: + + 53ded8: 48 8d 78 c0 lea -0x40(%rax),%rdi + 53dedc: 48 83 f8 40 cmp $0x40,%rax + 53dee0: 74 39 je 53df1b + +and now produces this one which is slightly more efficient as the +same register is used for both purposes: + + 53dd08: 48 83 ef 40 sub $0x40,%rdi + 53dd0c: 74 2d je 53dd3b + +Similarly, retrieving the channel from a stream_interface using si_ic() +and si_oc() used to cause this (stream-int in rdi): + + 1cb7: c7 47 1c 00 02 00 00 movl $0x200,0x1c(%rdi) + 1cbe: f6 47 04 10 testb $0x10,0x4(%rdi) + 1cc2: 74 1c je 1ce0 + 1cc4: 48 81 ef 00 03 00 00 sub $0x300,%rdi + 1ccb: 81 4f 10 00 08 00 00 orl $0x800,0x10(%rdi) + +and now causes this: + + 1cb7: c7 47 1c 00 02 00 00 movl $0x200,0x1c(%rdi) + 1cbe: f6 47 04 10 testb $0x10,0x4(%rdi) + 1cc2: 74 1c je 1ce0 + 1cc4: 81 8f 10 fd ff ff 00 orl $0x800,-0x2f0(%rdi) + +There is extremely little chance that this fix wakes up a dormant bug as +the emitted code effectively does what the source code intends. + +This must be backported to all supported branches (dropping MT_LIST_ELEM +and the spoa_example parts as needed), since the bug is subtle and may +not always be visible even when compiling with gcc-10. + +(cherry picked from commit 855796bdc8b1bc2ebcc697ca99fb304bc6e2abe9) +Signed-off-by: Willy Tarreau +--- + contrib/spoa_example/include/mini-clist.h | 2 +- + include/common/mini-clist.h | 4 ++-- + 2 files changed, 3 insertions(+), 3 deletions(-) + +diff --git a/contrib/spoa_example/include/mini-clist.h b/contrib/spoa_example/include/mini-clist.h +index a89255c46..d009704cc 100644 +--- a/contrib/spoa_example/include/mini-clist.h ++++ b/contrib/spoa_example/include/mini-clist.h +@@ -44,7 +44,7 @@ struct list { + * since it's used only once. + * Example: LIST_ELEM(cur_node->args.next, struct node *, args) + */ +-#define LIST_ELEM(lh, pt, el) ((pt)(((void *)(lh)) - ((void *)&((pt)NULL)->el))) ++#define LIST_ELEM(lh, pt, el) ((pt)(((const char *)(lh)) - ((size_t)&((pt)NULL)->el))) + + /* checks if the list head is empty or not */ + #define LIST_ISEMPTY(lh) ((lh)->n == (lh)) +diff --git a/include/common/mini-clist.h b/include/common/mini-clist.h +index e29451148..01bd0b695 100644 +--- a/include/common/mini-clist.h ++++ b/include/common/mini-clist.h +@@ -137,7 +137,7 @@ struct cond_wordlist { + * since it's used only once. + * Example: LIST_ELEM(cur_node->args.next, struct node *, args) + */ +-#define LIST_ELEM(lh, pt, el) ((pt)(((void *)(lh)) - ((void *)&((pt)NULL)->el))) ++#define LIST_ELEM(lh, pt, el) ((pt)(((const char *)(lh)) - ((size_t)&((pt)NULL)->el))) + + /* checks if the list head is empty or not */ + #define LIST_ISEMPTY(lh) ((lh)->n == (lh)) +@@ -463,7 +463,7 @@ struct cond_wordlist { + * since it's used only once. + * Example: MT_LIST_ELEM(cur_node->args.next, struct node *, args) + */ +-#define MT_LIST_ELEM(lh, pt, el) ((pt)(((void *)(lh)) - ((void *)&((pt)NULL)->el))) ++#define MT_LIST_ELEM(lh, pt, el) ((pt)(((const char *)(lh)) - ((size_t)&((pt)NULL)->el))) + + /* checks if the list head is empty or not */ + #define MT_LIST_ISEMPTY(lh) ((lh)->next == (lh)) +-- +2.24.1 + diff --git a/haproxy.spec b/haproxy.spec index f6ac562..54a5210 100644 --- a/haproxy.spec +++ b/haproxy.spec @@ -8,7 +8,7 @@ Name: haproxy Version: 2.1.3 -Release: 1%{?dist} +Release: 2%{?dist} Summary: HAProxy reverse proxy for high availability environments License: GPLv2+ @@ -21,6 +21,8 @@ Source3: %{name}.logrotate Source4: %{name}.sysconfig Source5: halog.1 +Patch1: fix-invalid-addr-calc.patch + BuildRequires: gcc BuildRequires: lua-devel BuildRequires: pcre2-devel @@ -48,6 +50,7 @@ availability environments. Indeed, it can: %prep %setup -q +%patch1 -p1 %build regparm_opts= @@ -132,6 +135,9 @@ exit 0 %{_mandir}/man1/* %changelog +* Mon Mar 16 2020 Ryan O'Hara - 2.1.3-2 +- Fix invalid element address calculation (#1801109) + * Wed Feb 12 2020 Ryan O'Hara - 2.1.3-1 - Update to 2.1.3 (#1802233)