Re: [PATCH] net: sched: act_ife: initialize struct tc_ife to fix KMSAN kernel-infoleak

[Ranganath V N <vnranganath.20@gmail.com>]

>> Fix a KMSAN kernel-infoleak detected  by the syzbot .
>>
>> [net?] KMSAN: kernel-infoleak in __skb_datagram_iter
>>
>> In tcf_ife_dump(), the variable 'opt' was partially initialized using a
>> designatied initializer. While the padding bytes are reamined
>> uninitialized. nla_put() copies the entire structure into a
>> netlink message, these uninitialized bytes leaked to userspace.
>>
>> Initialize the structure with memset before assigning its fields
>> to ensure all members and padding are cleared prior to beign copied.
>>
>> This change silences the KMSAN report and prevents potential information
>> leaks from the kernel memory.
>>
>> This fix has been tested and validated by syzbot. This patch closes the
>> bug reported at the following syzkaller link and ensures no infoleak.
>>
>> Reported-by: syzbot+0c85cae3350b7d486aee@syzkaller.appspotmail.com
>> Closes: https://syzkaller.appspot.com/bug?extid=0c85cae3350b7d486aee
>> Tested-by: syzbot+0c85cae3350b7d486aee@syzkaller.appspotmail.com
>> Fixes: ef6980b6becb ("introduce IFE action")
>> Signed-off-by: Ranganath V N <vnranganath.20@gmail.com>
>> ---
>> Fix a KMSAN kernel-infoleak detected  by the syzbot .
>>
>> [net?] KMSAN: kernel-infoleak in __skb_datagram_iter
>>
>> In tcf_ife_dump(), the variable 'opt' was partially initialized using a
>> designatied initializer. While the padding bytes are reamined
>> uninitialized. nla_put() copies the entire structure into a
>> netlink message, these uninitialized bytes leaked to userspace.
>>
>> Initialize the structure with memset before assigning its fields
>> to ensure all members and padding are cleared prior to beign copied.
>> ---
>>  net/sched/act_ife.c | 13 ++++++++-----
>>  1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
>> index 107c6d83dc5c..608ef6cc2224 100644
>> --- a/net/sched/act_ife.c
>> +++ b/net/sched/act_ife.c
>> @@ -644,13 +644,16 @@ static int tcf_ife_dump(struct sk_buff *skb, struct tc_action *a, int bind,
>>         unsigned char *b = skb_tail_pointer(skb);
>>         struct tcf_ife_info *ife = to_ife(a);
>>         struct tcf_ife_params *p;
>> -       struct tc_ife opt = {
>> -               .index = ife->tcf_index,
>> -               .refcnt = refcount_read(&ife->tcf_refcnt) - ref,
>> -               .bindcnt = atomic_read(&ife->tcf_bindcnt) - bind,
>> -       };
>> +       struct tc_ife opt;
>>         struct tcf_t t;
>>
>> +       memset(&opt, 0, sizeof(opt));
>> +       memset(&t, 0, sizeof(t));
>
>Why is the second memset() needed ? Please do not add unrelated changes.
>
>Also I would also fix tcf_connmark_dump()

Hi Eric,
Do you want me fix tcf_connmark_dump() in this patch or new?

Regards,
Ranganath