CC2530: zclParseInWriteRspCmd writes outside allocated memory - ZIGBEE-LINUX-SENSOR-TO-CLOUD_1.0.1 --

Hi,

Are there other attributes in this response, other than OccupiedHeatingSetpoint ?

For convenience, I've copied here the structs involved:

// Write Attribute Status record
typedef struct
{
  uint8  status;             // should be ZCL_STATUS_SUCCESS or error
  uint16 attrID;             // attribute ID
} zclWriteRspStatus_t;

// Write Attribute Response Command format
typedef struct
{
  uint8               numAttr;     // number of attribute status in the list
  zclWriteRspStatus_t attrList[];  // attribute status records
} zclWriteRspCmd_t;

For any Write Attributes Response, pCmd->dataLen is assumed to be a multiple of sizeof(zclWriteRspStatus_t). Perhaps an assert check here would be useful (assert( (pCmd->dataLen % sizeof(zclWriteRspStatus_t)    ==    0)).
If this passes, then I would say that the message you see could be a false positive (since zclWriteRspCmd_t) does include a flexible array member.

Regards,
Toby

I added a trace to this code.

It indicates that the sizeof(zclWriteRspCmd_t) equals to 2.

The output in the trace is

[2020-11-06 08:30:01.335,566] [GATEWAY/HNDL] UNMSKBL: zclParseInWriteLen 5 = 2 + 3
[2020-11-06 08:30:01]     #0 0x10e9f9 in zclParseInWriteRspCmd ../../../../Components/stack/zcl/zcl.c:3574

  zclWriteRspCmd_t *writeRspCmd;
  uint8 *pBuf = pCmd->pData;
  uint8 i = 0;

  uint16 len=sizeof ( zclWriteRspCmd_t ) + pCmd->dataLen;
  uiPrintfEx(trUNMASKABLE, "zclParseInWriteLen %u = %u + %u\n",(int)len, sizeof(zclWriteRspCmd_t), pCmd->dataLen);

  writeRspCmd = (zclWriteRspCmd_t *)zcl_mem_alloc( len );

Beyond this specific case, I think that the code relies a bit too much on lengths that should be of a certain size.

Any incoming packet from another system is subject to a difference between the supposed length and the actual length.  In this case the actual packet length is fine, but the fixed length of the struct is not the one expected.

As the condition verifies that the start pointer is still within memory space (and not the end pointer), there is one loop too many.

The issue is due to alignment, I suppose that the code has to be reviewed for this.

Using a 32bit gcc compiler, the sizeof results for the types are:

- zclWriteRspStatus_t 4
- zclWriteRspCmd_t 2

For a fast resolution, I am ammending the compilation with the '-fpack-struct' option.  Then the result is:

- zclWriteRspStatus_t 3
- zclWriteRspCmd_t 1

I do not know the the same issue might arise fo the ZNP compilation iitself as zcl.c is inside the Components directory.  I have not investigated that.

So I do believe that a code review with this issue in mind is needed.  Adding "#pragma pack" may be considered too.

I do not know if this might also impact the Applicaiton executeable for some functionnalities.  I think that it is safe though.

I am now updating the compile scripts.

Note:

- when adding -fpack-struct, I get warnings like this so the "#pragma_pack" technique is already used but not applied everywhere it  is needed.

In file included from ipclib/server/hal_spi.h:54:0,
                 from ipclib/server/hal_spi.c:59:
ipclib/server/hal_gpio.h:112:9: warning: #pragma pack has no effect with -fpack-struct - ignored [-Wpragmas]
 #pragma pack(1)
         ^

By adding tests to hal_types.h, it also appears that an unsigned long is not 64bit on for the target compiler.

I included <limits.h> and check LONG_MAX.

In order to make sure, I defined an uint64 global var and initialised it:

Compiling ../zstackpb/zstack.pb-c.c ...
In file included from <command-line>:0:0:
./../hal/hal_types.h:77:10: warning: large integer implicitly truncated to unsigned type [-Woverflow]
 uint64 t=0x7FFFFFFFFFFFFFFFULL;

It's not because of the ULL which is needed  o ensure that a big constant is defined.
"uint64 v=0x7FFFFFFFULL;" works with the above.

'-fpack-struct' can't be used in the current conditions, it generates an error in 'api_client'.
Including "<limits.h>" in 'api_client.c' resulted in the same or similar effect. 'api_client.c_ probably needs to be improved for that.

.

./srvwrapper/api_client.c: In function 'apicInit':
../srvwrapper/api_client.c:416:45: error: incompatible type for argument 2 of 'connect'
if ( connect( pInstance->sAPIconnected, p->ai_addr, p->ai_addrlen ) != -1 )^M
^
In file included from ../srvwrapper/api_client.c:44:0:
/opt/ti-processor-sdk-linux-am335x-evm-03.03.00.04/linux-devkit/sysroots/armv7ahf-neon-linux-gnueabi/usr/include/sys/socket.h:137:12: note: expected '__CONST_SOCKADDR_ARG {aka union <anonymous>}' but argument is of type 'struct sockaddr *'
extern int connect (int __fd, __CONST_SOCKADDR_ARG __addr, socklen_t __len);
^
Makefile:170: recipe for target 'out/api_client.o' failed
make[1]: *** [out/api_client.o] Error 1
make[1]: Leaving directory '/home/mdeweerd/Desktop/workspace/habitat/3rdparty/ti/Zigbee_3_0_Linux_Gateway_1_0_1/source/Projects/zstack/linux/zstackserverznp'
Makefile:87: recipe for target 'arch-all-arm' failed

I also note that uint64 is used in the gateway proto file, and UINT64 is typedef'd to it in ZComDef.h . Other than that there does not seem to be any use of it.

So I removed '#include <limits.h>' from 'hal_types.h' where the 64bit types are defined as long long for the occasion. I have also removed '-fpack-structs' and added '#pragma pack(1)' on all 'typedef struct' and 'typedef enum' in the 'zcl.h' file.

Unfortunately I get more errors from the gateway with that change.

Packing all structs in 'zcl.h' is not a solution; I got a misaligned access in './Projects/zstack/linux/nwkmgr/nwkmgrdatabase.c':690, 
and other locations .

So I reverted back and changed only the struct with variable length arrays that are not pointers.

I still ran into trouble there.

I am now in a situation where the app fails:

==5009==ERROR: AddressSanitizer: SEGV on unknown address 0x00000002 (pc 0x003250f4 bp 0x00000000 sp 0xadafdf48 T2)
    #0 0x3250f3 in device_process_local_info_response(pkt_buf_t const*, void*) (/home/root/z-stack_linux_gateway_arm_binaries/Habitat3+0x3250f3)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV ??:0 device_process_local_info_response(pkt_buf_t const*, void*)

Without a timeline from TI, I have to suppose that there may be an update for this in six months of so, beyond the date where the first systems will be installed (with the COVID crisis, this date is unclear, ).

While I started fixing the gateway, this does not seem to amount to fixes requiring to know exactly how the structures are used.

I hope that you can provide some earlier time frame.