Skip to content

Commit

Permalink
device property: Get rid of union aliasing
Browse files Browse the repository at this point in the history 
Commit 318a197 (device property: refactor built-in properties
support) went way too far and brought a union aliasing. Partially
revert it here to get rid of union aliasing.

Note, all Apple properties are considered as u8 arrays. To get a value
of any of them the caller must use device_property_read_u8_array().

What's union aliasing?
~~~~~~~~~~~~~~~~~~~~~~

The C99 standard in section 6.2.5 paragraph 20 defines union type as
"an overlapping nonempty set of member objects". It also states in
section 6.7.2.1 paragraph 14 that "the value of at most one of the
members can be stored in a union object at any time'.

Union aliasing is a type punning mechanism using union members to store
as one type and read back as another.

Why it's not good?
~~~~~~~~~~~~~~~~~~

Section 6.2.6.1 paragraph 6 says that a union object may not be a trap
representation, although its member objects may be.

Meanwhile annex J.1 says that "the value of a union member other than
the last one stored into" is unspecified [removed in C11].

In TC3, a footnote is added which specifies that accessing a member of a
union other than the last one stored causes "the object representation"
to be re-interpreted in the new type and specifically refers to this as
"type punning". This conflicts to some degree with Annex J.1.

While it's working in Linux with GCC, the use of union members to do
type punning is not clear area in the C standard and might lead to
unspecified behaviour.

More information is available in this [1] blog post.

[1]: https://davmac.wordpress.com/2010/02/26/c99-revisited/

Signed-off-by: Andy Shevchenko <[email protected]>
Acked-by: Ard Biesheuvel <[email protected]>
Reviewed-by: Mika Westerberg <[email protected]>
Signed-off-by: Rafael J. Wysocki <[email protected]>
  • Loading branch information
@andy-shev @rafaeljw
andy-shev authored and rafaeljw committed May 17, 2018
1 parent 67b8d5c commit 63dcc70
Show file tree
Hide file tree
Showing 3 changed files with 117 additions and 47 deletions.
104 changes: 86 additions & 18 deletions drivers/base/property.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,72 @@ pset_prop_get(const struct property_set *pset, const char *name)
return NULL;
}

static const void *property_get_pointer(const struct property_entry *prop)
{
switch (prop->type) {
case DEV_PROP_U8:
if (prop->is_array)
return prop->pointer.u8_data;
return &prop->value.u8_data;
case DEV_PROP_U16:
if (prop->is_array)
return prop->pointer.u16_data;
return &prop->value.u16_data;
case DEV_PROP_U32:
if (prop->is_array)
return prop->pointer.u32_data;
return &prop->value.u32_data;
case DEV_PROP_U64:
if (prop->is_array)
return prop->pointer.u64_data;
return &prop->value.u64_data;
case DEV_PROP_STRING:
if (prop->is_array)
return prop->pointer.str;
return &prop->value.str;
default:
return NULL;
}
}

static void property_set_pointer(struct property_entry *prop, const void *pointer)
{
switch (prop->type) {
case DEV_PROP_U8:
if (prop->is_array)
prop->pointer.u8_data = pointer;
else
prop->value.u8_data = *((u8 *)pointer);
break;
case DEV_PROP_U16:
if (prop->is_array)
prop->pointer.u16_data = pointer;
else
prop->value.u16_data = *((u16 *)pointer);
break;
case DEV_PROP_U32:
if (prop->is_array)
prop->pointer.u32_data = pointer;
else
prop->value.u32_data = *((u32 *)pointer);
break;
case DEV_PROP_U64:
if (prop->is_array)
prop->pointer.u64_data = pointer;
else
prop->value.u64_data = *((u64 *)pointer);
break;
case DEV_PROP_STRING:
if (prop->is_array)
prop->pointer.str = pointer;
else
prop->value.str = pointer;
break;
default:
break;
}
}

static const void *pset_prop_find(const struct property_set *pset,
const char *propname, size_t length)
{
Expand All @@ -65,10 +131,7 @@ static const void *pset_prop_find(const struct property_set *pset,
prop = pset_prop_get(pset, propname);
if (!prop)
return ERR_PTR(-EINVAL);
if (prop->is_array)
pointer = prop->pointer.raw_data;
else
pointer = &prop->value.raw_data;
pointer = property_get_pointer(prop);
if (!pointer)
return ERR_PTR(-ENODATA);
if (length > prop->length)
Expand Down Expand Up @@ -698,16 +761,17 @@ EXPORT_SYMBOL_GPL(fwnode_property_get_reference_args);

static void property_entry_free_data(const struct property_entry *p)
{
const void *pointer = property_get_pointer(p);
size_t i, nval;

if (p->is_array) {
if (p->is_string && p->pointer.str) {
if (p->type == DEV_PROP_STRING && p->pointer.str) {
nval = p->length / sizeof(const char *);
for (i = 0; i < nval; i++)
kfree(p->pointer.str[i]);
}
kfree(p->pointer.raw_data);
} else if (p->is_string) {
kfree(pointer);
} else if (p->type == DEV_PROP_STRING) {
kfree(p->value.str);
}
kfree(p->name);
Expand All @@ -716,7 +780,7 @@ static void property_entry_free_data(const struct property_entry *p)
static int property_copy_string_array(struct property_entry *dst,
const struct property_entry *src)
{
char **d;
const char **d;
size_t nval = src->length / sizeof(*d);
int i;

Expand All @@ -734,40 +798,44 @@ static int property_copy_string_array(struct property_entry *dst,
}
}

dst->pointer.raw_data = d;
dst->pointer.str = d;
return 0;
}

static int property_entry_copy_data(struct property_entry *dst,
const struct property_entry *src)
{
const void *pointer = property_get_pointer(src);
const void *new;
int error;

if (src->is_array) {
if (!src->length)
return -ENODATA;

if (src->is_string) {
if (src->type == DEV_PROP_STRING) {
error = property_copy_string_array(dst, src);
if (error)
return error;
new = dst->pointer.str;
} else {
dst->pointer.raw_data = kmemdup(src->pointer.raw_data,
src->length, GFP_KERNEL);
if (!dst->pointer.raw_data)
new = kmemdup(pointer, src->length, GFP_KERNEL);
if (!new)
return -ENOMEM;
}
} else if (src->is_string) {
dst->value.str = kstrdup(src->value.str, GFP_KERNEL);
if (!dst->value.str && src->value.str)
} else if (src->type == DEV_PROP_STRING) {
new = kstrdup(src->value.str, GFP_KERNEL);
if (!new && src->value.str)
return -ENOMEM;
} else {
dst->value.raw_data = src->value.raw_data;
new = pointer;
}

dst->length = src->length;
dst->is_array = src->is_array;
dst->is_string = src->is_string;
dst->type = src->type;

property_set_pointer(dst, new);

dst->name = kstrdup(src->name, GFP_KERNEL);
if (!dst->name)
Expand Down
8 changes: 6 additions & 2 deletions drivers/firmware/efi/apple-properties.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@
*
* You should have received a copy of the GNU General Public License
* along with this program; if not, see <http://www.gnu.org/licenses/>.
*
* Note, all properties are considered as u8 arrays.
* To get a value of any of them the caller must use device_property_read_u8_array().
*/

#define pr_fmt(fmt) "apple-properties: " fmt
Expand Down Expand Up @@ -96,12 +99,13 @@ static void __init unmarshal_key_value_pairs(struct dev_header *dev_header,
entry[i].name = key;
entry[i].length = val_len - sizeof(val_len);
entry[i].is_array = !!entry[i].length;
entry[i].pointer.raw_data = ptr + key_len + sizeof(val_len);
entry[i].type = DEV_PROP_U8;
entry[i].pointer.u8_data = ptr + key_len + sizeof(val_len);

if (dump_properties) {
dev_info(dev, "property: %s\n", entry[i].name);
print_hex_dump(KERN_INFO, pr_fmt(), DUMP_PREFIX_OFFSET,
16, 1, entry[i].pointer.raw_data,
16, 1, entry[i].pointer.u8_data,
entry[i].length, true);
}

Expand Down
52 changes: 25 additions & 27 deletions include/linux/property.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -178,26 +178,24 @@ static inline int fwnode_property_read_u64(const struct fwnode_handle *fwnode,
* @name: Name of the property.
* @length: Length of data making up the value.
* @is_array: True when the property is an array.
* @is_string: True when property is a string.
* @type: Type of the data in unions.
* @pointer: Pointer to the property (an array of items of the given type).
* @value: Value of the property (when it is a single item of the given type).
*/
struct property_entry {
const char *name;
size_t length;
bool is_array;
bool is_string;
enum dev_prop_type type;
union {
union {
const void *raw_data;
const u8 *u8_data;
const u16 *u16_data;
const u32 *u32_data;
const u64 *u64_data;
const char * const *str;
} pointer;
union {
unsigned long long raw_data;
u8 u8_data;
u16 u16_data;
u32 u32_data;
Expand All @@ -213,55 +211,55 @@ struct property_entry {
* and structs.
*/

#define PROPERTY_ENTRY_INTEGER_ARRAY(_name_, _type_, _val_) \
(struct property_entry) { \
.name = _name_, \
.length = ARRAY_SIZE(_val_) * sizeof(_type_), \
.is_array = true, \
.is_string = false, \
{ .pointer = { ._type_##_data = _val_ } }, \
#define PROPERTY_ENTRY_INTEGER_ARRAY(_name_, _type_, _Type_, _val_) \
(struct property_entry) { \
.name = _name_, \
.length = ARRAY_SIZE(_val_) * sizeof(_type_), \
.is_array = true, \
.type = DEV_PROP_##_Type_, \
{ .pointer = { ._type_##_data = _val_ } }, \
}

#define PROPERTY_ENTRY_U8_ARRAY(_name_, _val_) \
PROPERTY_ENTRY_INTEGER_ARRAY(_name_, u8, _val_)
PROPERTY_ENTRY_INTEGER_ARRAY(_name_, u8, U8, _val_)
#define PROPERTY_ENTRY_U16_ARRAY(_name_, _val_) \
PROPERTY_ENTRY_INTEGER_ARRAY(_name_, u16, _val_)
PROPERTY_ENTRY_INTEGER_ARRAY(_name_, u16, U16, _val_)
#define PROPERTY_ENTRY_U32_ARRAY(_name_, _val_) \
PROPERTY_ENTRY_INTEGER_ARRAY(_name_, u32, _val_)
PROPERTY_ENTRY_INTEGER_ARRAY(_name_, u32, U32, _val_)
#define PROPERTY_ENTRY_U64_ARRAY(_name_, _val_) \
PROPERTY_ENTRY_INTEGER_ARRAY(_name_, u64, _val_)
PROPERTY_ENTRY_INTEGER_ARRAY(_name_, u64, U64, _val_)

#define PROPERTY_ENTRY_STRING_ARRAY(_name_, _val_) \
(struct property_entry) { \
.name = _name_, \
.length = ARRAY_SIZE(_val_) * sizeof(const char *), \
.is_array = true, \
.is_string = true, \
.type = DEV_PROP_STRING, \
{ .pointer = { .str = _val_ } }, \
}

#define PROPERTY_ENTRY_INTEGER(_name_, _type_, _val_) \
(struct property_entry) { \
.name = _name_, \
.length = sizeof(_type_), \
.is_string = false, \
{ .value = { ._type_##_data = _val_ } }, \
#define PROPERTY_ENTRY_INTEGER(_name_, _type_, _Type_, _val_) \
(struct property_entry) { \
.name = _name_, \
.length = sizeof(_type_), \
.type = DEV_PROP_##_Type_, \
{ .value = { ._type_##_data = _val_ } }, \
}

#define PROPERTY_ENTRY_U8(_name_, _val_) \
PROPERTY_ENTRY_INTEGER(_name_, u8, _val_)
PROPERTY_ENTRY_INTEGER(_name_, u8, U8, _val_)
#define PROPERTY_ENTRY_U16(_name_, _val_) \
PROPERTY_ENTRY_INTEGER(_name_, u16, _val_)
PROPERTY_ENTRY_INTEGER(_name_, u16, U16, _val_)
#define PROPERTY_ENTRY_U32(_name_, _val_) \
PROPERTY_ENTRY_INTEGER(_name_, u32, _val_)
PROPERTY_ENTRY_INTEGER(_name_, u32, U32, _val_)
#define PROPERTY_ENTRY_U64(_name_, _val_) \
PROPERTY_ENTRY_INTEGER(_name_, u64, _val_)
PROPERTY_ENTRY_INTEGER(_name_, u64, U64, _val_)

#define PROPERTY_ENTRY_STRING(_name_, _val_) \
(struct property_entry) { \
.name = _name_, \
.length = sizeof(_val_), \
.is_string = true, \
.type = DEV_PROP_STRING, \
{ .value = { .str = _val_ } }, \
}

Expand Down

0 comments on commit 63dcc70

Please sign in to comment.