Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Sender: Larry Finger <larry.finger@gmail.com>
Message-ID: <19d41aed-591d-ce01-c5fc-350fe40e9f8f@lwfinger.net>
Date:   Wed, 12 Oct 2022 09:43:07 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.3.1
Subject: Re: rtlwifi: missing return value in
Content-Language: en-US
To:     "Colin King (gmail)" <colin.i.king@gmail.com>
Cc:     Kalle Valo <kvalo@kernel.org>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>
References: <11ea25ad-0ad9-7d34-de3f-09ca7d9c4ee2@gmail.com>
From:   Larry Finger <Larry.Finger@lwfinger.net>
In-Reply-To: <11ea25ad-0ad9-7d34-de3f-09ca7d9c4ee2@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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@lwfinger.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