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().
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.
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)`?...
Also, we could also use struct_size() during allocation, some lines above:Thanks again for the suggestion.
- 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);
--
Gustavo
INIT_WORK(&dg_info->work, dg_delayed_dispatch);
schedule_work(&dg_info->work);