netdev - Re: rtlwifi: missing return value in

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

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