Messages in this thread | ![/](/images/icornerl.gif) | | Date | Wed, 3 Jan 2024 00:07:30 +0530 | Subject | Re: [RFC PATCH] VMCI: Silence memcpy() run-time false positive warning | From | Harshit Mogalapalli <> |
| |
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);
| ![\](/images/icornerr.gif) |