| Message ID | c6864908-d093-1705-76ce-94d6af85e092@linux.alibaba.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: fix NULL pointer reference in cipso_v4_doi_free | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | success | CCed 7 of 7 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 1 this patch: 1 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | WARNING: From:/Signed-off-by: email name mismatch: 'From: "王贇" <yun.wang@linux.alibaba.com>' != 'Signed-off-by: Michael Wang <yun.wang@linux.alibaba.com>' |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 1 this patch: 1 |
| netdev/header_inline | success | Link |
On Wed, Aug 25, 2021 at 11:42 PM 王贇 <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' 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 <abaci@linux.alibaba.com> > Signed-off-by: Michael Wang <yun.wang@linux.alibaba.com> > --- > > 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; } > diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c > index 099259f..7fbd0b5 100644 > --- a/net/ipv4/cipso_ipv4.c > +++ b/net/ipv4/cipso_ipv4.c > @@ -465,14 +465,16 @@ void cipso_v4_doi_free(struct cipso_v4_doi *doi_def) > if (!doi_def) > return; > > - switch (doi_def->type) { > - case CIPSO_V4_MAP_TRANS: > - kfree(doi_def->map.std->lvl.cipso); > - kfree(doi_def->map.std->lvl.local); > - kfree(doi_def->map.std->cat.cipso); > - kfree(doi_def->map.std->cat.local); > - kfree(doi_def->map.std); > - break; > + if (doi_def->map.std) { > + switch (doi_def->type) { > + case CIPSO_V4_MAP_TRANS: > + kfree(doi_def->map.std->lvl.cipso); > + kfree(doi_def->map.std->lvl.local); > + kfree(doi_def->map.std->cat.cipso); > + kfree(doi_def->map.std->cat.local); > + kfree(doi_def->map.std); > + break; > + } > } > kfree(doi_def); > } > -- > 1.8.3.1 >
Just a ping... Should we fix this? Regards, Michael Wang On 2021/8/26 上午11:42, 王贇 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 <abaci@linux.alibaba.com> > Signed-off-by: Michael Wang <yun.wang@linux.alibaba.com> > --- > > net/ipv4/cipso_ipv4.c | 18 ++++++++++-------- > 1 file changed, 10 insertions(+), 8 deletions(-) > > diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c > index 099259f..7fbd0b5 100644 > --- a/net/ipv4/cipso_ipv4.c > +++ b/net/ipv4/cipso_ipv4.c > @@ -465,14 +465,16 @@ void cipso_v4_doi_free(struct cipso_v4_doi *doi_def) > if (!doi_def) > return; > > - switch (doi_def->type) { > - case CIPSO_V4_MAP_TRANS: > - kfree(doi_def->map.std->lvl.cipso); > - kfree(doi_def->map.std->lvl.local); > - kfree(doi_def->map.std->cat.cipso); > - kfree(doi_def->map.std->cat.local); > - kfree(doi_def->map.std); > - break; > + if (doi_def->map.std) { > + switch (doi_def->type) { > + case CIPSO_V4_MAP_TRANS: > + kfree(doi_def->map.std->lvl.cipso); > + kfree(doi_def->map.std->lvl.local); > + kfree(doi_def->map.std->cat.cipso); > + kfree(doi_def->map.std->cat.local); > + kfree(doi_def->map.std); > + break; > + } > } > kfree(doi_def); > } >
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] >> >> Reported-by: Abaci <abaci@linux.alibaba.com> >> Signed-off-by: Michael Wang <yun.wang@linux.alibaba.com> >> --- >> >> 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; > } > >> diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c >> index 099259f..7fbd0b5 100644 >> --- a/net/ipv4/cipso_ipv4.c >> +++ b/net/ipv4/cipso_ipv4.c >> @@ -465,14 +465,16 @@ void cipso_v4_doi_free(struct cipso_v4_doi *doi_def) >> if (!doi_def) >> return; >> >> - switch (doi_def->type) { >> - case CIPSO_V4_MAP_TRANS: >> - kfree(doi_def->map.std->lvl.cipso); >> - kfree(doi_def->map.std->lvl.local); >> - kfree(doi_def->map.std->cat.cipso); >> - kfree(doi_def->map.std->cat.local); >> - kfree(doi_def->map.std); >> - break; >> + if (doi_def->map.std) { >> + switch (doi_def->type) { >> + case CIPSO_V4_MAP_TRANS: >> + kfree(doi_def->map.std->lvl.cipso); >> + kfree(doi_def->map.std->lvl.local); >> + kfree(doi_def->map.std->cat.cipso); >> + kfree(doi_def->map.std->cat.local); >> + kfree(doi_def->map.std); >> + break; >> + } >> } >> kfree(doi_def); >> } >> -- >> 1.8.3.1 >> > >
diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c index 099259f..7fbd0b5 100644 --- a/net/ipv4/cipso_ipv4.c +++ b/net/ipv4/cipso_ipv4.c @@ -465,14 +465,16 @@ void cipso_v4_doi_free(struct cipso_v4_doi *doi_def) if (!doi_def) return; - switch (doi_def->type) { - case CIPSO_V4_MAP_TRANS: - kfree(doi_def->map.std->lvl.cipso); - kfree(doi_def->map.std->lvl.local); - kfree(doi_def->map.std->cat.cipso); - kfree(doi_def->map.std->cat.local); - kfree(doi_def->map.std); - break; + if (doi_def->map.std) { + switch (doi_def->type) { + case CIPSO_V4_MAP_TRANS: + kfree(doi_def->map.std->lvl.cipso); + kfree(doi_def->map.std->lvl.local); + kfree(doi_def->map.std->cat.cipso); + kfree(doi_def->map.std->cat.local); + kfree(doi_def->map.std); + break; + } } kfree(doi_def); }
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 <abaci@linux.alibaba.com> Signed-off-by: Michael Wang <yun.wang@linux.alibaba.com> --- net/ipv4/cipso_ipv4.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)