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;
Message-ID: <02e2ad88adfc46976803c2c0be7dce38a4d8b660.camel@sipsolutions.net>
Subject: Re: [PATCH 2/2] wifi: ath9k: fix fortify warnings
From:   Johannes Berg <johannes@sipsolutions.net>
To:     Dmitry Antipov <dmantipov@yandex.ru>,
        Toke =?ISO-8859-1?Q?H=F8iland-J=F8rgensen?= <toke@toke.dk>
Cc:     Kalle Valo <kvalo@kernel.org>, linux-wireless@vger.kernel.org
Date:   Tue, 20 Jun 2023 09:44:59 +0200
In-Reply-To: <20230620073228.363207-2-dmantipov@yandex.ru>
References: <20230620073228.363207-1-dmantipov@yandex.ru>
         <20230620073228.363207-2-dmantipov@yandex.ru>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.48.3 (3.48.3-1.fc38) 
MIME-Version: 1.0
Precedence: bulk

On Tue, 2023-06-20 at 10:31 +0300, Dmitry Antipov wrote:
> When compiling with gcc 13.1 and CONFIG_FORTIFY_SOURCE=3Dy,
> I've noticed the following:
>=20
> In function =E2=80=98fortify_memcpy_chk=E2=80=99,
>     inlined from =E2=80=98ath_tx_complete_aggr=E2=80=99 at drivers/net/wi=
reless/ath/ath9k/xmit.c:556:4,
>     inlined from =E2=80=98ath_tx_process_buffer=E2=80=99 at drivers/net/w=
ireless/ath/ath9k/xmit.c:773:3:
> ./include/linux/fortify-string.h:529:25: warning: call to =E2=80=98__read=
_overflow2_field=E2=80=99
> declared with attribute warning: detected read beyond size of field (2nd =
parameter);
> maybe use struct_group()? [-Wattribute-warning]
>   529 |                         __read_overflow2_field(q_size_field, size=
);
>       |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~=
~
>=20
> In function =E2=80=98fortify_memcpy_chk=E2=80=99,
>     inlined from =E2=80=98ath_tx_count_frames=E2=80=99 at drivers/net/wir=
eless/ath/ath9k/xmit.c:473:3,
>     inlined from =E2=80=98ath_tx_complete_aggr=E2=80=99 at drivers/net/wi=
reless/ath/ath9k/xmit.c:572:2,
>     inlined from =E2=80=98ath_tx_process_buffer=E2=80=99 at drivers/net/w=
ireless/ath/ath9k/xmit.c:773:3:
> ./include/linux/fortify-string.h:529:25: warning: call to =E2=80=98__read=
_overflow2_field=E2=80=99
> declared with attribute warning: detected read beyond size of field (2nd =
parameter);
> maybe use struct_group()? [-Wattribute-warning]
>   529 |                         __read_overflow2_field(q_size_field, size=
);
>       |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~=
~
>=20
> In both cases, the compiler complains on:
>=20
> memcpy(ba, &ts->ba_low, WME_BA_BMP_SIZE >> 3);
>=20
> which is the legal way to copy both 'ba_low' and following 'ba_high'
> members of 'struct ath_tx_status' at once (that is, issue one 8-byte
> 'memcpy()' for two 4-byte fields). Since the fortification logic
> seems interprets this trick as an attempt to overread 4-byte 'ba_low',
> relevant warnings may be silenced by specifying source buffer with
> 'offsetof()' of the 'ba_low' instead of using an address of the latter.
>=20

I think other places have typically fixed that with a struct_group, say
e.g. commit bfcc8ba45eb8 ("wifi: ath: Silence memcpy run-time false
positive warning"). To me, that seems slightly better since it still
ensures you're actually in the fields you wanted? But dunno.

johannes