Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] staging: rtl8723bs: hal: Fix memcpy calls
To:     Denis Efremov <efremov@linux.com>, devel@driverdev.osuosl.org
Cc:     linux-kernel@vger.kernel.org,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Bastien Nocera <hadess@hadess.net>,
        Larry Finger <Larry.Finger@lwfinger.net>,
        Jes Sorensen <jes.sorensen@gmail.com>, stable@vger.kernel.org
References: <20190930110141.29271-1-efremov@linux.com>
From:   Hans de Goede <hdegoede@redhat.com>
Message-ID: <94af475e-dd7a-6066-146a-30a9915cd325@redhat.com>
Date:   Wed, 9 Oct 2019 11:35:32 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.1.1
MIME-Version: 1.0
In-Reply-To: <20190930110141.29271-1-efremov@linux.com>
Content-Language: en-US
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Denis,

On 30-09-2019 13:01, Denis Efremov wrote:
> memcpy() in phy_ConfigBBWithParaFile() and PHY_ConfigRFWithParaFile() is
> called with "src =3D=3D NULL && len =3D=3D 0". This is an undefined behav=
ior.
> Moreover this if pre-condition "pBufLen && (*pBufLen =3D=3D 0) && !pBuf"
> is constantly false because it is a nested if in the else brach, i.e.,
> "if (cond) { ... } else { if (cond) {...} }". This patch alters the
> if condition to check "pBufLen && pBuf" pointers are not NULL.
>=20
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Bastien Nocera <hadess@hadess.net>
> Cc: Larry Finger <Larry.Finger@lwfinger.net>
> Cc: Jes Sorensen <jes.sorensen@gmail.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Denis Efremov <efremov@linux.com>
> ---
> Not tested. I don't have the hardware. The fix is based on my guess.

Thsnk you for your patch.

So I've been doing some digging and this code normally never executes.

For this to execute the user would need to change the rtw_load_phy_file mod=
ule
param from its default of 0x44 (LOAD_BB_PG_PARA_FILE | LOAD_RF_TXPWR_LMT_PA=
RA_FILE)
to something which includes 0x02 (LOAD_BB_PARA_FILE) as mask.

And even with that param set for this code to actually do something /
for pBuf to ever not be NULL the following conditions would have to
be true:

1) Set the rtw_load_phy_file module param from its default of
    0x44 (LOAD_BB_PG_PARA_FILE | LOAD_RF_TXPWR_LMT_PARA_FILE) to something
    which includes 0x02 as mask; and
2) Set rtw_phy_file_path module parameter to say "/lib/firmware/"; and
3) Store a /lib/firmware/rtl8723b/PHY_REG.txt file in the expected format.

So I've come to the conclusion that all the phy_Config*WithParaFile functio=
ns
(and a bunch of stuff they use) can be removed.

I will prepare and submit a patch for this.

Regards,

Hans


>=20
>   drivers/staging/rtl8723bs/hal/hal_com_phycfg.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>=20
> diff --git a/drivers/staging/rtl8723bs/hal/hal_com_phycfg.c b/drivers/sta=
ging/rtl8723bs/hal/hal_com_phycfg.c
> index 6539bee9b5ba..0902dc3c1825 100644
> --- a/drivers/staging/rtl8723bs/hal/hal_com_phycfg.c
> +++ b/drivers/staging/rtl8723bs/hal/hal_com_phycfg.c
> @@ -2320,7 +2320,7 @@ int phy_ConfigBBWithParaFile(
>   =09=09=09}
>   =09=09}
>   =09} else {
> -=09=09if (pBufLen && (*pBufLen =3D=3D 0) && !pBuf) {
> +=09=09if (pBufLen && pBuf) {
>   =09=09=09memcpy(pHalData->para_file_buf, pBuf, *pBufLen);
>   =09=09=09rtStatus =3D _SUCCESS;
>   =09=09} else
> @@ -2752,7 +2752,7 @@ int PHY_ConfigRFWithParaFile(
>   =09=09=09}
>   =09=09}
>   =09} else {
> -=09=09if (pBufLen && (*pBufLen =3D=3D 0) && !pBuf) {
> +=09=09if (pBufLen && pBuf) {
>   =09=09=09memcpy(pHalData->para_file_buf, pBuf, *pBufLen);
>   =09=09=09rtStatus =3D _SUCCESS;
>   =09=09} else
>=20