Coverity static analysis code defect

tcpip picture tcpip · May 5, 2014 · Viewed 7.7k times · Source

We use Coverity to detect vulnerabilities in our code. Basically this is the code snippet:

static int vendor_request(
      const struct OFPHDR *oh, 
      size_t length, 
      const struct OFPUTIL_MT **typep
      )
   {
      const struct OFPSM *osr;
      ovs_be32 vendor;

      osr = (const struct OFPSM *) oh;

      memcpy(&vendor,  ((char*)osr + sizeof(struct OFPSM)), sizeof( vendor ));
      if (vendor == htonl(VENDOR_A))
         return (functionA(oh, typep));
      if (vendor == htonl(VENDOR_B))
         return (functionB(oh, length, typep));
      else
         return 0;
   }

Here,

sizeof(struct OFPSM) = 12 bytes.

sizeof(struct OFPHDR) = 8 bytes.

Coverity says:

CID xxxxx (#1 of 2): Out-of-bounds access (OVERRUN)   
1. overrun-buffer-val: Overrunning struct type OFPHDR of 8 bytes by passing it to a function which accesses it at byte offset 12. Pointer osr indexed by constant 12U through dereference in call to memcpy.

Basically struct OFPHDR is a PDU on top of TCP layer, it's size is 8 bytes but it can vary depending upon what type of OFP message it is. Coverity says that I'm dereferencing *oh at byte offset index 12 which is out-bound-access index.

But I don't understand the problem since I'm typecasting OFPHDR to proper structure which is of 12 bytes and then dereferencing it. So, how could this error be avoided?

Answer

PeterSW picture PeterSW · May 5, 2014

This cast:

osr = (const struct OFPSM *) oh;

is breaking the strict aliasing rules since it is casting to an incompatible type. It's clear they are incompatible since you say:

sizeof(struct OFPSM) = 12 bytes.

sizeof(struct OFPHDR) = 8 bytes.