Message-ID: <6d2fe281-891c-8909-5330-f05d4407431a@cornelisnetworks.com>
Date:   Tue, 21 Feb 2023 08:00:12 -0500
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.8.0
Subject: Re: [PATCH] IB/rdmavt: Fix target union member for rvt_post_one_wr()
To:     Kees Cook <keescook@chromium.org>
Cc:     Zhang Yi <yizhan@redhat.com>, Jason Gunthorpe <jgg@ziepe.ca>,
        Leon Romanovsky <leon@kernel.org>, linux-rdma@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org
References: <20230218185701.never.779-kees@kernel.org>
Content-Language: en-US
From:   Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>
In-Reply-To: <20230218185701.never.779-kees@kernel.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?NHNvTVFuNkJwYzlIZS9NcGhMRW5Hckg3R0tjL3NxQVRSeVNOaXNmcnN1bDJJ?=
 =?utf-8?B?OG1LUlhQdHdlZFMwRXdmSkZGWmFaUlo3WlpMeHluS2Nlbzd5TUY0VmQwNjZT?=
 =?utf-8?B?ZXk0enk1VUh1N2lPOFV3MDF2SHh5UlJJcncyc0ZqREtpWFFhU1ozUkJVQnRC?=
 =?utf-8?B?UlNNYm5nTmN6QlFIUnJkdXFsTFo4cXMvZTlWYkxSdjhyaVhJS0pHbWkxU0Nw?=
 =?utf-8?B?VnBTYkd6MDlPZGxEYm5ieFBlcGFCamxaaHJXUVZVa2hQMlRLMGlaeStYMEhC?=
 =?utf-8?B?bkp6amRyVnRURE4wMlZoZ2MxT3pDZzZFcUNmR0NHNzg3cFZUTkxuUGNiR2ZR?=
 =?utf-8?B?eTBSL2Rta1hUdVBIVmxHc1JHYW43dWxHQ0xKdzFSbDlySk5paXR2NlZQMmU1?=
 =?utf-8?B?L2FvUGdwMUsyd1BleHNjdVN1ZWJXOW95c2s0cXpWcDdjT2UzRVhqSVR5b25R?=
 =?utf-8?B?SVorU1hkOVdldnRuUGk3MlM5UWVVdE1OekppZDVvQ0RvSFNQTHlVOTVhcXJz?=
 =?utf-8?B?TXV3cmd1MzlRMGlTUWlKMU1taXJycFlVRlBWcUtqaTdEZ00xeWh5ZlZUUCtY?=
 =?utf-8?B?aThyOVhWcVFldllsTlpSTC9nUkxVcDRZY1E3WUd1T1pkU2U1cXhoOE9KL3ZM?=
 =?utf-8?B?dWY0VWt3c0FSNUc0QmdVM1RYc3F1OVFzVkY4d0FMck03NDFsOTJNZmtrTU1s?=
 =?utf-8?B?NXk3eWpUYi9wckNJQU5waHFLUG9YWnFZT0c1UkFuRi9sT0F4M0Myd29vRncz?=
 =?utf-8?B?dldOOWZhZ2lVampJZjkzNEdIODFFWGpFRXZyMldhWm9Celc5ZnY3d3M3UUVV?=
 =?utf-8?B?bkJ5UWtUcWVDMWgxMWZVdkppeDcwL0tYa2Q0ejNKanNOaVFXSWZKeDJTdExE?=
 =?utf-8?B?NzgvS1hPM25ERFdmV0Iyd0hwdkllbmFpZTg3enpjM0dCRUdpYlQveEVvTUxY?=
 =?utf-8?B?cHd0T1RmT09VVjI2ZkFFL21IaUdiSlBuUG0xQVpnQUZhYUlIbkFwdFdybEM4?=
 =?utf-8?B?M244MUREb1JqVS9ITGpHSWlzZXpqc3h0aEdheW92Y0d0dld6Qkw2aDRyRGF3?=
 =?utf-8?B?ZW5yV3NzWDVuUUFjaS9zYmpVZDRnemtRemFDelRoaWVrUFQwVzhXR2oxTE9J?=
 =?utf-8?B?QVdJL0hpWXRjNUN5ZDhhcjlYZFpDelZJR29hdTBZUFpxL0s1WmoyMFZBdDNH?=
 =?utf-8?B?VUpZRWVZdjF2aDhnNzVYM2w3eW0rVEdFaDZqaXArSUY1amFZS2F4T2ltNTha?=
 =?utf-8?B?ZklGdU9ubUR6YTVjbWNzdUhabWpXWDltRHZZVG9UNUlwTjZKS1ZQVVNCa00r?=
 =?utf-8?B?dm0vOXRhQTRmRlVhV3FBaUJhcnY2M3BHckVMTlpiVlhMSkxPTSsxSm5GR0J6?=
 =?utf-8?B?dUFGMjBHdnJuak5kbWVlVGt1Y2EwMDhUbDVIS2hkTENZQ1JYUWFjeTR0NVJr?=
 =?utf-8?B?UEtGNTZhY0Zjd2pqUnVHay9oVFZ2ZWRZSXJUWEcvUDFlVGI0ZjZLUkVZYklY?=
 =?utf-8?B?alFBWjVJbE5wUWN3elZvTFVENWdCbmJ1OFkrVHB1TDdOQURKSzdwUkxuUDBR?=
 =?utf-8?B?MUpSWm9UdyticUNpeUhoa2xYVjRsY3FKMjhaOFNNK21JcTlKS1lUdEwrTDI1?=
 =?utf-8?B?OFIyZ0pqRWFHQlA3d0l3Q2ZiS1VsRGk1VndiVGdDM1hxU1BYbVkwOW90M2dD?=
 =?utf-8?B?ZFhzK2xUZW91S3N3L3VUSzRpa0JqNE1LdWoreUQ0dTFndUp0a3FOUHhnQmR2?=
 =?utf-8?B?MDV0ay9SWkI3SXAvWVVTciswUFJMSU04RjFncXN1SllCeVpGSXV0YW5aUFBo?=
 =?utf-8?B?dGQ0bmQyVjUydlM2N1hPZ0tWN1V6SWFMTXlMU3BTOFd1ZmdhVzRRRVRJdDRU?=
 =?utf-8?B?ZkZpZHI1Q2JtVlNqeFYyZktCVWkzcEJXMzhTckZJY0VkYXloKzYyazdaUWFn?=
 =?utf-8?B?NFJoMXJMUXQwclNXSEEwV2gzVEVlQnMxOEFROE5xeTVjOGZ5a2g4TURvcHg4?=
 =?utf-8?B?T0lLOVBjR0Zzbm5DcWdWMFJzMlFhUU96N0dGNS81R2xLNCtZLzJta2Q3ZUsz?=
 =?utf-8?B?S2pWb3BBa2wzYkNLT3lOVDBvMUJwbFM1YXZEbHFoZm1rWHJkZ1cxOWZNMEh5?=
 =?utf-8?B?a1U5TVp4RG1scEFqQ1NscGJoVGhCZytHVlU3YWtNTmtJaFgvSVJkMytYcjM4?=
 =?utf-8?Q?T+qTOdzsz50wTgk4+Hf0Jes=3D?=
X-OriginatorOrg: cornelisnetworks.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 6bf2d972-f835-4d17-3fcd-08db140b939a
X-MS-Exchange-CrossTenant-AuthSource: BN6PR01MB2610.prod.exchangelabs.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Feb 2023 13:00:15.8487
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4dbdb7da-74ee-4b45-8747-ef5ce5ebe68a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: eTaHnkB0zyPG8059MaB5Sj721osXJAMty9mLTSAQM2CogQiKefIqsvRSM/cz5qJySIg4CctKc5ldD5NlTCEdWiGEQGi0+2eTrh60SfHM0U7ZgeLXXCoHY3olJWz/3JdV
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR01MB8202
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 2/18/23 1:57 PM, Kees Cook wrote:
> The "cplen" result used by the memcpy() into struct rvt_swqe "wqe" may
> be sized to 80 for struct rvt_ud_wr (which is member "ud_wr", not "wr"
> which is only 40 bytes in size). Change the destination union member so
> the compiler can use the correct bounds check.
> 
> struct rvt_swqe {
>         union {
>                 struct ib_send_wr wr;   /* don't use wr.sg_list */
>                 struct rvt_ud_wr ud_wr;
> 		...
> 	};
> 	...
> };
> 
> Silences false positive memcpy() run-time warning:
> 
>   memcpy: detected field-spanning write (size 80) of single field "&wqe->wr" at drivers/infiniband/sw/rdmavt/qp.c:2043 (size 40)
> 
> Reported-by: Zhang Yi <yizhan@redhat.com>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=216561
> Cc: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> Cc: Leon Romanovsky <leon@kernel.org>
> Cc: linux-rdma@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  drivers/infiniband/sw/rdmavt/qp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/sw/rdmavt/qp.c b/drivers/infiniband/sw/rdmavt/qp.c
> index 3acab569fbb9..3f707e1fa517 100644
> --- a/drivers/infiniband/sw/rdmavt/qp.c
> +++ b/drivers/infiniband/sw/rdmavt/qp.c
> @@ -2040,7 +2040,7 @@ static int rvt_post_one_wr(struct rvt_qp *qp,
>  	wqe = rvt_get_swqe_ptr(qp, qp->s_head);
>  
>  	/* cplen has length from above */
> -	memcpy(&wqe->wr, wr, cplen);
> +	memcpy(&wqe->ud_wr, wr, cplen);
>  
>  	wqe->length = 0;
>  	j = 0;

Thanks for the patch. We've been debating this issue internally since last week.
The problem I have is this makes it look like everything is a "UD" req when it
could be a different QP type. Maybe we just need a comment explaining.

-Denny