* Re: [PATCH v1] Fix NULL pointer dereference in cn_filter()
2023-10-17 8:02 ` Simon Horman
@ 2023-10-17 18:23 ` Anjali Kulkarni
2023-10-20 23:30 ` Anjali Kulkarni
1 sibling, 0 replies; 4+ messages in thread
From: Anjali Kulkarni @ 2023-10-17 18:23 UTC (permalink / raw)
To: Simon Horman
Cc: open list, David S. Miller, Liam Howlett, Netdev,
kernel test robot, kuba@kernel.org
> On Oct 17, 2023, at 1:02 AM, Simon Horman <horms@kernel.org> wrote:
>
> On Fri, Oct 13, 2023 at 03:56:19PM -0700, Anjali Kulkarni wrote:
>> Check that sk_user_data is not NULL, else return from cn_filter().
>
> Thanks,
>
> I agree that this change seems likely to address the problem at the link
> below. And I also think cn_filter() is a good place to fix this [1].
> But I am wondering if you could add some commentary to the patch
> description, describing under what circumstances this problem can occur.
Thanks for reviewing! Yes, it seems this patch has fixed the issue (as tested by Oliver Sang). I will add some description for when this problem can occur in the next patch revision.
>
> [1] https://urldefense.com/v3/__https://lore.kernel.org/all/20231013120105.GH29570@kernel.org/__;!!ACWV5N9M2RV99hQ!NNsMHaho3lyW2NyoT8Sju1BL78S5pNu5AhtZC76cc1Xb1_psXwfP0lmVtVmTxGRrsQkzClWNS8HriosTMols$
>
>> Fixes: 2aa1f7a1f47c ("connector/cn_proc: Add filtering to fix some bugs")
>> Reported-by: kernel test robot <oliver.sang@intel.com>
>> Closes: https://urldefense.com/v3/__https://lore.kernel.org/oe-lkp/202309201456.84c19e27-oliver.sang@intel.com/__;!!ACWV5N9M2RV99hQ!NNsMHaho3lyW2NyoT8Sju1BL78S5pNu5AhtZC76cc1Xb1_psXwfP0lmVtVmTxGRrsQkzClWNS8HrirVfNms7$
>> Signed-off-by: Anjali Kulkarni <anjali.k.kulkarni@oracle.com>
>> ---
>> drivers/connector/cn_proc.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c
>> index 05d562e9c8b1..a8e55569e4f5 100644
>> --- a/drivers/connector/cn_proc.c
>> +++ b/drivers/connector/cn_proc.c
>> @@ -54,7 +54,7 @@ static int cn_filter(struct sock *dsk, struct sk_buff *skb, void *data)
>> enum proc_cn_mcast_op mc_op;
>> uintptr_t val;
>>
>> - if (!dsk || !data)
>> + if (!dsk || !data || !dsk->sk_user_data)
>> return 0;
>>
>> ptr = (__u32 *)data;
>> --
>> 2.42.0
>>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v1] Fix NULL pointer dereference in cn_filter()
2023-10-17 8:02 ` Simon Horman
2023-10-17 18:23 ` Anjali Kulkarni
@ 2023-10-20 23:30 ` Anjali Kulkarni
1 sibling, 0 replies; 4+ messages in thread
From: Anjali Kulkarni @ 2023-10-20 23:30 UTC (permalink / raw)
To: Simon Horman
Cc: open list, David S. Miller, Liam Howlett, Netdev,
kernel test robot, Jakub Kicinski, Anjali Kulkarni
> On Oct 17, 2023, at 1:02 AM, Simon Horman <horms@kernel.org> wrote:
>
> On Fri, Oct 13, 2023 at 03:56:19PM -0700, Anjali Kulkarni wrote:
>> Check that sk_user_data is not NULL, else return from cn_filter().
>
> Thanks,
>
> I agree that this change seems likely to address the problem at the link
> below. And I also think cn_filter() is a good place to fix this [1].
> But I am wondering if you could add some commentary to the patch
> description, describing under what circumstances this problem can occur.
Hi,
We always allocate sk_user_data in cn_proc_mcast_ctl(), which should ideally
happen before any fork event is triggered, which ends up calling cn_filter(),
where we need to use sk_user_data. So in the normal flow of events, sk_user_data
should not be NULL in cn_filter(). I also looked at whether there were any race
conditions which could cause this issue and found one possibility - if a fork
event was triggered between the time that the bind() call was made by the
application (which ends up adding the socket to the multicast list for CONNECTOR
maintained by netlink layer), and the call to cn_proc_mcast_ctl(), then the netlink
layer will try to send the event to the sockets in the multicast list via cn_filter() and find that sk_user_data is NULL.
However, when I tried to reproduce this case, I could not do it by adding a wait
between bind() & cn_proc_mcast_ctl(). On looking at the code, I did see that
in proc_fork_connector(), we do check for proc_event_num_listeners being greater
than 0 before we invoke netlink layer's send message, which calls cn_filter(). And
proc_event_num_listeners is only incremented after sk_user_data is allocated. This
is why we cannot reproduce it this way. Perhaps there is some other way I
am not aware of, but I could not think of anything else.
I will just resend the patch.
Thanks
Anjali
>
> [1] https://urldefense.com/v3/__https://lore.kernel.org/all/20231013120105.GH29570@kernel.org/__;!!ACWV5N9M2RV99hQ!NNsMHaho3lyW2NyoT8Sju1BL78S5pNu5AhtZC76cc1Xb1_psXwfP0lmVtVmTxGRrsQkzClWNS8HriosTMols$
>
>> Fixes: 2aa1f7a1f47c ("connector/cn_proc: Add filtering to fix some bugs")
>> Reported-by: kernel test robot <oliver.sang@intel.com>
>> Closes: https://urldefense.com/v3/__https://lore.kernel.org/oe-lkp/202309201456.84c19e27-oliver.sang@intel.com/__;!!ACWV5N9M2RV99hQ!NNsMHaho3lyW2NyoT8Sju1BL78S5pNu5AhtZC76cc1Xb1_psXwfP0lmVtVmTxGRrsQkzClWNS8HrirVfNms7$
>> Signed-off-by: Anjali Kulkarni <anjali.k.kulkarni@oracle.com>
>> ---
>> drivers/connector/cn_proc.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c
>> index 05d562e9c8b1..a8e55569e4f5 100644
>> --- a/drivers/connector/cn_proc.c
>> +++ b/drivers/connector/cn_proc.c
>> @@ -54,7 +54,7 @@ static int cn_filter(struct sock *dsk, struct sk_buff *skb, void *data)
>> enum proc_cn_mcast_op mc_op;
>> uintptr_t val;
>>
>> - if (!dsk || !data)
>> + if (!dsk || !data || !dsk->sk_user_data)
>> return 0;
>>
>> ptr = (__u32 *)data;
>> --
>> 2.42.0
>>
^ permalink raw reply [flat|nested] 4+ messages in thread