| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <60291521-2f71-8933-e90d-95b84c5f0be8@embeddedor.com>
Date: Mon, 5 Apr 2021 06:27:58 -0500
From: "Gustavo A. R. Silva" <gustavo@...eddedor.com>
To: Avri Altman <Avri.Altman@....com>,
"Gustavo A. R. Silva" <gustavoars@...nel.org>,
Alim Akhtar <alim.akhtar@...sung.com>,
"James E.J. Bottomley" <jejb@...ux.ibm.com>,
"Martin K. Petersen" <martin.petersen@...cle.com>
Cc: "linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-hardening@...r.kernel.org" <linux-hardening@...r.kernel.org>
Subject: Re: [PATCH][next] scsi: ufs: Fix out-of-bounds warnings in
ufshcd_exec_raw_upiu_cmd
On 4/3/21 14:44, Avri Altman wrote:
>>
>> Fix the following out-of-bounds warnings by enclosing
>> some structure members into new structure objects upiu_req
>> and upiu_rsp:
>>
>> include/linux/fortify-string.h:20:29: warning: '__builtin_memcpy' offset [29,
>> 48] from the object at 'treq' is out of the bounds of referenced subobject
>> 'req_header' with type 'struct utp_upiu_header' at offset 16 [-Warray-bounds]
>> include/linux/fortify-string.h:20:29: warning: '__builtin_memcpy' offset [61,
>> 80] from the object at 'treq' is out of the bounds of referenced subobject
>> 'rsp_header' with type 'struct utp_upiu_header' at offset 48 [-Warray-bounds]
>> arch/m68k/include/asm/string.h:72:25: warning: '__builtin_memcpy' offset
>> [29, 48] from the object at 'treq' is out of the bounds of referenced subobject
>> 'req_header' with type 'struct utp_upiu_header' at offset 16 [-Warray-bounds]
>> arch/m68k/include/asm/string.h:72:25: warning: '__builtin_memcpy' offset
>> [61, 80] from the object at 'treq' is out of the bounds of referenced subobject
>> 'rsp_header' with type 'struct utp_upiu_header' at offset 48 [-Warray-bounds]
>>
>> Refactor the code by making it more structured.
>>
>> The problem is that the original code is trying to copy data into a
>> bunch of struct members adjacent to each other in a single call to
>> memcpy(). Now that a new struct _upiu_req_ enclosing all those adjacent
>> members is introduced, memcpy() doesn't overrun the length of
>> &treq.req_header, because the address of the new struct object
>> _upiu_req_ is used as the destination, instead. The same problem
>> is present when memcpy() overruns the length of the source
>> &treq.rsp_header; in this case the address of the new struct
>> object _upiu_rsp_ is used, instead.
>>
>> Also, this helps with the ongoing efforts to enable -Warray-bounds
>> and avoid confusing the compiler.
>>
>> Link: https://github.com/KSPP/linux/issues/109
>> Reported-by: kernel test robot <lkp@...el.com>
>> Build-tested-by: kernel test robot <lkp@...el.com>
>> Link:
>> https://lore.kernel.org/lkml/60640558.lsAxiK6otPwTo9rv%25lkp@intel.com/
>> Signed-off-by: Gustavo A. R. Silva <gustavoars@...nel.org>
> Reviewed-by: Avri Altman <avri.altman@....com>
Thanks, Avri. :)
--
Gustavo
>
> Thanks,
> Avri
>
>> ---
>> drivers/scsi/ufs/ufshcd.c | 28 ++++++++++++++++------------
>> drivers/scsi/ufs/ufshci.h | 22 +++++++++++++---------
>> 2 files changed, 29 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 7539a4ee9494..324eb641e66f 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -336,11 +336,15 @@ static void ufshcd_add_tm_upiu_trace(struct
>> ufs_hba *hba, unsigned int tag,
>> return;
>>
>> if (str_t == UFS_TM_SEND)
>> - trace_ufshcd_upiu(dev_name(hba->dev), str_t, &descp->req_header,
>> - &descp->input_param1, UFS_TSF_TM_INPUT);
>> + trace_ufshcd_upiu(dev_name(hba->dev), str_t,
>> + &descp->upiu_req.req_header,
>> + &descp->upiu_req.input_param1,
>> + UFS_TSF_TM_INPUT);
>> else
>> - trace_ufshcd_upiu(dev_name(hba->dev), str_t, &descp->rsp_header,
>> - &descp->output_param1, UFS_TSF_TM_OUTPUT);
>> + trace_ufshcd_upiu(dev_name(hba->dev), str_t,
>> + &descp->upiu_rsp.rsp_header,
>> + &descp->upiu_rsp.output_param1,
>> + UFS_TSF_TM_OUTPUT);
>> }
>>
>> static void ufshcd_add_uic_command_trace(struct ufs_hba *hba,
>> @@ -6420,7 +6424,7 @@ static int __ufshcd_issue_tm_cmd(struct ufs_hba
>> *hba,
>> spin_lock_irqsave(host->host_lock, flags);
>> task_tag = hba->nutrs + free_slot;
>>
>> - treq->req_header.dword_0 |= cpu_to_be32(task_tag);
>> + treq->upiu_req.req_header.dword_0 |= cpu_to_be32(task_tag);
>>
>> memcpy(hba->utmrdl_base_addr + free_slot, treq, sizeof(*treq));
>> ufshcd_vops_setup_task_mgmt(hba, free_slot, tm_function);
>> @@ -6493,16 +6497,16 @@ static int ufshcd_issue_tm_cmd(struct ufs_hba
>> *hba, int lun_id, int task_id,
>> treq.header.dword_2 = cpu_to_le32(OCS_INVALID_COMMAND_STATUS);
>>
>> /* Configure task request UPIU */
>> - treq.req_header.dword_0 = cpu_to_be32(lun_id << 8) |
>> + treq.upiu_req.req_header.dword_0 = cpu_to_be32(lun_id << 8) |
>> cpu_to_be32(UPIU_TRANSACTION_TASK_REQ << 24);
>> - treq.req_header.dword_1 = cpu_to_be32(tm_function << 16);
>> + treq.upiu_req.req_header.dword_1 = cpu_to_be32(tm_function << 16);
>>
>> /*
>> * The host shall provide the same value for LUN field in the basic
>> * header and for Input Parameter.
>> */
>> - treq.input_param1 = cpu_to_be32(lun_id);
>> - treq.input_param2 = cpu_to_be32(task_id);
>> + treq.upiu_req.input_param1 = cpu_to_be32(lun_id);
>> + treq.upiu_req.input_param2 = cpu_to_be32(task_id);
>>
>> err = __ufshcd_issue_tm_cmd(hba, &treq, tm_function);
>> if (err == -ETIMEDOUT)
>> @@ -6513,7 +6517,7 @@ static int ufshcd_issue_tm_cmd(struct ufs_hba *hba,
>> int lun_id, int task_id,
>> dev_err(hba->dev, "%s: failed, ocs = 0x%x\n",
>> __func__, ocs_value);
>> else if (tm_response)
>> - *tm_response = be32_to_cpu(treq.output_param1) &
>> + *tm_response = be32_to_cpu(treq.upiu_rsp.output_param1) &
>> MASK_TM_SERVICE_RESP;
>> return err;
>> }
>> @@ -6693,7 +6697,7 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba
>> *hba,
>> treq.header.dword_0 = cpu_to_le32(UTP_REQ_DESC_INT_CMD);
>> treq.header.dword_2 =
>> cpu_to_le32(OCS_INVALID_COMMAND_STATUS);
>>
>> - memcpy(&treq.req_header, req_upiu, sizeof(*req_upiu));
>> + memcpy(&treq.upiu_req, req_upiu, sizeof(*req_upiu));
>>
>> err = __ufshcd_issue_tm_cmd(hba, &treq, tm_f);
>> if (err == -ETIMEDOUT)
>> @@ -6706,7 +6710,7 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba
>> *hba,
>> break;
>> }
>>
>> - memcpy(rsp_upiu, &treq.rsp_header, sizeof(*rsp_upiu));
>> + memcpy(rsp_upiu, &treq.upiu_rsp, sizeof(*rsp_upiu));
>>
>> break;
>> default:
>> diff --git a/drivers/scsi/ufs/ufshci.h b/drivers/scsi/ufs/ufshci.h
>> index 6795e1f0e8f8..235236859285 100644
>> --- a/drivers/scsi/ufs/ufshci.h
>> +++ b/drivers/scsi/ufs/ufshci.h
>> @@ -482,17 +482,21 @@ struct utp_task_req_desc {
>> struct request_desc_header header;
>>
>> /* DW 4-11 - Task request UPIU structure */
>> - struct utp_upiu_header req_header;
>> - __be32 input_param1;
>> - __be32 input_param2;
>> - __be32 input_param3;
>> - __be32 __reserved1[2];
>> + struct {
>> + struct utp_upiu_header req_header;
>> + __be32 input_param1;
>> + __be32 input_param2;
>> + __be32 input_param3;
>> + __be32 __reserved1[2];
>> + } upiu_req;
>>
>> /* DW 12-19 - Task Management Response UPIU structure */
>> - struct utp_upiu_header rsp_header;
>> - __be32 output_param1;
>> - __be32 output_param2;
>> - __be32 __reserved2[3];
>> + struct {
>> + struct utp_upiu_header rsp_header;
>> + __be32 output_param1;
>> + __be32 output_param2;
>> + __be32 __reserved2[3];
>> + } upiu_rsp;
>> };
>>
>> #endif /* End of Header */
>> --
>> 2.27.0
>
Powered by blists - more mailing lists