Skip to content

Commit

Permalink
libsframe asan: avoid generating misaligned loads
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@ibhagatgnu
ibhagatgnu committed Dec 15, 2022
1 parent 69de431 commit 8c078ab
Show file tree
Hide file tree
Showing 4 changed files with 42 additions and 24 deletions.
24 changes: 12 additions & 12 deletions bfd/elf64-x86-64.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Expand Down
8 changes: 6 additions & 2 deletions include/sframe-api.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,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)
Expand Down
18 changes: 16 additions & 2 deletions libsframe/sframe.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -652,13 +652,21 @@ 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,
unsigned int fre_type)
{
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)
{
Expand All @@ -668,12 +676,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);
Expand Down
16 changes: 8 additions & 8 deletions libsframe/testsuite/libsframe.encode/encode-1.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down

0 comments on commit 8c078ab

Please sign in to comment.