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

[Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>]

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