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);
