Potential array overflow in iis3dwb_fifo_out_raw_get

[escherstair]

This snippet of code https://github.com/STMicroelectronics/iis3dwb-pid/blob/7dac6d5f408aa7a7ea18721c5a77b224f35b13ff/iis3dwb_reg.c#L919-L920 reads sizeof(iis3dwb_fifo_out_raw_t) bytes from the FIFO and stores them into buf (which is 7 bytes large). I suppose that the purpose is to read 7 bytes from the FIFO, because from the datasheet every FIFO item is 7 bytes large (1 byte of tag + 6 bytes of data). The problem is that iis3dwb_fifo_out_raw_t is declared as https://github.com/STMicroelectronics/iis3dwb-pid/blob/7dac6d5f408aa7a7ea18721c5a77b224f35b13ff/iis3dwb_reg.h#L1110-L1119 but in C (not C++) the sizeof of th enum is not defined by the standard. As an example, you can read here.

If you use ARM Compiler with -fshort-enums option (see here) or GCC (I haven't tried), probably the sizeof of the tag enum is 1 byes (based on the values declared for tag), but this is not guaranteed. And so, sizeof(iis3dwb_fifo_out_raw_t) can be larger than 7 bytes. In this case, buf is written beyond its boundary.

I see only one clean and safe possibility to fix this: changing iis3dwb_fifo_out_raw_t declaration so that it's 7 bytes long. In this case it cannot include an enum anymore, but it should be

typedef struct
{
  uint8_t tag;
  uint8_t data[6];
} iis3dwb_fifo_out_raw_t;

But even in this way, I'm not sure if the starting address of a iis3dwb_fifo_out_raw_t variable would remain byte-aligned, or it would be chnaged to int-aligned.

The problem is that https://github.com/STMicroelectronics/iis3dwb-pid/blob/7dac6d5f408aa7a7ea18721c5a77b224f35b13ff/iis3dwb_reg.c#L961-L973 reads 7 * num bytes and stores them into fdata which is an array of num iis3dwb_fifo_out_raw_t variables. This massive read works only if iis3dwb_fifo_out_raw_t are stored into memory byte-aligned.

The topic is quite technical, and I hope I've been able to describe it.

[escherstair]

Hi @avisconti , I can open a PR that removes the enum tag but I'm not sure if this is the "approved" fix from ST side

[avisconti]

This snippet of code

https://github.com/STMicroelectronics/iis3dwb-pid/blob/7dac6d5f408aa7a7ea18721c5a77b224f35b13ff/iis3dwb_reg.c#L919-L920

reads sizeof(iis3dwb_fifo_out_raw_t) bytes from the FIFO and stores them into buf (which is 7 bytes large). I suppose that the purpose is to read 7 bytes from the FIFO, because from the datasheet every FIFO item is 7 bytes large (1 byte of tag + 6 bytes of data). The problem is that iis3dwb_fifo_out_raw_t is declared as https://github.com/STMicroelectronics/iis3dwb-pid/blob/7dac6d5f408aa7a7ea18721c5a77b224f35b13ff/iis3dwb_reg.h#L1110-L1119

but in C (not C++) the sizeof of th enum is not defined by the standard. As an example, you can read here. If you use ARM Compiler with -fshort-enums option (see here) or GCC (I haven't tried), probably the sizeof of the tag enum is 1 byes (based on the values declared for tag), but this is not guaranteed. And so, sizeof(iis3dwb_fifo_out_raw_t) can be larger than 7 bytes. In this case, buf is written beyond its boundary.

I see only one clean and safe possibility to fix this: changing iis3dwb_fifo_out_raw_t declaration so that it's 7 bytes long. In this case it cannot include an enum anymore, but it should be

typedef struct
{
  uint8_t tag;
  uint8_t data[6];
} iis3dwb_fifo_out_raw_t;

But even in this way, I'm not sure if the starting address of a iis3dwb_fifo_out_raw_t variable would remain byte-aligned, or it would be chnaged to int-aligned.

The problem is that

https://github.com/STMicroelectronics/iis3dwb-pid/blob/7dac6d5f408aa7a7ea18721c5a77b224f35b13ff/iis3dwb_reg.c#L961-L973

reads 7 * num bytes and stores them into fdata which is an array of num iis3dwb_fifo_out_raw_t variables. This massive read works only if iis3dwb_fifo_out_raw_t are stored into memory byte-aligned. The topic is quite technical, and I hope I've been able to describe it.

Hi @escherstair ,

Yes, the topic is quite technical and I agree with you that we might have issue related to different size/alignment on various architecture. The usage of enum was introduced only for the sake of clearness in the code, but I agree that it might work not as expected. I was in fact assuming that the size of an enumerative type was the minimum enough to represent it.

If you don't mind I'm available to evaluate your proposal in terms of a Pull Request. Thanks again!

[escherstair]

@avisconti I did it, but the PR is refused. I signed the CLA when requested

[cparata]

Hello @avisconti and @escherstair , I proposed a PR to fix the issue. Let me know what you think about it. Best Regards, Carlo

[avisconti]

Closed via PR #9