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' equal 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 for similar
cases.
Reported-by: Abaci <redacted>
Signed-off-by: Michael Wang <redacted>
---
net/ipv4/cipso_ipv4.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
From: Paul Moore <paul@paul-moore.com> Date: 2021-08-27 00:09:32
On Wed, Aug 25, 2021 at 11:42 PM 王贇 [off-list ref] 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' equal 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 for similar
cases.
Reported-by: Abaci <redacted>
Signed-off-by: Michael Wang <redacted>
---
net/ipv4/cipso_ipv4.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
Thanks for the problem report. It's hard to say for certain due to
the abbreviated backtrace without line number information, but it
looks like the problem you are describing is happening when the
allocation for doi_def->map.std fails near the top of
netlbl_cipsov4_add_std() which causes the function to jump the
add_std_failure target which ends up calling cipso_v4_doi_free().
doi_def = kmalloc(sizeof(*doi_def), GFP_KERNEL);
if (doi_def == NULL)
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;
}
...
add_std_failure:
cipso_v4_doi_free(doi_def);
Since the doi_def allocation is not zero'd out, it is possible that
the doi_def->type value could have a value of CIPSO_V4_MAP_TRANS when
the doi_def->map.std allocation fails, causing the NULL pointer deref
in cipso_v4_doi_free(). As this is the only case where we would see a
problem like this, I suggest a better solution would be to change the
if-block following the doi_def->map.std allocation to something like
this:
doi_def->map.std = kzalloc(sizeof(*doi_def->map.std), GFP_KERNEL);
if (doi_def->map.std == NULL) {
kfree(doi_def);
return -ENOMEM;
}
Hi, Paul
I'm sorry for missing this mail since my stupid filter rules...
Will send a new one soon as you suggested :-)
Regards,
Michael Wang
On 2021/8/27 上午8:09, Paul Moore wrote:
[snip]
quoted
Reported-by: Abaci <redacted>
Signed-off-by: Michael Wang <redacted>
---
net/ipv4/cipso_ipv4.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
Thanks for the problem report. It's hard to say for certain due to
the abbreviated backtrace without line number information, but it
looks like the problem you are describing is happening when the
allocation for doi_def->map.std fails near the top of
netlbl_cipsov4_add_std() which causes the function to jump the
add_std_failure target which ends up calling cipso_v4_doi_free().
doi_def = kmalloc(sizeof(*doi_def), GFP_KERNEL);
if (doi_def == NULL)
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;
}
...
add_std_failure:
cipso_v4_doi_free(doi_def);
Since the doi_def allocation is not zero'd out, it is possible that
the doi_def->type value could have a value of CIPSO_V4_MAP_TRANS when
the doi_def->map.std allocation fails, causing the NULL pointer deref
in cipso_v4_doi_free(). As this is the only case where we would see a
problem like this, I suggest a better solution would be to change the
if-block following the doi_def->map.std allocation to something like
this:
doi_def->map.std = kzalloc(sizeof(*doi_def->map.std), GFP_KERNEL);
if (doi_def->map.std == NULL) {
kfree(doi_def);
return -ENOMEM;
}
Just a ping... Should we fix this?
Regards,
Michael Wang
On 2021/8/26 上午11:42, 王贇 wrote:
quoted hunk
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' equal 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 for similar
cases.
Reported-by: Abaci <redacted>
Signed-off-by: Michael Wang <redacted>
---
net/ipv4/cipso_ipv4.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
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 <redacted>
Signed-off-by: Michael Wang <redacted>
---
net/netlabel/netlabel_cipso_v4.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Paul Moore <paul@paul-moore.com> Date: 2021-08-30 14:17:22
On Mon, Aug 30, 2021 at 6:28 AM 王贇 [off-list ref] 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 <redacted>
Signed-off-by: Michael Wang <redacted>
---
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.
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-08-30 16:45:29
On Mon, 30 Aug 2021 10:17:05 -0400 Paul Moore wrote:
On Mon, Aug 30, 2021 at 6:28 AM 王贇 [off-list ref] wrote:
quoted
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 <redacted>
Signed-off-by: Michael Wang <redacted>
---
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.
From: Paul Moore <paul@paul-moore.com> Date: 2021-08-30 16:50:48
On Mon, Aug 30, 2021 at 12:45 PM Jakub Kicinski [off-list ref] wrote:
On Mon, 30 Aug 2021 10:17:05 -0400 Paul Moore wrote:
quoted
On Mon, Aug 30, 2021 at 6:28 AM 王贇 [off-list ref] wrote:
quoted
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 <redacted>
Signed-off-by: Michael Wang <redacted>
---
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.
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?
--
paul moore
www.paul-moore.com
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
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.
--
paul moore
www.paul-moore.com
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
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.
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
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.
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
This reverts commit 733c99ee8be9a1410287cdbb943887365e83b2d6.
Since commit e842cb60e8ac ("net: fix NULL pointer reference in
cipso_v4_doi_free") also applied to fix the root cause, we can
just revert the old version now.
Suggested-by: Paul Moore <paul@paul-moore.com>
Signed-off-by: Michael Wang <redacted>
---
net/ipv4/cipso_ipv4.c | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)
Hi Paul, it confused me since it's the first time I face
such situation, but I just realized what you're asking is
actually this revert, correct?
Regards,
Michael Wang
On 2021/9/1 上午10:18, 王贇 wrote:
quoted hunk
This reverts commit 733c99ee8be9a1410287cdbb943887365e83b2d6.
Since commit e842cb60e8ac ("net: fix NULL pointer reference in
cipso_v4_doi_free") also applied to fix the root cause, we can
just revert the old version now.
Suggested-by: Paul Moore <paul@paul-moore.com>
Signed-off-by: Michael Wang <redacted>
---
net/ipv4/cipso_ipv4.c | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)
From: Paul Moore <paul@paul-moore.com> Date: 2021-09-01 21:05:53
On Tue, Aug 31, 2021 at 10:21 PM 王贇 [off-list ref] wrote:
Hi Paul, it confused me since it's the first time I face
such situation, but I just realized what you're asking is
actually this revert, correct?
I believe DaveM already answered your question in the other thread,
but if you are still unsure about the patch let me know.
--
paul moore
www.paul-moore.com
On Tue, Aug 31, 2021 at 10:21 PM 王贇 [off-list ref] wrote:
quoted
Hi Paul, it confused me since it's the first time I face
such situation, but I just realized what you're asking is
actually this revert, correct?
I believe DaveM already answered your question in the other thread,
but if you are still unsure about the patch let me know.
I do failed to get the point :-(
As I checked the:
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git
both v1 and v2 are there with the same description and both code modification
are applied.
We want revert v1 but not in a revert patch style, then do you suggest
send a normal patch to do the code revert?
Regards,
Michael Wang
From: Paul Moore <paul@paul-moore.com> Date: 2021-09-03 02:15:59
On Wed, Sep 1, 2021 at 10:37 PM 王贇 [off-list ref] wrote:
On 2021/9/2 上午5:05, Paul Moore wrote:
quoted
On Tue, Aug 31, 2021 at 10:21 PM 王贇 [off-list ref] wrote:
quoted
Hi Paul, it confused me since it's the first time I face
such situation, but I just realized what you're asking is
actually this revert, correct?
I believe DaveM already answered your question in the other thread,
but if you are still unsure about the patch let me know.
I do failed to get the point :-(
As I checked the:
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git
both v1 and v2 are there with the same description and both code modification
are applied.
We want revert v1 but not in a revert patch style, then do you suggest
send a normal patch to do the code revert?
It sounds like DaveM wants you to create a normal (not a revert) patch
that removes the v1 changes while leaving the v2 changes intact. In
the patch description you can mention that v1 was merged as a mistake
and that v2 is the correct fix (provide commit IDs for each in your
commit description using the usual 12-char hash snippet followed by
the subject in parens-and-quotes).
--
paul moore
www.paul-moore.com
both v1 and v2 are there with the same description and both code modification
are applied.
We want revert v1 but not in a revert patch style, then do you suggest
send a normal patch to do the code revert?
It sounds like DaveM wants you to create a normal (not a revert) patch
that removes the v1 changes while leaving the v2 changes intact. In
the patch description you can mention that v1 was merged as a mistake
and that v2 is the correct fix (provide commit IDs for each in your
commit description using the usual 12-char hash snippet followed by
the subject in parens-and-quotes).
Thanks for the kindly explain, I've sent:
[PATCH] net: remove the unnecessary check in cipso_v4_doi_free
Which actually revert the v1 and mentioned v2 fixed the root casue,
Would you please take a look see if that is helpful?
Regards,
Michael Wang
From: Paul Moore <paul@paul-moore.com> Date: 2021-09-03 14:09:02
On Thu, Sep 2, 2021 at 10:31 PM 王贇 [off-list ref] wrote:
On 2021/9/3 上午10:15, Paul Moore wrote:
[snip]
quoted
quoted
both v1 and v2 are there with the same description and both code modification
are applied.
We want revert v1 but not in a revert patch style, then do you suggest
send a normal patch to do the code revert?
It sounds like DaveM wants you to create a normal (not a revert) patch
that removes the v1 changes while leaving the v2 changes intact. In
the patch description you can mention that v1 was merged as a mistake
and that v2 is the correct fix (provide commit IDs for each in your
commit description using the usual 12-char hash snippet followed by
the subject in parens-and-quotes).
Thanks for the kindly explain, I've sent:
[PATCH] net: remove the unnecessary check in cipso_v4_doi_free
Which actually revert the v1 and mentioned v2 fixed the root casue,
Would you please take a look see if that is helpful?
That looks correct to me, acked. Thanks.
--
paul moore
www.paul-moore.com
The commit 733c99ee8be9 ("net: fix NULL pointer reference in
cipso_v4_doi_free") was merged by a mistake, this patch try
to cleanup the mess.
And we already have the commit e842cb60e8ac ("net: fix NULL
pointer reference in cipso_v4_doi_free") which fixed the root
cause of the issue mentioned in it's description.
Suggested-by: Paul Moore <paul@paul-moore.com>
Signed-off-by: Michael Wang <redacted>
---
net/ipv4/cipso_ipv4.c | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)
From: Paul Moore <paul@paul-moore.com> Date: 2021-09-03 14:08:17
On Thu, Sep 2, 2021 at 10:27 PM 王贇 [off-list ref] wrote:
The commit 733c99ee8be9 ("net: fix NULL pointer reference in
cipso_v4_doi_free") was merged by a mistake, this patch try
to cleanup the mess.
And we already have the commit e842cb60e8ac ("net: fix NULL
pointer reference in cipso_v4_doi_free") which fixed the root
cause of the issue mentioned in it's description.
Suggested-by: Paul Moore <paul@paul-moore.com>
Signed-off-by: Michael Wang <redacted>
---
net/ipv4/cipso_ipv4.c | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)
Verified that the v2 patch is in net/master so removing the v1 patch
as this does is the right thing to do. Thanks for sending this out.
Acked-by: Paul Moore <paul@paul-moore.com>