Received-SPF: pass (google.com: domain of linux-kernel+bounces-53870-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <CAK-6q+jsZ13Cs9iuk_WjFeYFCEnnj-dJ9QYkWaw4fh6Gi=JtHA@mail.gmail.com>
 <20240112131554.10352-1-n.zhandarovich@fintech.ru> <CAK-6q+gcs2djQfKRsuGpD7WERmbLhzjkHEm80MRe+2UE3bteKw@mail.gmail.com>
 <CAK-6q+hRbsFkQec3O8FnT-G9Mx07rdhEMfmTE2Q0SDN0kKN-8g@mail.gmail.com> <64dbd05c-4939-49ba-a8d5-807fe3ff2987@fintech.ru>
In-Reply-To: <64dbd05c-4939-49ba-a8d5-807fe3ff2987@fintech.ru>
From: Alexander Aring <aahringo@redhat.com>
Date: Mon, 5 Feb 2024 14:49:26 -0500
Message-ID: <CAK-6q+gEjqCrnFkpKSuQiuhpx9zyuWr6y0tQpJOLquoz2pnmzw@mail.gmail.com>
Subject: Re: [PATCH RESEND] mac802154: Fix uninit-value access in ieee802154_hdr_push_sechdr
To: Nikita Zhandarovich <n.zhandarovich@fintech.ru>
Cc: Zhang Shurong <zhang_shurong@foxmail.com>, alex.aring@gmail.com, 
	stefan@datenfreihafen.org, davem@davemloft.net, edumazet@google.com, 
	kuba@kernel.org, pabeni@redhat.com, linux-wpan@vger.kernel.org, 
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org, 
	harperchen1110@gmail.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi,

On Thu, Jan 18, 2024 at 8:00=E2=80=AFAM Nikita Zhandarovich
<n.zhandarovich@fintech.ru> wrote:
..
>
> I was curious whether a smaller change would suffice since I might be
> too green to see the full picture here.
>
> In all honesty I am failing to see how exactly it happens that cb->secen
> =3D=3D 1 and cb->secen_override =3D=3D 0 (which is exactly what occurs du=
ring
> this error repro) at the start of mac802154_set_header_security().
> Since there is a check in mac802154_set_header_security()
>
>         if (!params.enabled && cb->secen_override && cb->secen)
>
> maybe we take off 'cb->secen_override' part of the condition? That way
> we catch the case when security is supposedly enabled without parameters
> being available (not enabled) and return with error. Or is this approach
> too lazy?

I need to see the full patch for this. In my opinion there are two patches =
here:

1. fix uninit values
2. return an error with some mismatched security parameters. (I think
this is where your approach comes in place)

The 1. case is what syzbot is complaining about and in my opinion easy
to fix at [0] to init some more default values of "struct dgram_sock"
[1].

Then 2. can be fixed afterwards.

- Alex

[0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree=
/net/ieee802154/socket.c#n474
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree=
/net/ieee802154/socket.c#n435