linux-hardening - Re: [PATCH v2 2/2] VMCI: Fix memcpy() run-time warning in dg_dispatch_as

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Mon, 8 Jan 2024 10:33:23 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Harshit Mogalapalli <harshit.m.mogalapalli@...cle.com>
Cc: linux-hardening@...r.kernel.org, keescook@...omium.org,
	error27@...il.com, gustavoars@...nel.org,
	Bryan Tan <bryantan@...are.com>, Vishnu Dasa <vdasa@...are.com>,
	VMware PV-Drivers Reviewers <pv-drivers@...are.com>,
	Arnd Bergmann <arnd@...db.de>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-kernel@...r.kernel.org, vegard.nossum@...cle.com,
	darren.kenny@...cle.com, syzkaller <syzkaller@...glegroups.com>
Subject: Re: [PATCH v2 2/2] VMCI: Fix memcpy() run-time warning in
 dg_dispatch_as_host()

On Fri, Jan 05, 2024 at 08:40:00AM -0800, 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)
> 
> WARNING: CPU: 0 PID: 1555 at drivers/misc/vmw_vmci/vmci_datagram.c:237
> dg_dispatch_as_host+0x88e/0xa60 drivers/misc/vmw_vmci/vmci_datagram.c:237
> 
> Some code commentry, based on my understanding:
> 
> 544 #define VMCI_DG_SIZE(_dg) (VMCI_DG_HEADERSIZE + (size_t)(_dg)->payload_size)
> /// This is 24 + payload_size
> 
> memcpy(&dg_info->msg, dg, dg_size);
> 	Destination = dg_info->msg ---> this is a 24 byte
> 					structure(struct vmci_datagram)
> 	Source = dg --> this is a 24 byte structure (struct vmci_datagram)
> 	Size = dg_size = 24 + payload_size
> 
> {payload_size = 56-24 =32} -- Syzkaller managed to set payload_size to 32.
> 
>  35 struct delayed_datagram_info {
>  36         struct datagram_entry *entry;
>  37         struct work_struct work;
>  38         bool in_dg_host_queue;
>  39         /* msg and msg_payload must be together. */
>  40         struct vmci_datagram msg;
>  41         u8 msg_payload[];
>  42 };
> 
> So those extra bytes of payload are copied into msg_payload[], a run time
> warning is seen while fuzzing with Syzkaller.
> 
> One possible way to fix the warning is to split the memcpy() into
> two parts -- one -- direct assignment of msg and second taking care of payload.
> 
> Gustavo quoted:
> "Under FORTIFY_SOURCE we should not copy data across multiple members
> in a structure."
> 
> Reported-by: syzkaller <syzkaller@...glegroups.com>
> Suggested-by: Vegard Nossum <vegard.nossum@...cle.com>
> Suggested-by: Gustavo A. R. Silva <gustavoars@...nel.org>
> Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@...cle.com>
> ---
> This patch is only tested with the C reproducer, not any testing
> specific to driver is done.
> 
> v1->v2: ( Suggestions from Gustavo )
>         1. Change the commit message false positive --> legitimate
>            warning.

The commit message is fine.

Reviewed-by: Dan Carpenter <dan.carpenter@...aro.org>

But, I mean, it's not really "legitimate".  It meets the fortify source
heuristic, but it's still a false positive.  Fortify source is
*supposed* to find memory corruption bugs and this is not a memory
corruption bug.  It's just that these days we have to treat foritify
false positives as crashing bugs because people enable it and we have to
fix it.

Let's not pretend that fortify has helped us in this situation, it has
caused us a problem.  It has taken valid code and created a crashing
bug.  I'm not saying that the cost isn't worth it, but let's not pretend.

regards,
dan carpenter