Received-SPF: pass (google.com: domain of linux-kernel+bounces-14716-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <e7e1bda7-3067-4e27-8db4-8406c4e088a0@oracle.com>
Date: Wed, 3 Jan 2024 00:07:30 +0530
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC PATCH] VMCI: Silence memcpy() run-time false positive
 warning
Content-Language: en-US
To: "Gustavo A. R. Silva" <gustavo@embeddedor.com>,
        linux-hardening@vger.kernel.org, keescook@chromium.org,
        gustavoars@kernel.org, Bryan Tan <bryantan@vmware.com>,
        Vishnu Dasa <vdasa@vmware.com>,
        VMware PV-Drivers Reviewers <pv-drivers@vmware.com>,
        Arnd Bergmann <arnd@arndb.de>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-kernel@vger.kernel.org
Cc: vegard.nossum@oracle.com, darren.kenny@oracle.com,
        syzkaller <syzkaller@googlegroups.com>
References: <20240101130828.3666251-1-harshit.m.mogalapalli@oracle.com>
 <cc8d5ac1-99b6-480d-86ca-f76ed9c324c3@embeddedor.com>
From: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
In-Reply-To: <cc8d5ac1-99b6-480d-86ca-f76ed9c324c3@embeddedor.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?utf-8?B?WHNNREo3MUhmNUk0OVdOaEZFQVJqcHdIZTgzekU1QjllazBCbFJUMmtsRFkv?=
 =?utf-8?B?cnBCTTBkUVdJZjJDTUlaUXZ1dE1ZMUgxS0I2WlZCN0ozZGttUnpjclU4ODNB?=
 =?utf-8?B?SURPRDh5Q3JScXVNeVpoQWszR0cyWndEWlUvRHJTVHRYeE93VGM2K1d3Q2xT?=
 =?utf-8?B?NFRFb0FuZGpLVEFWYVR3QmF1NHplNnZSOG1QcnV5TS85THZLSUJ1ZnBzdERu?=
 =?utf-8?B?emtmc2ZZVjRRMkFiNG9ZODdkT1FWM2k5K283dnoyeXd4SVc0Y3Myb0l3Q1l3?=
 =?utf-8?B?MndyY0xuVXVTVkZ6cUhIQzl5Q0laaG02ZTZmTzVSTFBFcTkzNXh6Q3V3eGtl?=
 =?utf-8?B?Wk5MTU9aME9SRU4zejJsb1F1ZGMzUm0vWHFYWnNLV2lQUmxpOXNjcGZTek8v?=
 =?utf-8?B?Vy9qWk5FaWZubHRsUWtRTDVVWFN1aUw1U3o1d0tVK3pVTXV5ZkQ2L1Q0Ymgy?=
 =?utf-8?B?a3pZTE1QajBJaEh4NnhTRHphemx2U1lNZW0vYm1kcnpsVWlHQ1RSU2ZIbk1Q?=
 =?utf-8?B?S1R1ZVBnd01wWVFqNXlCMEI3eEpzaVJhVWZOSEppT1RIZWNiY1hUQzFlendV?=
 =?utf-8?B?WTJ3eStMRmdhYmtYbER2QWs1eHg4aGFpYzN4MFlPSDQ4djJ3SFdnRzUwYmxV?=
 =?utf-8?B?SFVWelB2L3h2Y3V5cmVzOVUrYUx1STBXYTd5K0JjMVhwdWFmVXpTemZRVWZR?=
 =?utf-8?B?Z1A0WmJvKzJRcTMxZ3FyVFM0NmJTdldIdXFsNS9aZ2kyQmlZMkI4ZWsxLzR5?=
 =?utf-8?B?clFkcVczcHdFNHh2NGx3WUxEeU40S1h6Mkl2Vy9Bc3JiSXQwNTNRNm5WMHNM?=
 =?utf-8?B?YklQWkNqWElmMExscW9GVjAyMVdvM05NbTFkcitRTnNNNVVuOWUwY2VDTUlO?=
 =?utf-8?B?Ri9udUpqWUhIbTFXTDBHcnRjNG1tSTR4SG82cFdPMU5FL1U1VUhvWk1TR2Vt?=
 =?utf-8?B?MitiQUhWZ09pQ3lDeEp1bFhjRFRKN3phRzFldzJmUjY4ZCtxNjU1OGdrK0FZ?=
 =?utf-8?B?LzJtdFltZVpHYWkxcUF4SkFIdjNZS0pxQUl0MXNKaUUrWUdnTHc1YVZXUFMy?=
 =?utf-8?B?VWVuMmV4c241OXFQdW5MMUxCdkVQd2N4UWVEZ0FzVGw1RmFYZ3AzZGlheEh1?=
 =?utf-8?B?Q1ZYVkliR2JncG4wV1ZBREpaL3NmV1VoVkVUR1F6U2NjTU5HdldVVnRLU2lO?=
 =?utf-8?B?SWxubEllOVhrM3pCTVgvRFRYcjZvaVhkYUdSbHNtMVZBWEkrMi9HWWMxTVRZ?=
 =?utf-8?B?NXBkRjRvaVVkUWVNeVNiS3FkeUdNdTl5VGlVN3VkY3F0dDFmSkY3cXJiNXUy?=
 =?utf-8?B?ZjJFQWpMdzFheVV1cFdZTHdZYm1QZEV4RWFoQndvQk94eXR2eSs5bEFzSkl3?=
 =?utf-8?B?T3QzMEF5cUo2MmNlMnk1RGFScFlITXk0RFpYUnF2U3YwV1kvSTlWbVZZbFVW?=
 =?utf-8?B?T295T2sxRjZQZ2xJVnk2ZExEckVTbytsUTIwK1ovanlKb2Q4QXpWT25vQnlP?=
 =?utf-8?B?Q2JweDIzMFdtcEo0U2RCbFNGd2tKVTlDNEpzVEFLR29QS0JvMzFjdGI5TE4y?=
 =?utf-8?B?VkJuYURSKzZOSklUN0UreHVqZW1yVktud2hReXVJZEg4T2VXV1NHc1FwV2Y3?=
 =?utf-8?B?SWxjT0Z0cVhlUThBdElXYmdvbnlzcWJ2SzEzTXdlYnVjWGlzZUx6Q0dvRDdt?=
 =?utf-8?B?WHo0WFdHRmNuWkp2RzczaEhrNEFEZ3A4c0U4ZnlmeFZOT3QvTGJVaExZc040?=
 =?utf-8?B?QXBIWlVXeE9maVJoTEYraW1vaFZmUW1xWUV5T3JoNmZuajl4TENCTXZIYk5J?=
 =?utf-8?B?UVMzeTdwYmR2bVZVVUhMVGkrVENZNVlNc2h5aHBCUkNGcUE0c05BUHdiWHBN?=
 =?utf-8?B?WEZiVDBhb1NNMkJrMnJNanVSbkRZcU1iQ3NsaUNJS1VURjlEb1FRb1hodzVB?=
 =?utf-8?B?UFJrT0h6K1ZoN3VBSzJEN2d3WTdLd25zTmxhWklJSldzVHFLNkdXQXF5M2Zz?=
 =?utf-8?B?cnAwZC9yM3JWMEJIMDFkMUlFaGNEaUVMc3c4ZW9sbVVMV2tqVHM3QmZCNTJl?=
 =?utf-8?B?UmN5dnA0NGUxenpxYjIvMGhQU2Jhc3BiK2x1OGFLR0llSG4reTE2TjhiZWlI?=
 =?utf-8?B?U3FqRStuVFVraVJFdW45UC9ueFBwR1F5aFcyN3RGazVhUHdEbnJ5R1dmWXdm?=
 =?utf-8?Q?6K8R5Rt1owH8iOQUsBW5AOc=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	fApFKJBLnx0u+xGYkkxA8r2a24j1h21mMuFpfmNLTBlGRDiTAQF5onh5Are1ssFbXYhg40ba0OhBoA/72tbSLVSC/TJdzP55OqTihsjuqwEsAjqhM2/7jNXG/MkhRJ/oXBYTN2A3rAzpmyEzq1IwbsPuMSRJYtRs0jU8Vz8hvxyRyuc4xixgm2dTP+7zc+bLLP5IwpY9IEFdT42dvR2D4ktsndk5wSiSHnqLbIrBD3lTXRuF9cO+Dlb2eaVjnXctTOIaKwa+UxMLi0PzgEYafWfEpEMuxu1x5joXBMfEudP8ri9HOT1FJMNDDwNRd8K0XBXU+dN1t3FcKPOfon5FS1arnH+yYbqaA8SeUZ0uPBIiRqt1rqaCaOej6M7TjM+nEt+4352Q/JTGMc1BtcuO77I9yuzkrfE0nbMNvv7Y5f8u0owQvjmOWwX8hA/Kv7d5qqtxr0wmHmVgwuA5zcoP+0dC+gb9BLUgMYdSPuQwrSmtp09h9OAlSpE1Ya1dHHy42B+JiAAjWmK9Kgf++Gx99xPNg3YKuSmUm7B1tpP3ikM2+e4m6VJ7ndpK5uysRzxtHTvJ9S3LnDaTofMreya92WeesQ2fag/l0K62P+0w6dY=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 2c152877-e0ec-4bd1-b51c-08dc0bc1e7ef
X-MS-Exchange-CrossTenant-AuthSource: PH8PR10MB6290.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Jan 2024 18:37:42.9238
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: zUtBrujz/C8qEEQF6/G1TIDj9pkU8IgIM+9C0Lm0X7qy6iYqfgx8fwurpUJpUWz9j3iCnlDETCUA5FlipXFkViL2uI81hZ/UQvR2w1LyDzi0Lzb39uO3qomvZ6JyBR0s
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN0PR10MB5336
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.997,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-01-02_06,2024-01-02_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 malwarescore=0
 mlxscore=0 bulkscore=0 adultscore=0 suspectscore=0 spamscore=0
 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2311290000 definitions=main-2401020139
X-Proofpoint-GUID: vuuyfzUV4DGlwJNRBi7AVO7l46pwKFGq
X-Proofpoint-ORIG-GUID: vuuyfzUV4DGlwJNRBi7AVO7l46pwKFGq

Hi Gustavo,

On 01/01/24 11:13 pm, Gustavo A. R. Silva wrote:
> 
> 
> On 1/1/24 07:08, Harshit Mogalapalli wrote:
>> Syzkaller hit 'WARNING in dg_dispatch_as_host' bug.
>>
>> memcpy: detected field-spanning write (size 56) of single field 
>> "&dg_info->msg"
>> at drivers/misc/vmw_vmci/vmci_datagram.c:237 (size 24)
> 
> This is not a 'false postive warning.' This is a legitimately warning
> coming from the fortified memcpy().
> 
> Under FORTIFY_SOURCE we should not copy data across multiple members
> in a structure. For that we alternatives like struct_group(), or as
> in this case, splitting memcpy(), or as I suggest below, a mix of
> direct assignment and memcpy().
> 

Thanks for sharing this.
> 
>>
>> struct vmci_datagram *dg)
>>           if (dst_entry->run_delayed ||
>>               dg->src.context == VMCI_HOST_CONTEXT_ID) {
>>               struct delayed_datagram_info *dg_info;
>> +            size_t payload_size = dg_size - VMCI_DG_HEADERSIZE;
> 
> This seems to be the same as `dg->payload_size`, so I don't think a new
> variable is necessary.
> 

Oh right, this is unnecessary. I will remove it.

>>               if (atomic_add_return(1, &delayed_dg_host_queue_size)
>>                   == VMCI_MAX_DELAYED_DG_HOST_QUEUE_SIZE) {
>> @@ -234,7 +235,8 @@ static int dg_dispatch_as_host(u32 context_id, 
>> struct vmci_datagram *dg)
>>               dg_info->in_dg_host_queue = true;
>>               dg_info->entry = dst_entry;
>> -            memcpy(&dg_info->msg, dg, dg_size);
>> +            memcpy(&dg_info->msg, dg, VMCI_DG_HEADERSIZE);
>> +            memcpy(&dg_info->msg_payload, dg + 1, payload_size);
> 
> I think a direct assignment and a call to memcpy() is better in this case,
> something like this:
> 
> dg_info->msg = *dg;
> memcpy(&dg_info->msg_payload, dg + 1, dg->payload_size);
> 
> However, that `dg + 1` thing is making my eyes twitch. Where exactly are we
> making sure that `dg` actually points to an area in memory bigger than
> `sizeof(*dg)`?...
>

Going up on the call tree:

-> vmci_transport_dgram_enqueue()
--> vmci_datagram_send()
---> vmci_datagram_dispatch()
----> dg_dispatch_as_host()

1694 static int vmci_transport_dgram_enqueue(
1695         struct vsock_sock *vsk,
1696         struct sockaddr_vm *remote_addr,
1697         struct msghdr *msg,
1698         size_t len)
1699 {
1700         int err;
1701         struct vmci_datagram *dg;
1702
1703         if (len > VMCI_MAX_DG_PAYLOAD_SIZE)
1704                 return -EMSGSIZE;
1705
1706         if (!vmci_transport_allow_dgram(vsk, remote_addr->svm_cid))
1707                 return -EPERM;
1708
1709         /* Allocate a buffer for the user's message and our packet 
header. */
1710         dg = kmalloc(len + sizeof(*dg), GFP_KERNEL);
1711         if (!dg)
1712                 return -ENOMEM;

^^^ dg = kmalloc(len + sizeof(*dg), GFP_KERNEL);
I think from this we can say allocated memory for dg is bigger than 
sizeof(*dg).


> Also, we could also use struct_size() during allocation, some lines above:
> 
> -                       dg_info = kmalloc(sizeof(*dg_info) +
> -                                   (size_t) dg->payload_size, GFP_ATOMIC);
> +                       dg_info = kmalloc(struct_size(dg_info, 
> msg_payload, dg->payload_size),
> +                                         GFP_ATOMIC);
> 
Thanks again for the suggestion.

I still couldn't figure out the performance comparison before and after 
patch. Once I have some reasoning, I will include the above changes and 
send a V2.

Thanks,
Harshit
> -- 
> Gustavo
> 
>>               INIT_WORK(&dg_info->work, dg_delayed_dispatch);
>>               schedule_work(&dg_info->work);