Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: yZpZUtLl4uacUWbagOLW3V8OW1lefRKqeZhPjl36Zz9LCrW8f5Aosa0Kxq38Xo290hJWCYRMRj
 gdQRIhuD5ARxPROTqpDHbMNjbMRnccyo8D4NH81pwrfUU93zC8lxwcKjEMdAdKND91dOTrLNk+
 /SrbrfGJr+Ho5WnFA20KCEE67N21YyikjTKwzYpFYeYPHacqRhMtlrdDF9V5/vWWuch9RfULL5
 KdS0aRln8Ffx+OZiHuTpVSBbvGTlJuEEbwEDAhsmTbOHQJIQfoSwgpF0CUk+q8U8/BXZEByVp1
 y3U=
From:   Avri Altman <Avri.Altman@wdc.com>
To:     "Gustavo A. R. Silva" <gustavoars@kernel.org>,
        Alim Akhtar <alim.akhtar@samsung.com>,
        "James E.J. Bottomley" <jejb@linux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@oracle.com>
CC:     "linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-hardening@vger.kernel.org" <linux-hardening@vger.kernel.org>
Subject: RE: [PATCH][next] scsi: ufs: Fix out-of-bounds warnings in
 ufshcd_exec_raw_upiu_cmd
Thread-Topic: [PATCH][next] scsi: ufs: Fix out-of-bounds warnings in
 ufshcd_exec_raw_upiu_cmd
Thread-Index: AQHXJoeuCthvokHAnk+y1L1bOvWX4aqjNDKA
Date:   Sat, 3 Apr 2021 19:44:32 +0000
Message-ID: <DM6PR04MB65751397A9CCA9B677875B3EFC799@DM6PR04MB6575.namprd04.prod.outlook.com>
References: <20210331224338.GA347171@embeddedor>
In-Reply-To: <20210331224338.GA347171@embeddedor>
Accept-Language: en-US
Content-Language: en-US
wdcipoutbound: EOP-TRUE
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: f0b17007-8080-4cf9-25fe-08d8f6d8e737
X-MS-Exchange-CrossTenant-originalarrivaltime: 03 Apr 2021 19:44:32.4061
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b61c8803-16f3-4c35-9b17-6f65f441df86
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: HCyN7bqNz0uRItPH02mtM3GIllNgxo142237/27ypHomrpLg2LqDkqfZUMS4i8neq8voBNlpb1qjgMoLp68Uag==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR04MB5626
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

>=20
> Fix the following out-of-bounds warnings by enclosing
> some structure members into new structure objects upiu_req
> and upiu_rsp:
>=20
> 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 subobjec=
t
> 'req_header' with type 'struct utp_upiu_header' at offset 16 [-Warray-bou=
nds]
> 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 subobjec=
t
> 'rsp_header' with type 'struct utp_upiu_header' at offset 48 [-Warray-bou=
nds]
> 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 sub=
object
> 'req_header' with type 'struct utp_upiu_header' at offset 16 [-Warray-bou=
nds]
> 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 sub=
object
> 'rsp_header' with type 'struct utp_upiu_header' at offset 48 [-Warray-bou=
nds]
>=20
> Refactor the code by making it more structured.
>=20
> 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.
>=20
> Also, this helps with the ongoing efforts to enable -Warray-bounds
> and avoid confusing the compiler.
>=20
> Link: https://github.com/KSPP/linux/issues/109
> Reported-by: kernel test robot <lkp@intel.com>
> Build-tested-by: kernel test robot <lkp@intel.com>
> Link:
> https://lore.kernel.org/lkml/60640558.lsAxiK6otPwTo9rv%25lkp@intel.com/
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
Reviewed-by: Avri Altman <avri.altman@wdc.com>

Thanks,
Avri

> ---
>  drivers/scsi/ufs/ufshcd.c | 28 ++++++++++++++++------------
>  drivers/scsi/ufs/ufshci.h | 22 +++++++++++++---------
>  2 files changed, 29 insertions(+), 21 deletions(-)
>=20
> 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;
>=20
>         if (str_t =3D=3D 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_OUTPU=
T);
> +               trace_ufshcd_upiu(dev_name(hba->dev), str_t,
> +                                 &descp->upiu_rsp.rsp_header,
> +                                 &descp->upiu_rsp.output_param1,
> +                                 UFS_TSF_TM_OUTPUT);
>  }
>=20
>  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 =3D hba->nutrs + free_slot;
>=20
> -       treq->req_header.dword_0 |=3D cpu_to_be32(task_tag);
> +       treq->upiu_req.req_header.dword_0 |=3D cpu_to_be32(task_tag);
>=20
>         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 =3D cpu_to_le32(OCS_INVALID_COMMAND_STATUS);
>=20
>         /* Configure task request UPIU */
> -       treq.req_header.dword_0 =3D cpu_to_be32(lun_id << 8) |
> +       treq.upiu_req.req_header.dword_0 =3D cpu_to_be32(lun_id << 8) |
>                                   cpu_to_be32(UPIU_TRANSACTION_TASK_REQ <=
< 24);
> -       treq.req_header.dword_1 =3D cpu_to_be32(tm_function << 16);
> +       treq.upiu_req.req_header.dword_1 =3D cpu_to_be32(tm_function << 1=
6);
>=20
>         /*
>          * The host shall provide the same value for LUN field in the bas=
ic
>          * header and for Input Parameter.
>          */
> -       treq.input_param1 =3D cpu_to_be32(lun_id);
> -       treq.input_param2 =3D cpu_to_be32(task_id);
> +       treq.upiu_req.input_param1 =3D cpu_to_be32(lun_id);
> +       treq.upiu_req.input_param2 =3D cpu_to_be32(task_id);
>=20
>         err =3D __ufshcd_issue_tm_cmd(hba, &treq, tm_function);
>         if (err =3D=3D -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 =3D 0x%x\n",
>                                 __func__, ocs_value);
>         else if (tm_response)
> -               *tm_response =3D be32_to_cpu(treq.output_param1) &
> +               *tm_response =3D 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 =3D cpu_to_le32(UTP_REQ_DESC_INT_CMD)=
;
>                 treq.header.dword_2 =3D
> cpu_to_le32(OCS_INVALID_COMMAND_STATUS);
>=20
> -               memcpy(&treq.req_header, req_upiu, sizeof(*req_upiu));
> +               memcpy(&treq.upiu_req, req_upiu, sizeof(*req_upiu));
>=20
>                 err =3D __ufshcd_issue_tm_cmd(hba, &treq, tm_f);
>                 if (err =3D=3D -ETIMEDOUT)
> @@ -6706,7 +6710,7 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba
> *hba,
>                         break;
>                 }
>=20
> -               memcpy(rsp_upiu, &treq.rsp_header, sizeof(*rsp_upiu));
> +               memcpy(rsp_upiu, &treq.upiu_rsp, sizeof(*rsp_upiu));
>=20
>                 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;
>=20
>         /* 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;
>=20
>         /* 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;
>  };
>=20
>  #endif /* End of Header */
> --
> 2.27.0