| Message ID | 20231013225619.987912-1-anjali.k.kulkarni@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v1] Fix NULL pointer dereference in cn_filter() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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. [1] https://lore.kernel.org/all/20231013120105.GH29570@kernel.org/ > Fixes: 2aa1f7a1f47c ("connector/cn_proc: Add filtering to fix some bugs") > Reported-by: kernel test robot <oliver.sang@intel.com> > Closes: https://lore.kernel.org/oe-lkp/202309201456.84c19e27-oliver.sang@intel.com/ > 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 >
> 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 >>
> 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 >>
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;
Check that sk_user_data is not NULL, else return from cn_filter(). Fixes: 2aa1f7a1f47c ("connector/cn_proc: Add filtering to fix some bugs") Reported-by: kernel test robot <oliver.sang@intel.com> Closes: https://lore.kernel.org/oe-lkp/202309201456.84c19e27-oliver.sang@intel.com/ Signed-off-by: Anjali Kulkarni <anjali.k.kulkarni@oracle.com> --- drivers/connector/cn_proc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)