Message-ID: <Zw6LILtuJdIT5-Eu@bogus>
Date: Tue, 15 Oct 2024 16:32:48 +0100
From: Sudeep Holla <sudeep.holla@....com>
To: Gavin Shan <gshan@...hat.com>
Cc: linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	shan.gavin@...il.com, Sudeep Holla <sudeep.holla@....com>
Subject: Re: [PATCH] firmware: arm_ffa: Fix warning caused by export_uuid()

On Tue, Oct 15, 2024 at 09:43:32AM +1000, Gavin Shan wrote:
> On 10/15/24 12:41 AM, Sudeep Holla wrote:
> > On Mon, Oct 14, 2024 at 08:25:54PM +1000, Gavin Shan wrote:
> > > On 10/14/24 8:08 PM, Sudeep Holla wrote:
> > > > On Mon, Oct 14, 2024 at 10:47:24AM +1000, Gavin Shan wrote:
> > > > > Run into build warning caused by export_uuid() where the UUID's
> > > > > length exceeds that of ffa_value_t::a2, as the following warning
> > > > > messages indicate.
> > > > > 
> > > > > In function ‘fortify_memcpy_chk’,
> > > > > inlined from ‘export_uuid’ at ./include/linux/uuid.h:88:2,
> > > > > inlined from ‘ffa_msg_send_direct_req2’ at drivers/firmware/arm_ffa/driver.c:488:2:
> > > > > ./include/linux/fortify-string.h:571:25: error: call to ‘__write_overflow_field’ \
> > > > > declared with attribute warning: detected write beyond size of field (1st parameter); \
> > > > > maybe use struct_group()? [-Werror=attribute-warning]
> > > > > 571 |                         __write_overflow_field(p_size_field, size);
> > > > >       |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > > 
> > > > > Fix it by not passing a plain buffer to memcpy() to avoid the overflow
> > > > > and underflow warning, similar to what have been done to copy over the
> > > > > struct ffa_send_direct_data2.
> > > > > 
> > > > 
> > > > Are you observing this just on the upstream or -next as well? There is a
> > > > fix in the -next which I haven't sent to soc team yet, will do so soon.
> > > > 
> > > 
> > > I just tried the upstream when the patch was posted. I just have a try with -next
> > > and similar error exists.
> > > 
> > > [root@...dia-grace-hopper-01 linux-next]# git remote -v
> > > origin	git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git (fetch)
> > > origin	git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git (push)
> > > [root@...dia-grace-hopper-01 linux-next]# make W=1 drivers/firmware/arm_ffa/driver.o
> > >     :
> > > In function ‘fortify_memcpy_chk’,
> > >      inlined from ‘ffa_msg_send_direct_req2’ at drivers/firmware/arm_ffa/driver.c:504:3:
> > > ./include/linux/fortify-string.h:580:25: error: call to ‘__read_overflow2_field’ declared with attribute warning: \
> > > detected read beyond size of field (2nd parameter); maybe use struct_group()? [-Werror=attribute-warning]
> > >    580 |                         __read_overflow2_field(q_size_field, size);
> > >        |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > cc1: all warnings being treated as errors
> > > 
> > > 
> > > Part of the changes included this patch is still needed by -next. Could you please
> > > squeeze the changes to that one to be pulled?
> > 
> > Sure I can do that. Can you share the build command(specifically if any
> > extra warning are enabled) and the toolchain used ? I am unable to reproduce
> > it with clang 20.0.0, not sure if it my toolchain or build command/flags that
> > differs here.
> > 
> 
> Thanks, Sudeep. I think what matters is 'W=1', which is translated to '-DKBUILD_EXTRA_WARN1'.
> With KBUILD_EXTRA_WARN1 declared in linux-next, the warning is raised by __read_overflow2_field().
> 

Sorry my bad, I got confused. I was not able to reproduce the write overflow
warning even if I revert the patch in the -next. I was seeing this warning,
just got confused. Thanks for the details. I have pushed this patch partially
as discussed since other warning is already addressed.

Regards,
Sudeep

-->8
>From b0798838418abe996d9b618d341d865462264cbe Mon Sep 17 00:00:00 2001
From: Gavin Shan <gshan@...hat.com>
Date: Mon, 14 Oct 2024 10:47:24 +1000
Subject: [PATCH] firmware: arm_ffa: Avoid string-fortify warning caused by
 memcpy()

Copying from a 144 byte structure arm_smccc_1_2_regs at an offset of 32
into an 112 byte struct ffa_send_direct_data2 causes a compile-time warning:

 | In file included from drivers/firmware/arm_ffa/driver.c:25:
 | In function 'fortify_memcpy_chk',
 |    inlined from 'ffa_msg_send_direct_req2' at drivers/firmware/arm_ffa/driver.c:504:3:
 | include/linux/fortify-string.h:580:4: warning: call to '__read_overflow2_field'
 | 	declared with 'warning' attribute: detected read beyond size of field
 | 	(2nd parameter); maybe use struct_group()? [-Wattribute-warning]
 |                         __read_overflow2_field(q_size_field, size);

Fix it by not passing a plain buffer to memcpy() to avoid the overflow
warning.

Fixes: aaef3bc98129 ("firmware: arm_ffa: Add support for FFA_MSG_SEND_DIRECT_{REQ,RESP}2")
Signed-off-by: Gavin Shan <gshan@...hat.com>
Message-Id: <20241014004724.991353-1-gshan@...hat.com>
Signed-off-by: Sudeep Holla <sudeep.holla@....com>
---
 drivers/firmware/arm_ffa/driver.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
index 8dd81db9b071..b14cbdae94e8 100644
--- a/drivers/firmware/arm_ffa/driver.c
+++ b/drivers/firmware/arm_ffa/driver.c
@@ -501,7 +501,7 @@ static int ffa_msg_send_direct_req2(u16 src_id, u16 dst_id, const uuid_t *uuid,
 		return ffa_to_linux_errno((int)ret.a2);

 	if (ret.a0 == FFA_MSG_SEND_DIRECT_RESP2) {
-		memcpy(data, &ret.a4, sizeof(*data));
+		memcpy(data, (void *)&ret + offsetof(ffa_value_t, a4), sizeof(*data));
 		return 0;
 	}

--
2.34.1

Powered by blists - more mailing lists