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
| ||
|
Message-ID: <19d41aed-591d-ce01-c5fc-350fe40e9f8f@lwfinger.net> Date: Wed, 12 Oct 2022 09:43:07 -0500 From: Larry Finger <Larry.Finger@...inger.net> To: "Colin King (gmail)" <colin.i.king@...il.com> Cc: Kalle Valo <kvalo@...nel.org>, "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, "linux-wireless@...r.kernel.org" <linux-wireless@...r.kernel.org>, "netdev@...r.kernel.org" <netdev@...r.kernel.org> Subject: Re: rtlwifi: missing return value in On 10/12/22 08:10, Colin King (gmail) wrote: > Hi, > > Static analysis with cppcheck has found an issue with a function that returns an > int value but there is a code path that does not return a value causing > undefined behaviour: > > Source: drivers/net/wireless/realtek/rtlwifi/usb.c function _rtl_rx_get_padding > - introduced by commit: > > commit 354d0f3c40fb40193213e40f3177ff528798ca8d > Author: Larry Finger <Larry.Finger@...inger.net> > Date: Wed Sep 25 12:57:47 2013 -0500 > > rtlwifi: Fix smatch warnings in usb.c > > The issue occurs when NET_IP_ALIGN is zero and when len >= sizeof(*hdr), then > the following return is *not* taken: > > /* make function no-op when possible */ > if (NET_IP_ALIGN == 0 || len < sizeof(*hdr)) > return 0 > > and then execution reaches the end of the function where no return value is > returned because the #if NET_IP_ALIGN != 0 is false so the return padding hunk > of the code is not compiled in. Colin, The no-op code snippet you quote above returns zero if NET_IP_ALIGN is zero. In that case, the value of len is irrelevant. If NET_IP_ALIGN is not zero, then the function returns a value in every case. I admit that the logic is overly convoluted and clearly it overwhelmed cppcheck, but as I see it, the problem is with the tool, not with the code. Obviously, a patch such as shown below would let your tool come to the correct conclusion, but it is not wrong now. diff --git a/drivers/net/wireless/realtek/rtlwifi/usb.c b/drivers/net/wireless/realtek/rtlwifi/usb.c index a8eebafb9a7e..7b7277b33a7e 100644 --- a/drivers/net/wireless/realtek/rtlwifi/usb.c +++ b/drivers/net/wireless/realtek/rtlwifi/usb.c @@ -554,13 +554,11 @@ static unsigned int _rtl_rx_get_padding(struct ieee80211_hdr *hdr, { #if NET_IP_ALIGN != 0 unsigned int padding = 0; -#endif /* make function no-op when possible */ - if (NET_IP_ALIGN == 0 || len < sizeof(*hdr)) + if (len < sizeof(*hdr)) return 0; -#if NET_IP_ALIGN != 0 /* alignment calculation as in lbtf_rx() / carl9170_rx_copy_data() */ /* TODO: deduplicate common code, define helper function instead? */ @@ -581,6 +579,8 @@ static unsigned int _rtl_rx_get_padding(struct ieee80211_hdr *hdr, padding ^= NET_IP_ALIGN; return padding; +#else + return 0; #endif } I happen to believe that fixing bugs in tools is more important than needlessly modifying the kernel so that the tool gets the right answer. I say that as someone that has been burned in the past by a faulty tool. Larry
Powered by blists - more mailing lists