Skip to content

at86rf2xx/kw2xrf: scalar NETOPT options checked as arrays #8631

@miri64

Description

@miri64

For the get and set options of both at86rf2xx and kw2xrf (and maybe more devices, but I did not care to look), the width of some scalar values is assumed to be like an array (the check is not done with a != operator but with <, >, <= or >=) but than set as a scalar value by way of pointer dereferencing. I noticed it first for NETOPT_MAX_PACKET_SIZE, but I suspect there are more:

        case NETOPT_MAX_PACKET_SIZE:
            if (len < sizeof(int16_t)) {
                return -EOVERFLOW;
            }

            *((uint16_t *)value) = KW2XRF_MAX_PKT_LENGTH - _MAX_MHR_OVERHEAD;
            return sizeof(uint16_t);

While for unsigned numbers on little-endian platforms this shouldn't be a problem, signed numbers or on big endian platform the user might get returned a non-error, but the value might not be as expected. E.g. if in the above example len == sizeof(uint32) on the setting of a value results in a memory region like this:

 0                   1                   2                   3
 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|----------value_space----------|0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0|
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

On a little-endian platform uint32_t be converted correctly, since they are in the most significant place, on big-endian platforms they would however be in the most significant place, making the number really large. The other way around it would be for the set case as on big-endian platforms the 16 most significant bits would be taken, which are 0 here.

To fix this constructs like the one check should be transferred to a

if (len != sizeof(uint16_t)) {
    return -EOVERFLOW;
}

or assert(len == sizeof(uint16_t)); (which would be preferred anyway).

A subset of this issue was already solved in #7488, but there are more problems.

Metadata

Metadata

Labels

Area: driversArea: Device driversArea: networkArea: NetworkingState: staleState: The issue / PR has no activity for >185 daysType: bugThe issue reports a bug / The PR fixes a bug (including spelling errors)

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions