* [PATCH, PR 29856] libsframe: avoid generating misaligned loads
@ 2022-12-14 23:43 Indu Bhagat
2022-12-15 12:58 ` Nick Clifton
0 siblings, 1 reply; 2+ messages in thread
From: Indu Bhagat @ 2022-12-14 23:43 UTC (permalink / raw)
To: binutils; +Cc: Indu Bhagat
Hello,
This patch fixes the PR 29856 "libsframe asan: load misaligned at
sframe.c:516". More details below in the commit log.
Testing Notes:
- Regression tested gas testsuite with -fsanitize=alignment for
--target=aarch64-elf showed the issues fixed and no new regressions.
- binutils try bit showed no new regressions
- various native and cross builds (on host aarch64-linux and host
x86_64-linux) showed no new regressions.
Thanks
------------------------
There are two places where unaligned loads were seen on aarch64:
- #1. access to the SFrame FRE stack offsets in the in-memory
representation/abstraction provided by libsframe.
- #2. access to the SFrame FRE start address in the on-disk representation
of the frame row entry.
For #1, we can fix this by reordering the struct members of
sframe_frame_row_entry in libsframe/sframe-api.h.
For #2, we need to default to using memcpy instead, and copy out the bytes
to a location for output.
SFrame format is an unaligned on-disk format. As such, there are other blobs
of memory in the on-disk SFrame FRE that are on not on their natural
boundaries. But that does not pose further problems yet, because the users
are provided access to the on-disk SFrame FRE data via libsframe's
sframe_frame_row_entry, the latter has its' struct members aligned on their
respective natural boundaries (and initialized using memcpy).
PR 29856 libsframe asan: load misaligned at sframe.c:516
ChangeLog:
PR libsframe/29856
* bfd/elf64-x86-64.c: Adjust as the struct members have been
reordered.
* libsframe/sframe.c (sframe_decode_fre_start_address): Use
memcpy to perform 16-bit/32-bit reads.
* libsframe/testsuite/libsframe.encode/encode-1.c: Adjust as the
struct members have been reordered.
include/ChangeLog:
PR libsframe/29856
* sframe-api.h: Reorder fre_offsets for natural alignment.
---
bfd/elf64-x86-64.c | 24 +++++++++----------
include/sframe-api.h | 8 +++++--
libsframe/sframe.c | 18 ++++++++++++--
.../testsuite/libsframe.encode/encode-1.c | 16 ++++++-------
4 files changed, 42 insertions(+), 24 deletions(-)
diff --git a/bfd/elf64-x86-64.c b/bfd/elf64-x86-64.c
index afc8c76c52b..8cf733d89e0 100644
--- a/bfd/elf64-x86-64.c
+++ b/bfd/elf64-x86-64.c
@@ -822,48 +822,48 @@ static const bfd_byte elf_x86_64_eh_frame_non_lazy_plt[] =
static const sframe_frame_row_entry elf_x86_64_sframe_null_fre =
{
0,
- SFRAME_V1_FRE_INFO (SFRAME_BASE_REG_SP, 1, SFRAME_FRE_OFFSET_1B), /* FRE info. */
- {16, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0} /* 12 bytes. */
+ {16, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}, /* 12 bytes. */
+ SFRAME_V1_FRE_INFO (SFRAME_BASE_REG_SP, 1, SFRAME_FRE_OFFSET_1B) /* FRE info. */
};
/* .sframe FRE covering the .plt section entry. */
static const sframe_frame_row_entry elf_x86_64_sframe_plt0_fre1 =
{
0, /* SFrame FRE start address. */
- SFRAME_V1_FRE_INFO (SFRAME_BASE_REG_SP, 1, SFRAME_FRE_OFFSET_1B), /* FRE info. */
- {16, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0} /* 12 bytes. */
+ {16, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}, /* 12 bytes. */
+ SFRAME_V1_FRE_INFO (SFRAME_BASE_REG_SP, 1, SFRAME_FRE_OFFSET_1B) /* FRE info. */
};
/* .sframe FRE covering the .plt section entry. */
static const sframe_frame_row_entry elf_x86_64_sframe_plt0_fre2 =
{
6, /* SFrame FRE start address. */
- SFRAME_V1_FRE_INFO (SFRAME_BASE_REG_SP, 1, SFRAME_FRE_OFFSET_1B), /* FRE info. */
- {24, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0} /* 12 bytes. */
+ {24, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}, /* 12 bytes. */
+ SFRAME_V1_FRE_INFO (SFRAME_BASE_REG_SP, 1, SFRAME_FRE_OFFSET_1B) /* FRE info. */
};
/* .sframe FRE covering the .plt section entry. */
static const sframe_frame_row_entry elf_x86_64_sframe_pltn_fre1 =
{
0, /* SFrame FRE start address. */
- SFRAME_V1_FRE_INFO (SFRAME_BASE_REG_SP, 1, SFRAME_FRE_OFFSET_1B), /* FRE info. */
- {8, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0} /* 12 bytes. */
+ {8, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}, /* 12 bytes. */
+ SFRAME_V1_FRE_INFO (SFRAME_BASE_REG_SP, 1, SFRAME_FRE_OFFSET_1B) /* FRE info. */
};
/* .sframe FRE covering the .plt section entry. */
static const sframe_frame_row_entry elf_x86_64_sframe_pltn_fre2 =
{
11, /* SFrame FRE start address. */
- SFRAME_V1_FRE_INFO (SFRAME_BASE_REG_SP, 1, SFRAME_FRE_OFFSET_1B), /* FRE info. */
- {16, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0} /* 12 bytes. */
+ {16, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}, /* 12 bytes. */
+ SFRAME_V1_FRE_INFO (SFRAME_BASE_REG_SP, 1, SFRAME_FRE_OFFSET_1B) /* FRE info. */
};
/* .sframe FRE covering the second .plt section entry. */
static const sframe_frame_row_entry elf_x86_64_sframe_sec_pltn_fre1 =
{
0, /* SFrame FRE start address. */
- SFRAME_V1_FRE_INFO (SFRAME_BASE_REG_SP, 1, SFRAME_FRE_OFFSET_1B), /* FRE info. */
- {8, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0} /* 12 bytes. */
+ {8, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}, /* 12 bytes. */
+ SFRAME_V1_FRE_INFO (SFRAME_BASE_REG_SP, 1, SFRAME_FRE_OFFSET_1B) /* FRE info. */
};
/* SFrame helper object for non-lazy PLT. Also used for IBT enabled PLT. */
diff --git a/include/sframe-api.h b/include/sframe-api.h
index 3338a2ebd5c..bd1833558a4 100644
--- a/include/sframe-api.h
+++ b/include/sframe-api.h
@@ -35,13 +35,17 @@ typedef struct sframe_encoder_ctx sframe_encoder_ctx;
/* User interfacing SFrame Row Entry.
An abstraction provided by libsframe so the consumer is decoupled from
- the binary format representation of the same. */
+ the binary format representation of the same.
+
+ The members are best ordered such that they are aligned at their natural
+ boundaries. This helps avoid usage of undesirable misaligned memory
+ accesses. See PR libsframe/29856. */
typedef struct sframe_frame_row_entry
{
uint32_t fre_start_addr;
- unsigned char fre_info;
unsigned char fre_offsets[MAX_OFFSET_BYTES];
+ unsigned char fre_info;
} sframe_frame_row_entry;
#define SFRAME_ERR ((int) -1)
diff --git a/libsframe/sframe.c b/libsframe/sframe.c
index ef821da3901..b8fde2f04f8 100644
--- a/libsframe/sframe.c
+++ b/libsframe/sframe.c
@@ -670,6 +670,11 @@ sframe_frame_row_entry_copy (sframe_frame_row_entry *dst, sframe_frame_row_entry
return 0;
}
+/* Decode the SFrame FRE start address offset value from FRE_BUF in on-disk
+ binary format, given the FRE_TYPE. Updates the FRE_START_ADDR.
+
+ Returns 0 on success, SFRAME_ERR otherwise. */
+
static int
sframe_decode_fre_start_address (const char *fre_buf,
uint32_t *fre_start_addr,
@@ -677,6 +682,9 @@ sframe_decode_fre_start_address (const char *fre_buf,
{
uint32_t saddr = 0;
int err = 0;
+ size_t addr_size = 0;
+
+ addr_size = sframe_fre_start_addr_size (fre_type);
if (fre_type == SFRAME_FRE_TYPE_ADDR1)
{
@@ -686,12 +694,18 @@ sframe_decode_fre_start_address (const char *fre_buf,
else if (fre_type == SFRAME_FRE_TYPE_ADDR2)
{
uint16_t *ust = (uint16_t *)fre_buf;
- saddr = (uint32_t)*ust;
+ /* SFrame is an unaligned on-disk format. Using memcpy helps avoid the
+ use of undesirable unaligned loads. See PR libsframe/29856. */
+ uint16_t tmp = 0;
+ memcpy (&tmp, ust, addr_size);
+ saddr = (uint32_t)tmp;
}
else if (fre_type == SFRAME_FRE_TYPE_ADDR4)
{
uint32_t *uit = (uint32_t *)fre_buf;
- saddr = (uint32_t)*uit;
+ int32_t tmp = 0;
+ memcpy (&tmp, uit, addr_size);
+ saddr = (uint32_t)tmp;
}
else
return sframe_set_errno (&err, SFRAME_ERR_INVAL);
diff --git a/libsframe/testsuite/libsframe.encode/encode-1.c b/libsframe/testsuite/libsframe.encode/encode-1.c
index 01481106a62..0f5225ff9ec 100644
--- a/libsframe/testsuite/libsframe.encode/encode-1.c
+++ b/libsframe/testsuite/libsframe.encode/encode-1.c
@@ -33,10 +33,10 @@ add_fde1 (sframe_encoder_ctx *encode, int idx)
int i, err;
/* A contiguous block containing 4 FREs. */
sframe_frame_row_entry fres[]
- = { {0x0, 0x3, {0x8, 0, 0}},
- {0x1, 0x5, {0x10, 0xf0, 0}},
- {0x4, 0x4, {0x10, 0xf0, 0}},
- {0x1a, 0x5, {0x8, 0xf0, 0}}
+ = { {0x0, {0x8, 0, 0}, 0x3},
+ {0x1, {0x10, 0xf0, 0}, 0x5},
+ {0x4, {0x10, 0xf0, 0}, 0x4},
+ {0x1a, {0x8, 0xf0, 0}, 0x5}
};
unsigned char finfo = sframe_fde_create_func_info (SFRAME_FRE_TYPE_ADDR1,
@@ -58,10 +58,10 @@ add_fde2 (sframe_encoder_ctx *encode, int idx)
int i, err;
/* A contiguous block containing 4 FREs. */
sframe_frame_row_entry fres[]
- = { {0x0, 0x3, {0x8, 0, 0}},
- {0x1, 0x5, {0x10, 0xf0, 0}},
- {0x4, 0x4, {0x10, 0xf0, 0}},
- {0xf, 0x5, {0x8, 0xf0, 0}}
+ = { {0x0, {0x8, 0, 0}, 0x3},
+ {0x1, {0x10, 0xf0, 0}, 0x5},
+ {0x4, {0x10, 0xf0, 0}, 0x4},
+ {0xf, {0x8, 0xf0, 0}, 0x5}
};
unsigned char finfo = sframe_fde_create_func_info (SFRAME_FRE_TYPE_ADDR1,
--
2.37.2
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH, PR 29856] libsframe: avoid generating misaligned loads
2022-12-14 23:43 [PATCH, PR 29856] libsframe: avoid generating misaligned loads Indu Bhagat
@ 2022-12-15 12:58 ` Nick Clifton
0 siblings, 0 replies; 2+ messages in thread
From: Nick Clifton @ 2022-12-15 12:58 UTC (permalink / raw)
To: Indu Bhagat, binutils
Hi Indu,
> ChangeLog:
>
> PR libsframe/29856
> * bfd/elf64-x86-64.c: Adjust as the struct members have been
> reordered.
> * libsframe/sframe.c (sframe_decode_fre_start_address): Use
> memcpy to perform 16-bit/32-bit reads.
> * libsframe/testsuite/libsframe.encode/encode-1.c: Adjust as the
> struct members have been reordered.
>
> include/ChangeLog:
>
> PR libsframe/29856
> * sframe-api.h: Reorder fre_offsets for natural alignment.
Approved = please apply.
Cheers
Nick
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-12-15 12:59 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-14 23:43 [PATCH, PR 29856] libsframe: avoid generating misaligned loads Indu Bhagat
2022-12-15 12:58 ` Nick Clifton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).