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

[Gustavo A. R. Silva]

On 1/8/24 12:36, Dan Carpenter wrote:
> On Mon, Jan 08, 2024 at 11:03:51AM -0600, Gustavo A. R. Silva wrote:
> > On 1/8/24 01:33, Dan Carpenter wrote:
> > > 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@xxxxxxxxxxxxxxxx>
> > > > Suggested-by: Vegard Nossum <vegard.nossum@xxxxxxxxxx>
> > > > Suggested-by: Gustavo A. R. Silva <gustavoars@xxxxxxxxxx>
> > > > Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@xxxxxxxxxx>
> > > > ---
> > > > 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@xxxxxxxxxx>
> > >
> > > 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.
> >
> > It's a "legitimate warning" (which differs from a "legitimate memory
> > corruption bug") in the sense that the feature is doing what it's
> > supposed to do: reporting a write beyond the boundaries of a field/member
> > in a structure.
> >
> > Is that simple.  I don't see the "pretense" here.
> >
> > BTW, is this _warning_ really causing a crash?
>
> I don't know how many people have Reboot on Warn enabled but I've heard
> it's a shocking high number of people.
>
> My problem with "legitimate" is that it's a biased word which imples
> "good".  A more neutral way to describe it would be "acceptable" or
> "matches the heuristic".
>
> Generally, we get a lot of patches which are to make a tool happy and
> sometimes like here it's probably an acceptable cost.  But I think
> other times people lose sight of what it's all about and confuse good
> and bad.  In some kind of very literal and narrow sense this warning is
> bad.  It takes perfectly okay code and turns it into a crashing bug.  In
> the larger sense and long term view then, sure, the heuristic is useful,
> but right here, in this situation, it's bad.


This is right on point:

"In some kind of very literal and narrow sense this warning is bad."

Let's say the vast majority of people is of this opinion. Thus, they
engage in never-ending discussions, and end up disregarding this sort
of warning, deciding to ignore it completely. Then, a lot more of these
warnings go unfixed. Then, a couple of actual memory corruption bugs
are introduced. Then, people don't notice them. Then, the hardening
feature ends up becoming useless.

Why insist on disregarding something that is clearly beneficial to
acknowledge and worth correcting right on the spot?

This is real work that must be done if we want the feature to help us
detect bugs and potential vulnerabilities.

Thanks
--
Gustavo