| Message ID | 18f0171e-0cc8-6ae6-d04a-a69a2a3c1a39@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] net: fix NULL pointer reference in cipso_v4_doi_free | expand |
Hello: This patch was applied to netdev/net-next.git (refs/heads/master): On Mon, 30 Aug 2021 18:28:01 +0800 you wrote: > In netlbl_cipsov4_add_std() when 'doi_def->map.std' alloc > failed, we sometime observe panic: > > BUG: kernel NULL pointer dereference, address: > ... > RIP: 0010:cipso_v4_doi_free+0x3a/0x80 > ... > Call Trace: > netlbl_cipsov4_add_std+0xf4/0x8c0 > netlbl_cipsov4_add+0x13f/0x1b0 > genl_family_rcv_msg_doit.isra.15+0x132/0x170 > genl_rcv_msg+0x125/0x240 > > [...] Here is the summary with links: - [v2] net: fix NULL pointer reference in cipso_v4_doi_free https://git.kernel.org/netdev/net-next/c/e842cb60e8ac You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
On Mon, Aug 30, 2021 at 6:28 AM 王贇 <yun.wang@linux.alibaba.com> wrote: > > In netlbl_cipsov4_add_std() when 'doi_def->map.std' alloc > failed, we sometime observe panic: > > BUG: kernel NULL pointer dereference, address: > ... > RIP: 0010:cipso_v4_doi_free+0x3a/0x80 > ... > Call Trace: > netlbl_cipsov4_add_std+0xf4/0x8c0 > netlbl_cipsov4_add+0x13f/0x1b0 > genl_family_rcv_msg_doit.isra.15+0x132/0x170 > genl_rcv_msg+0x125/0x240 > > This is because in cipso_v4_doi_free() there is no check > on 'doi_def->map.std' when doi_def->type got value 1, which > is possibe, since netlbl_cipsov4_add_std() haven't initialize > it before alloc 'doi_def->map.std'. > > This patch just add the check to prevent panic happen in similar > cases. > > Reported-by: Abaci <abaci@linux.alibaba.com> > Signed-off-by: Michael Wang <yun.wang@linux.alibaba.com> > --- > net/netlabel/netlabel_cipso_v4.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) I see this was already merged, but it looks good to me, thanks for making those changes. > diff --git a/net/netlabel/netlabel_cipso_v4.c b/net/netlabel/netlabel_cipso_v4.c > index baf2357..344c228 100644 > --- a/net/netlabel/netlabel_cipso_v4.c > +++ b/net/netlabel/netlabel_cipso_v4.c > @@ -144,8 +144,8 @@ static int netlbl_cipsov4_add_std(struct genl_info *info, > return -ENOMEM; > doi_def->map.std = kzalloc(sizeof(*doi_def->map.std), GFP_KERNEL); > if (doi_def->map.std == NULL) { > - ret_val = -ENOMEM; > - goto add_std_failure; > + kfree(doi_def); > + return -ENOMEM; > } > doi_def->type = CIPSO_V4_MAP_TRANS; > > -- > 1.8.3.1
On Mon, 30 Aug 2021 10:17:05 -0400 Paul Moore wrote: > On Mon, Aug 30, 2021 at 6:28 AM 王贇 <yun.wang@linux.alibaba.com> wrote: > > > > In netlbl_cipsov4_add_std() when 'doi_def->map.std' alloc > > failed, we sometime observe panic: > > > > BUG: kernel NULL pointer dereference, address: > > ... > > RIP: 0010:cipso_v4_doi_free+0x3a/0x80 > > ... > > Call Trace: > > netlbl_cipsov4_add_std+0xf4/0x8c0 > > netlbl_cipsov4_add+0x13f/0x1b0 > > genl_family_rcv_msg_doit.isra.15+0x132/0x170 > > genl_rcv_msg+0x125/0x240 > > > > This is because in cipso_v4_doi_free() there is no check > > on 'doi_def->map.std' when doi_def->type got value 1, which > > is possibe, since netlbl_cipsov4_add_std() haven't initialize > > it before alloc 'doi_def->map.std'. > > > > This patch just add the check to prevent panic happen in similar > > cases. > > > > Reported-by: Abaci <abaci@linux.alibaba.com> > > Signed-off-by: Michael Wang <yun.wang@linux.alibaba.com> > > --- > > net/netlabel/netlabel_cipso_v4.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > I see this was already merged, but it looks good to me, thanks for > making those changes. FWIW it looks like v1 was also merged: https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=733c99ee8b
On Mon, Aug 30, 2021 at 12:45 PM Jakub Kicinski <kuba@kernel.org> wrote: > On Mon, 30 Aug 2021 10:17:05 -0400 Paul Moore wrote: > > On Mon, Aug 30, 2021 at 6:28 AM 王贇 <yun.wang@linux.alibaba.com> wrote: > > > > > > In netlbl_cipsov4_add_std() when 'doi_def->map.std' alloc > > > failed, we sometime observe panic: > > > > > > BUG: kernel NULL pointer dereference, address: > > > ... > > > RIP: 0010:cipso_v4_doi_free+0x3a/0x80 > > > ... > > > Call Trace: > > > netlbl_cipsov4_add_std+0xf4/0x8c0 > > > netlbl_cipsov4_add+0x13f/0x1b0 > > > genl_family_rcv_msg_doit.isra.15+0x132/0x170 > > > genl_rcv_msg+0x125/0x240 > > > > > > This is because in cipso_v4_doi_free() there is no check > > > on 'doi_def->map.std' when doi_def->type got value 1, which > > > is possibe, since netlbl_cipsov4_add_std() haven't initialize > > > it before alloc 'doi_def->map.std'. > > > > > > This patch just add the check to prevent panic happen in similar > > > cases. > > > > > > Reported-by: Abaci <abaci@linux.alibaba.com> > > > Signed-off-by: Michael Wang <yun.wang@linux.alibaba.com> > > > --- > > > net/netlabel/netlabel_cipso_v4.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > I see this was already merged, but it looks good to me, thanks for > > making those changes. > > FWIW it looks like v1 was also merged: > > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=733c99ee8b Yeah, that is unfortunate, there was a brief discussion about that over on one of the -stable patches for the v1 patch (odd that I never saw a patchbot post for the v1 patch?). Having both merged should be harmless, but we want to revert the v1 patch as soon as we can. Michael, can you take care of this?
On 2021/8/31 上午12:50, Paul Moore wrote: [SNIP] >>>> Reported-by: Abaci <abaci@linux.alibaba.com> >>>> Signed-off-by: Michael Wang <yun.wang@linux.alibaba.com> >>>> --- >>>> net/netlabel/netlabel_cipso_v4.c | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> I see this was already merged, but it looks good to me, thanks for >>> making those changes. >> >> FWIW it looks like v1 was also merged: >> >> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=733c99ee8b > > Yeah, that is unfortunate, there was a brief discussion about that > over on one of the -stable patches for the v1 patch (odd that I never > saw a patchbot post for the v1 patch?). Having both merged should be > harmless, but we want to revert the v1 patch as soon as we can. > Michael, can you take care of this? As v1 already merged, may be we could just goon with it? Actually both working to fix the problem, v1 will cover all the cases, v2 take care one case since that's currently the only one, but maybe there will be more in future. Regards, Michael Wang >
On Mon, Aug 30, 2021 at 10:42 PM 王贇 <yun.wang@linux.alibaba.com> wrote: > On 2021/8/31 上午12:50, Paul Moore wrote: > [SNIP] > >>>> Reported-by: Abaci <abaci@linux.alibaba.com> > >>>> Signed-off-by: Michael Wang <yun.wang@linux.alibaba.com> > >>>> --- > >>>> net/netlabel/netlabel_cipso_v4.c | 4 ++-- > >>>> 1 file changed, 2 insertions(+), 2 deletions(-) > >>> > >>> I see this was already merged, but it looks good to me, thanks for > >>> making those changes. > >> > >> FWIW it looks like v1 was also merged: > >> > >> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=733c99ee8b > > > > Yeah, that is unfortunate, there was a brief discussion about that > > over on one of the -stable patches for the v1 patch (odd that I never > > saw a patchbot post for the v1 patch?). Having both merged should be > > harmless, but we want to revert the v1 patch as soon as we can. > > Michael, can you take care of this? > > As v1 already merged, may be we could just goon with it? > > Actually both working to fix the problem, v1 will cover all the > cases, v2 take care one case since that's currently the only one, > but maybe there will be more in future. No. Please revert v1 and stick with the v2 patch. The v1 patch is in my opinion a rather ugly hack that addresses the symptom of the problem and not the root cause. It isn't your fault that both v1 and v2 were merged, but I'm asking you to help cleanup the mess. If you aren't able to do that please let us know so that others can fix this properly.
On 2021/8/31 下午9:48, Paul Moore wrote: > On Mon, Aug 30, 2021 at 10:42 PM 王贇 <yun.wang@linux.alibaba.com> wrote: >> On 2021/8/31 上午12:50, Paul Moore wrote: >> [SNIP] >>>>>> Reported-by: Abaci <abaci@linux.alibaba.com> >>>>>> Signed-off-by: Michael Wang <yun.wang@linux.alibaba.com> >>>>>> --- >>>>>> net/netlabel/netlabel_cipso_v4.c | 4 ++-- >>>>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>>> >>>>> I see this was already merged, but it looks good to me, thanks for >>>>> making those changes. >>>> >>>> FWIW it looks like v1 was also merged: >>>> >>>> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=733c99ee8b >>> >>> Yeah, that is unfortunate, there was a brief discussion about that >>> over on one of the -stable patches for the v1 patch (odd that I never >>> saw a patchbot post for the v1 patch?). Having both merged should be >>> harmless, but we want to revert the v1 patch as soon as we can. >>> Michael, can you take care of this? >> >> As v1 already merged, may be we could just goon with it? >> >> Actually both working to fix the problem, v1 will cover all the >> cases, v2 take care one case since that's currently the only one, >> but maybe there will be more in future. > > No. Please revert v1 and stick with the v2 patch. The v1 patch is in > my opinion a rather ugly hack that addresses the symptom of the > problem and not the root cause. > > It isn't your fault that both v1 and v2 were merged, but I'm asking > you to help cleanup the mess. If you aren't able to do that please > let us know so that others can fix this properly. No problem I can help on that, just try to make sure it's not a meaningless work. So would it be fine to send out a v3 which revert v1 and apply v2? Regards, Michael Wang >
From: 王贇 <yun.wang@linux.alibaba.com> Date: Wed, 1 Sep 2021 09:51:28 +0800 > > > On 2021/8/31 下午9:48, Paul Moore wrote: >> On Mon, Aug 30, 2021 at 10:42 PM 王贇 <yun.wang@linux.alibaba.com> wrote: >>> On 2021/8/31 上午12:50, Paul Moore wrote: >>> [SNIP] >>>>>>> Reported-by: Abaci <abaci@linux.alibaba.com> >>>>>>> Signed-off-by: Michael Wang <yun.wang@linux.alibaba.com> >>>>>>> --- >>>>>>> net/netlabel/netlabel_cipso_v4.c | 4 ++-- >>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>>>> >>>>>> I see this was already merged, but it looks good to me, thanks for >>>>>> making those changes. >>>>> >>>>> FWIW it looks like v1 was also merged: >>>>> >>>>> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=733c99ee8b >>>> >>>> Yeah, that is unfortunate, there was a brief discussion about that >>>> over on one of the -stable patches for the v1 patch (odd that I never >>>> saw a patchbot post for the v1 patch?). Having both merged should be >>>> harmless, but we want to revert the v1 patch as soon as we can. >>>> Michael, can you take care of this? >>> >>> As v1 already merged, may be we could just goon with it? >>> >>> Actually both working to fix the problem, v1 will cover all the >>> cases, v2 take care one case since that's currently the only one, >>> but maybe there will be more in future. >> >> No. Please revert v1 and stick with the v2 patch. The v1 patch is in >> my opinion a rather ugly hack that addresses the symptom of the >> problem and not the root cause. >> >> It isn't your fault that both v1 and v2 were merged, but I'm asking >> you to help cleanup the mess. If you aren't able to do that please >> let us know so that others can fix this properly. > > No problem I can help on that, just try to make sure it's not a > meaningless work. > > So would it be fine to send out a v3 which revert v1 and apply v2? Please don't do things this way just send the relative change. Thanks.
On 2021/9/1 下午5:30, David Miller wrote: > From: 王贇 <yun.wang@linux.alibaba.com> > Date: Wed, 1 Sep 2021 09:51:28 +0800 > >> >> >> On 2021/8/31 下午9:48, Paul Moore wrote: >>> On Mon, Aug 30, 2021 at 10:42 PM 王贇 <yun.wang@linux.alibaba.com> wrote: >>>> On 2021/8/31 上午12:50, Paul Moore wrote: >>>> [SNIP] >>>>>>>> Reported-by: Abaci <abaci@linux.alibaba.com> >>>>>>>> Signed-off-by: Michael Wang <yun.wang@linux.alibaba.com> >>>>>>>> --- >>>>>>>> net/netlabel/netlabel_cipso_v4.c | 4 ++-- >>>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>>>>> >>>>>>> I see this was already merged, but it looks good to me, thanks for >>>>>>> making those changes. >>>>>> >>>>>> FWIW it looks like v1 was also merged: >>>>>> >>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=733c99ee8b >>>>> >>>>> Yeah, that is unfortunate, there was a brief discussion about that >>>>> over on one of the -stable patches for the v1 patch (odd that I never >>>>> saw a patchbot post for the v1 patch?). Having both merged should be >>>>> harmless, but we want to revert the v1 patch as soon as we can. >>>>> Michael, can you take care of this? >>>> >>>> As v1 already merged, may be we could just goon with it? >>>> >>>> Actually both working to fix the problem, v1 will cover all the >>>> cases, v2 take care one case since that's currently the only one, >>>> but maybe there will be more in future. >>> >>> No. Please revert v1 and stick with the v2 patch. The v1 patch is in >>> my opinion a rather ugly hack that addresses the symptom of the >>> problem and not the root cause. >>> >>> It isn't your fault that both v1 and v2 were merged, but I'm asking >>> you to help cleanup the mess. If you aren't able to do that please >>> let us know so that others can fix this properly. >> >> No problem I can help on that, just try to make sure it's not a >> meaningless work. >> >> So would it be fine to send out a v3 which revert v1 and apply v2? > > Please don't do things this way just send the relative change. Could you please check the patch: Revert "net: fix NULL pointer reference in cipso_v4_doi_free" see if that's the right way? Regards, Michael Wang > > Thanks. >
From: 王贇 <yun.wang@linux.alibaba.com> Date: Wed, 1 Sep 2021 17:41:00 +0800 > > > On 2021/9/1 下午5:30, David Miller wrote: >> From: 王贇 <yun.wang@linux.alibaba.com> >> Date: Wed, 1 Sep 2021 09:51:28 +0800 >> >>> >>> >>> On 2021/8/31 下午9:48, Paul Moore wrote: >>>> On Mon, Aug 30, 2021 at 10:42 PM 王贇 <yun.wang@linux.alibaba.com> wrote: >>>>> On 2021/8/31 上午12:50, Paul Moore wrote: >>>>> [SNIP] >>>>>>>>> Reported-by: Abaci <abaci@linux.alibaba.com> >>>>>>>>> Signed-off-by: Michael Wang <yun.wang@linux.alibaba.com> >>>>>>>>> --- >>>>>>>>> net/netlabel/netlabel_cipso_v4.c | 4 ++-- >>>>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>>>>>> >>>>>>>> I see this was already merged, but it looks good to me, thanks for >>>>>>>> making those changes. >>>>>>> >>>>>>> FWIW it looks like v1 was also merged: >>>>>>> >>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=733c99ee8b >>>>>> >>>>>> Yeah, that is unfortunate, there was a brief discussion about that >>>>>> over on one of the -stable patches for the v1 patch (odd that I never >>>>>> saw a patchbot post for the v1 patch?). Having both merged should be >>>>>> harmless, but we want to revert the v1 patch as soon as we can. >>>>>> Michael, can you take care of this? >>>>> >>>>> As v1 already merged, may be we could just goon with it? >>>>> >>>>> Actually both working to fix the problem, v1 will cover all the >>>>> cases, v2 take care one case since that's currently the only one, >>>>> but maybe there will be more in future. >>>> >>>> No. Please revert v1 and stick with the v2 patch. The v1 patch is in >>>> my opinion a rather ugly hack that addresses the symptom of the >>>> problem and not the root cause. >>>> >>>> It isn't your fault that both v1 and v2 were merged, but I'm asking >>>> you to help cleanup the mess. If you aren't able to do that please >>>> let us know so that others can fix this properly. >>> >>> No problem I can help on that, just try to make sure it's not a >>> meaningless work. >>> >>> So would it be fine to send out a v3 which revert v1 and apply v2? >> >> Please don't do things this way just send the relative change. > > Could you please check the patch: > > Revert "net: fix NULL pointer reference in cipso_v4_doi_free" > > see if that's the right way? It is not. Please just send a patch against the net GIT tree which relatively changes the code to match v2 of your change. Thank you.
On 2021/9/1 下午6:45, David Miller wrote: [snip] >>>>> >>>>> It isn't your fault that both v1 and v2 were merged, but I'm asking >>>>> you to help cleanup the mess. If you aren't able to do that please >>>>> let us know so that others can fix this properly. >>>> >>>> No problem I can help on that, just try to make sure it's not a >>>> meaningless work. >>>> >>>> So would it be fine to send out a v3 which revert v1 and apply v2? >>> >>> Please don't do things this way just send the relative change. >> >> Could you please check the patch: >> >> Revert "net: fix NULL pointer reference in cipso_v4_doi_free" >> >> see if that's the right way? > > It is not. Please just send a patch against the net GIT tree which relatively changes the code to match v2 of your change. Sorry for my horrible reading comprehension... I checked netdev/net.git master branch and saw v2's change already applied, thus I've no idea how to change it again but pretty sure I still misunderstanding the suggestion, could please kindly provide more details? Regards, Michael Wang > > Thank you. >
diff --git a/net/netlabel/netlabel_cipso_v4.c b/net/netlabel/netlabel_cipso_v4.c index baf2357..344c228 100644 --- a/net/netlabel/netlabel_cipso_v4.c +++ b/net/netlabel/netlabel_cipso_v4.c @@ -144,8 +144,8 @@ static int netlbl_cipsov4_add_std(struct genl_info *info, return -ENOMEM; doi_def->map.std = kzalloc(sizeof(*doi_def->map.std), GFP_KERNEL); if (doi_def->map.std == NULL) { - ret_val = -ENOMEM; - goto add_std_failure; + kfree(doi_def); + return -ENOMEM; } doi_def->type = CIPSO_V4_MAP_TRANS;
In netlbl_cipsov4_add_std() when 'doi_def->map.std' alloc failed, we sometime observe panic: BUG: kernel NULL pointer dereference, address: ... RIP: 0010:cipso_v4_doi_free+0x3a/0x80 ... Call Trace: netlbl_cipsov4_add_std+0xf4/0x8c0 netlbl_cipsov4_add+0x13f/0x1b0 genl_family_rcv_msg_doit.isra.15+0x132/0x170 genl_rcv_msg+0x125/0x240 This is because in cipso_v4_doi_free() there is no check on 'doi_def->map.std' when doi_def->type got value 1, which is possibe, since netlbl_cipsov4_add_std() haven't initialize it before alloc 'doi_def->map.std'. This patch just add the check to prevent panic happen in similar cases. Reported-by: Abaci <abaci@linux.alibaba.com> Signed-off-by: Michael Wang <yun.wang@linux.alibaba.com> --- net/netlabel/netlabel_cipso_v4.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)