Hi, On 2/5/24 11:49, Alexander Aring wrote: > Hi, > > On Thu, Jan 18, 2024 at 8:00 AM Nikita Zhandarovich > <n.zhandarovich@xxxxxxxxxx> 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 >> == 1 and cb->secen_override == 0 (which is exactly what occurs during >> 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: > Alex, I am gonna try to test your version and send out patches before the end of week, thank you for reminding me. > 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]. However, if I may, I am still worried that initing stuff in [0] won't help much. They way I see it, there are mismatched sec. parameters that lead to the actual uninit issue, but are not victim of it themselves. Specifically, once we enter mac802154_set_header_security() all fields of 'cb' have values (albeit a possibly wrong combo of them), values copied from 'ro' seemingly w/o a hitch; the function ends early (cause of mismatch); we end up NOT filling values in ieee802154_hdr *hdr, at the very least these: hdr->sec.level = level; hdr->sec.key_id_mode = params.out_key.mode; Then we are back in ieee802154_header_create(): ieee802154_header_create -> ieee802154_hdr_push -> ieee802154_hdr_push_sechdr, where we finally access aforementioned values despite putting nothing in them. In other words, I am unsure that mismatch in sec. parameters (cb->secen, params.enabled etc.), which leads to uninit issues in hdr->sec.XXX fields, is itself caused by the uninit values in dgram_sock (since KMSAN should have caught it earlier). But if you are certain, I don't mind taking on the patches you suggested. > > 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 > Thank you for your patience, Nikita
