Re: [RFC PATCH] VMCI: Silence memcpy() run-time false positive warning

Harshit Mogalapalli
2024-01-02T18:38:10Z

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