On 2020-05-11 00:36, Arun Easi wrote:
> On Sun, 26 Apr 2020, 8:03pm, Bart Van Assche wrote:
>
>> This patch fixes the following Coverity complaint without changing any
>> functionality:
>>
>> CID 337793 (#1 of 1): Wrong size argument (SIZEOF_MISMATCH)
>> suspicious_sizeof: Passing argument ha->fcp_prio_cfg of type
>> struct qla_fcp_prio_cfg * and argument 32768UL to function memset is
>> suspicious because a multiple of sizeof (struct qla_fcp_prio_cfg) /*48*/
>> is expected.
>>
>> memset(ha->fcp_prio_cfg, 0, FCP_PRIO_CFG_SIZE);
>>
>> Cc: Nilesh Javali <njavali@xxxxxxxxxxx>
>> Cc: Himanshu Madhani <himanshu.madhani@xxxxxxxxxx>
>> Cc: Quinn Tran <qutran@xxxxxxxxxxx>
>> Cc: Martin Wilck <mwilck@xxxxxxxx>
>> Cc: Daniel Wagner <dwagner@xxxxxxx>
>> Cc: Roman Bolshakov <r.bolshakov@xxxxxxxxx>
>> Signed-off-by: Bart Van Assche <bvanassche@xxxxxxx>
>> ---
>> drivers/scsi/qla2xxx/qla_fw.h | 3 ++-
>> drivers/scsi/qla2xxx/qla_os.c | 1 +
>> 2 files changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/scsi/qla2xxx/qla_fw.h b/drivers/scsi/qla2xxx/qla_fw.h
>> index b364a497e33d..4fa34374f34f 100644
>> --- a/drivers/scsi/qla2xxx/qla_fw.h
>> +++ b/drivers/scsi/qla2xxx/qla_fw.h
>> @@ -2217,8 +2217,9 @@ struct qla_fcp_prio_cfg {
>> #define FCP_PRIO_ATTR_PERSIST 0x2
>> uint8_t reserved; /* Reserved for future use */
>> #define FCP_PRIO_CFG_HDR_SIZE 0x10
>> - struct qla_fcp_prio_entry entry[1]; /* fcp priority entries */
>> + struct qla_fcp_prio_entry entry[1023]; /* fcp priority entries */
>> #define FCP_PRIO_CFG_ENTRY_SIZE 0x20
>> + uint8_t reserved2[16];
>> };
>>
>> #define FCP_PRIO_CFG_SIZE (32*1024) /* fcp prio data per port*/
>> diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
>> index 2dd9c2a39cd5..30c2750c5745 100644
>> --- a/drivers/scsi/qla2xxx/qla_os.c
>> +++ b/drivers/scsi/qla2xxx/qla_os.c
>> @@ -7877,6 +7877,7 @@ qla2x00_module_init(void)
>> BUILD_BUG_ON(sizeof(struct qla82xx_uri_data_desc) != 28);
>> BUILD_BUG_ON(sizeof(struct qla82xx_uri_table_desc) != 32);
>> BUILD_BUG_ON(sizeof(struct qla83xx_fw_dump) != 51196);
>> + BUILD_BUG_ON(sizeof(struct qla_fcp_prio_cfg) != FCP_PRIO_CFG_SIZE);
>> BUILD_BUG_ON(sizeof(struct qla_fdt_layout) != 128);
>> BUILD_BUG_ON(sizeof(struct qla_flt_header) != 8);
>> BUILD_BUG_ON(sizeof(struct qla_flt_region) != 16);
>>
>
> The changes themselves look ok, but..
>
> Could the warning be avoided by memset of FCP_PRIO_CFG_HDR_SIZE
> before first read_optrom(), and another memset of
> "FCP_PRIO_CFG_SIZE - FCP_PRIO_CFG_HDR_SIZE" before second
> read_optrom() call?
>
> The reason I ask is that, the kind of "1" element array
> declaration in a struct is a common way of mapping a header
> followed by N records of some nature. It is a bit sad if we are
> moving away from that style and hard computing the structure by hand.
Coverity would definitely complain about calling memset() to clear
multiple array elements while the array declaration only specifies a
single element.
BTW, the style that is currently preferred in the Linux kernel for
declaring flexible arrays is to use [] instead of [1]. See e.g. the
following commit:
commit 1a91a36aba9c232c73e4a5fce038147f5d29e566
Author: Gustavo A. R. Silva <gustavo@xxxxxxxxxxxxxx>
Date: Wed Feb 26 16:31:25 2020 -0600
mmc: Replace zero-length array with flexible-array member
The current codebase makes use of the zero-length array language
extension to the C90 standard, but the preferred mechanism to declare
variable-length types such as these ones is a flexible array
member[1][2], introduced in C99:
struct foo {
int stuff;
struct boo array[];
};
By making use of the mechanism above, we will get a compiler warning
in case the flexible array does not occur last in the structure, which
will help us prevent some kind of undefined behavior bugs from being
inadvertently introduced[3] to the codebase from now on.
Also, notice that, dynamic memory allocations won't be affected by
this change:
"Flexible array members have incomplete type, and so the sizeof
operator may not be applied. As a quirk of the original implementation
of zero-length arrays, sizeof evaluates to zero."[1]
This issue was found with the help of Coccinelle.
[1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
[2] https://github.com/KSPP/linux/issues/21
[3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
Signed-off-by: Gustavo A. R. Silva <gustavo@xxxxxxxxxxxxxx>
Acked-by: Adrian Hunter <adrian.hunter@xxxxxxxxx>
Link: https://lore.kernel.org/r/20200226223125.GA20630@embeddedor
Signed-off-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
[ ... ]
Bart.
