| Message ID | 20210810132007.296008-1-islituo@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: 9p: Fix possible null-pointer dereference in p9_cm_event_handler() | 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: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 9 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
On Tue, Aug 10, 2021 at 06:20:07AM -0700, Tuo Li wrote: > The variable rdma is checked when event->event is equal to > RDMA_CM_EVENT_DISCONNECTED: > if (rdma) > > This indicates that it can be NULL. If so, a null-pointer dereference will > occur when calling complete(): > complete(&rdma->cm_done); > > To fix this possible null-pointer dereference, calling complete() only > when rdma is not NULL. You need to explain how is it possible and blindly set if () checks. I would say first "if (rdma)" is not needed, but don't know for sure. > > Reported-by: TOTE Robot <oslab@tsinghua.edu.cn> > Signed-off-by: Tuo Li <islituo@gmail.com> > --- > net/9p/trans_rdma.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/net/9p/trans_rdma.c b/net/9p/trans_rdma.c > index af0a8a6cd3fd..fb3435dfd071 100644 > --- a/net/9p/trans_rdma.c > +++ b/net/9p/trans_rdma.c > @@ -285,7 +285,8 @@ p9_cm_event_handler(struct rdma_cm_id *id, struct rdma_cm_event *event) > default: > BUG(); > } > - complete(&rdma->cm_done); > + if (rdma) > + complete(&rdma->cm_done); > return 0; > } > > -- > 2.25.1 >
Leon Romanovsky wrote on Tue, Aug 10, 2021 at 04:55:42PM +0300: > On Tue, Aug 10, 2021 at 06:20:07AM -0700, Tuo Li wrote: > > The variable rdma is checked when event->event is equal to > > RDMA_CM_EVENT_DISCONNECTED: > > if (rdma) > > > > This indicates that it can be NULL. If so, a null-pointer dereference will > > occur when calling complete(): > > complete(&rdma->cm_done); > > > > To fix this possible null-pointer dereference, calling complete() only > > when rdma is not NULL. > > You need to explain how is it possible and blindly set if () checks. > I would say first "if (rdma)" is not needed, but don't know for sure. Sounds like static analysis because there's a if (rdma) check in RDMA_CM_EVENT_DISCONNECTED above, so if that needed check then it will bug right afterwards I'd tend to agree I don't think it's possible client->trans is null there (it's filled right after rdma_create_id which defines the handler, there might be a window where the callback is called before? But as I understand it shouldn't be called until we resolve address and connect then later disconnect) So, I agree with Leon - unless you have a backtrace of a real bug let's remove the other 'if' if you want to cleanup something for your robot.
diff --git a/net/9p/trans_rdma.c b/net/9p/trans_rdma.c index af0a8a6cd3fd..fb3435dfd071 100644 --- a/net/9p/trans_rdma.c +++ b/net/9p/trans_rdma.c @@ -285,7 +285,8 @@ p9_cm_event_handler(struct rdma_cm_id *id, struct rdma_cm_event *event) default: BUG(); } - complete(&rdma->cm_done); + if (rdma) + complete(&rdma->cm_done); return 0; }
The variable rdma is checked when event->event is equal to RDMA_CM_EVENT_DISCONNECTED: if (rdma) This indicates that it can be NULL. If so, a null-pointer dereference will occur when calling complete(): complete(&rdma->cm_done); To fix this possible null-pointer dereference, calling complete() only when rdma is not NULL. Reported-by: TOTE Robot <oslab@tsinghua.edu.cn> Signed-off-by: Tuo Li <islituo@gmail.com> --- net/9p/trans_rdma.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)