Fix problem with size of allocation in libsas

From: Jesper Juhl <[email protected]>

in sas_get_phy_change_count(), the line

	disc_resp = alloc_smp_resp(DISCOVER_RESP_SIZE);
will allocate 56 bytes due to this define:
	#define DISCOVER_RESP_SIZE 56

But, the struct is actually 60 bytes in size.

So change the define to be

	#define DISCOVER_RESP_SIZE sizeof(struct smp_resp)

so we always get the correct size even when people fiddle with the structure.

This change also fixes the same problem in
sas_get_phy_attached_sas_addr()

(Found by the Coverity checker. Compile tested only)

Quoted 16 lines · Jesper Juhl

From: Jesper Juhl <[email protected]>

in sas_get_phy_change_count(), the line

      disc_resp = alloc_smp_resp(DISCOVER_RESP_SIZE);
will allocate 56 bytes due to this define:
      #define DISCOVER_RESP_SIZE 56

But, the struct is actually 60 bytes in size.

So change the define to be

      #define DISCOVER_RESP_SIZE sizeof(struct smp_resp)

so we always get the correct size even when people fiddle with the structure.

This change also fixes the same problem in
sas_get_phy_attached_sas_addr()

Quoted 1 line

(Found by the Coverity checker. Compile tested only)

Well, your fix is definitely wrong.

Could you explain the problem a little more? The discover response SMP frame is 56 bytes as mandated by the standard. I don't see anywhere in the code where we're actually using a value beyond the 56th byte ... where is the problem use?

Quoted 17 lines · James Bottomley

Quoted 16 lines · Jesper Juhl

From: Jesper Juhl <[email protected]>

in sas_get_phy_change_count(), the line

      disc_resp = alloc_smp_resp(DISCOVER_RESP_SIZE);
will allocate 56 bytes due to this define:
      #define DISCOVER_RESP_SIZE 56

But, the struct is actually 60 bytes in size.

So change the define to be

      #define DISCOVER_RESP_SIZE sizeof(struct smp_resp)

so we always get the correct size even when people fiddle with the structure.

This change also fixes the same problem in
sas_get_phy_attached_sas_addr()

Quoted 9 lines

Quoted 1 line

(Found by the Coverity checker. Compile tested only)

Well, your fix is definitely wrong.

Could you explain the problem a little more? The discover response SMP frame is 56 bytes as mandated by the standard. I don't see anywhere in the code where we're actually using a value beyond the 56th byte ... where is the problem use?

I haven't found any actual problem *use*, I just looked at the size of 'struct smp_resp' and noticed that coverity seemed to be right that 56 bytes are not sufficient to hold the members of the struct.

There are
32 bytes in the first members + the union and I don't see how that can ever stay at 56 bytes...? So, we are allocating memory and storing it in a 'struct smp_resp *', but we are allocating less than sizeof(smp_resp) - how is that not a (potential) problem?

Quoted 28 lines · Jesper Juhl

Quoted 17 lines · James Bottomley

Quoted 16 lines · Jesper Juhl

From: Jesper Juhl <[email protected]>

in sas_get_phy_change_count(), the line

      disc_resp = alloc_smp_resp(DISCOVER_RESP_SIZE);
will allocate 56 bytes due to this define:
      #define DISCOVER_RESP_SIZE 56

But, the struct is actually 60 bytes in size.

So change the define to be

      #define DISCOVER_RESP_SIZE sizeof(struct smp_resp)

so we always get the correct size even when people fiddle with the structure.

This change also fixes the same problem in
sas_get_phy_attached_sas_addr()

Quoted 9 lines

Quoted 1 line

(Found by the Coverity checker. Compile tested only)

Well, your fix is definitely wrong.

Could you explain the problem a little more? The discover response SMP frame is 56 bytes as mandated by the standard. I don't see anywhere in the code where we're actually using a value beyond the 56th byte ... where is the problem use?

Quoted 3 lines

I haven't found any actual problem *use*, I just looked at the size of 'struct smp_resp' and noticed that coverity seemed to be right that 56 bytes are not sufficient to hold the members of the struct.

There are 11 current (well, as of the 1.1 standard) SMP frame responses. Each of them is actually a different length. This driver treats SMP frame response underruns as errors, so the fix you propose would cause the whole discovery process to collapse.

Quoted 5 lines

There are
32 bytes in the first members + the union and I don't see how that can ever stay at 56 bytes...? So, we are allocating memory and storing it in a 'struct smp_resp *', but we are allocating less than sizeof(smp_resp) - how is that not a (potential) problem?

We're not storing anything. We're allocating a byte area for the driver to deposit a response frame, we know we just sent a SMP DISCOVER request, so we'd better get a discover response back (and the driver will error on underrun or overrun, so we know if it returns successfully that's exatly what we have). The struct smp_resp contains the SMP invariant header and then a union of all the other response data types, so as long as we use either the header or the disc piece of the union, there's no possibility of error, is there?

Quoted 16 lines · Jesper Juhl

From: Jesper Juhl <[email protected]>

in sas_get_phy_change_count(), the line

	disc_resp = alloc_smp_resp(DISCOVER_RESP_SIZE);
will allocate 56 bytes due to this define:
	#define DISCOVER_RESP_SIZE 56

But, the struct is actually 60 bytes in size.

So change the define to be

	#define DISCOVER_RESP_SIZE sizeof(struct smp_resp)

so we always get the correct size even when people fiddle with the structure.

This change also fixes the same problem in
sas_get_phy_attached_sas_addr()

Quoted 1 line

(Found by the Coverity checker. Compile tested only)

Well, your fix is definitely wrong.

Could you explain the problem a little more? The discover response SMP frame is 56 bytes as mandated by the standard. I don't see anywhere in the code where we're actually using a value beyond the 56th byte ... where is the problem use?

Quoted 17 lines · James Bottomley

Quoted 16 lines · Jesper Juhl

From: Jesper Juhl <[email protected]>

in sas_get_phy_change_count(), the line

	disc_resp = alloc_smp_resp(DISCOVER_RESP_SIZE);
will allocate 56 bytes due to this define:
	#define DISCOVER_RESP_SIZE 56

But, the struct is actually 60 bytes in size.

So change the define to be

	#define DISCOVER_RESP_SIZE sizeof(struct smp_resp)

so we always get the correct size even when people fiddle with the structure.

This change also fixes the same problem in
sas_get_phy_attached_sas_addr()

Quoted 8 lines

Quoted 1 line

(Found by the Coverity checker. Compile tested only)

Well, your fix is definitely wrong.

Could you explain the problem a little more? The discover response SMP frame is 56 bytes as mandated by the standard. I don't see anywhere in the code where we're actually using a value beyond the 56th byte ... where is the problem use?

The response size of SMP DISCOVER keeps growing with each rev. Currently in SAS-2 revision 12 it is 112 bytes long! The original SAS standard and SAS 1.1 have implicit response sizes and for DISCOVER that is 56 bytes.

To be compliant with SAS-2 the code should read byte 3 of a DISCOVER response. If it is zero then the response length is 56 bytes, otherwise the response length is that many dwords (i.e. 4 byte units) plus 4 bytes for the CRC. [Similar logic applies to many other SMP responses.]

I have tables for all SMP requests and response (up until SAS-2 rev 12) in my smp_utils package, see the smp_lib.c file.