Received-SPF: pass (google.com: domain of linux-kernel+bounces-21000-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <4603630c-3bb2-42b6-b0dd-79d500460ab4@embeddedor.com>
Date: Tue, 9 Jan 2024 08:35:37 -0600
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 2/2] VMCI: Fix memcpy() run-time warning in
 dg_dispatch_as_host()
Content-Language: en-US
To: Dan Carpenter <dan.carpenter@linaro.org>
Cc: Kees Cook <keescook@chromium.org>,
 Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>,
 linux-hardening@vger.kernel.org, error27@gmail.com, gustavoars@kernel.org,
 Bryan Tan <bryantan@vmware.com>, Vishnu Dasa <vdasa@vmware.com>,
 VMware PV-Drivers Reviewers <pv-drivers@vmware.com>,
 Arnd Bergmann <arnd@arndb.de>,
 Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
 linux-kernel@vger.kernel.org, vegard.nossum@oracle.com,
 darren.kenny@oracle.com, syzkaller <syzkaller@googlegroups.com>
References: <20240105164001.2129796-1-harshit.m.mogalapalli@oracle.com>
 <20240105164001.2129796-2-harshit.m.mogalapalli@oracle.com>
 <202401081430.9DAB37B46@keescook>
 <9c742547-0021-464b-b7a8-7af46b0a4afa@embeddedor.com>
 <79ca2f85-1f8d-4a12-aa5a-09137033605f@moroto.mountain>
 <7d7b94ba-9a1f-44de-8491-dc4864338f80@embeddedor.com>
 <36aecc9a-ac30-436f-b42b-39f63513d743@moroto.mountain>
From: "Gustavo A. R. Silva" <gustavo@embeddedor.com>
In-Reply-To: <36aecc9a-ac30-436f-b42b-39f63513d743@moroto.mountain>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


On 1/9/24 07:22, Dan Carpenter wrote:
> On Tue, Jan 09, 2024 at 06:31:41AM -0600, Gustavo A. R. Silva wrote:
>>
>> You're arguing that fortify caused a problem.
> 
> Yes.
> 
> Before: Code working correctly
> After: Kernel Panic
> 
> At first, I started to question if I was going mad, but then I looked
> through the email thread and Harshit tested it and proved that the
> kernel does actually panic depending on the .config.
> 
> I mean realistically we should backport this patch to old kernels,
> right?  And if we had to assign a Fixes tag to this it would need to be
> the commit which adds Fortify to the kernel.  Prior to that commit the
> code was fine.
> 
> Again, I'm not saying that Fortify is bad overall.  Probably in DnD it
> would be Chaotic Good where it's overall good but sometimes a pain.
> 

You know what, I think the discrepancy here lies in the fact that, before,
fortify didn't complain about writes across multiple members in the same
struct, as long as we remained between the boundaries of the struct. This
was done via __builtin_object_size(p, 0).

In recent times, that has changed, and now fortify has been tightened to warn
about writes beyond the boundaries of a single object. This is now done via
__builtin_dynamic_object_size(p, 1)/__builtin_object_size(p, 1).

And, if this patch is to be backported, _technically_, the Fixes tag should
probably be assigned to that change from __bos(p, 0) to __bos(p, 1):

commit f68f2ff91512 ("fortify: Detect struct member overflows in memcpy() at compile-time")

In any case, fortify should now emit this sort of warning, and that adds
an extra layer of hardening to the kernel.

Thanks
--
Gustavo