Re: [PATCH 1/3] s390/ctcm: Fix return type of ctc{mp,}m_tx()

From: Alexandra Winter <wintera@linux.ibm.com>
To: Kees Cook <keescook@chromium.org>, Nathan Chancellor <nathan@kernel.org>
Cc: Wenjia Zhang <wenjia@linux.ibm.com>,
	Heiko Carstens <hca@linux.ibm.com>,
	Vasily Gorbik <gor@linux.ibm.com>,
	Alexander Gordeev <agordeev@linux.ibm.com>,
	Christian Borntraeger <borntraeger@linux.ibm.com>,
	Sven Schnelle <svens@linux.ibm.com>,
	linux-s390@vger.kernel.org, netdev@vger.kernel.org,
	Nick Desaulniers <ndesaulniers@google.com>,
	Tom Rix <trix@redhat.com>,
	Sami Tolvanen <samitolvanen@google.com>,
	llvm@lists.linux.dev, linux-kernel@vger.kernel.org,
	patches@lists.linux.dev
Subject: Re: [PATCH 1/3] s390/ctcm: Fix return type of ctc{mp,}m_tx()
Date: Thu, 3 Nov 2022 12:06:24 +0100	[thread overview]
Message-ID: <c7f5b3db-76f8-2772-b2cd-355f0c4c55b1@linux.ibm.com> (raw)
In-Reply-To: <202211021209.276A8BA@keescook>



On 02.11.22 20:09, Kees Cook wrote:
> On Wed, Nov 02, 2022 at 09:32:50AM -0700, Nathan Chancellor wrote:
>> With clang's kernel control flow integrity (kCFI, CONFIG_CFI_CLANG),
>> indirect call targets are validated against the expected function
>> pointer prototype to make sure the call target is valid to help mitigate
>> ROP attacks. If they are not identical, there is a failure at run time,
>> which manifests as either a kernel panic or thread getting killed. A
>> proposed warning in clang aims to catch these at compile time, which
>> reveals:
>>
>>   drivers/s390/net/ctcm_main.c:1064:21: error: incompatible function pointer types initializing 'netdev_tx_t (*)(struct sk_buff *, struct net_device *)' (aka 'enum netdev_tx (*)(struct sk_buff *, struct net_device *)') with an expression of type 'int (struct sk_buff *, struct net_device *)' [-Werror,-Wincompatible-function-pointer-types-strict]
>>           .ndo_start_xmit         = ctcm_tx,
>>                                     ^~~~~~~
>>   drivers/s390/net/ctcm_main.c:1072:21: error: incompatible function pointer types initializing 'netdev_tx_t (*)(struct sk_buff *, struct net_device *)' (aka 'enum netdev_tx (*)(struct sk_buff *, struct net_device *)') with an expression of type 'int (struct sk_buff *, struct net_device *)' [-Werror,-Wincompatible-function-pointer-types-strict]
>>           .ndo_start_xmit         = ctcmpc_tx,
>>                                     ^~~~~~~~~
>>
>> ->ndo_start_xmit() in 'struct net_device_ops' expects a return type of
>> 'netdev_tx_t', not 'int'. Adjust the return type of ctc{mp,}m_tx() to
>> match the prototype's to resolve the warning and potential CFI failure,
>> should s390 select ARCH_SUPPORTS_CFI_CLANG in the future.
>>
>> Link: https://github.com/ClangBuiltLinux/linux/issues/1750
>> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> 
> Reviewed-by: Kees Cook <keescook@chromium.org>
> 

Could you please also remove the corresponding comments:

diff --git a/drivers/s390/net/ctcm_main.c b/drivers/s390/net/ctcm_main.c
index 37b551bd43bf..14200548704a 100644
--- a/drivers/s390/net/ctcm_main.c
+++ b/drivers/s390/net/ctcm_main.c
@@ -825,13 +825,6 @@ static int ctcmpc_transmit_skb(struct channel *ch, struct sk_buff *skb)
 /*
  * Start transmission of a packet.
  * Called from generic network device layer.
- *
- *  skb                Pointer to buffer containing the packet.
- *  dev                Pointer to interface struct.
- *
- * returns 0 if packet consumed, !0 if packet rejected.
- *         Note: If we return !0, then the packet is free'd by
- *               the generic network layer.
  */
 /* first merge version - leaving both functions separated */
 static int ctcm_tx(struct sk_buff *skb, struct net_device *dev)

Reviewed-by: Alexandra Winter <wintera@linux.ibm.com>
