| Message ID | 20210105055754.16486-1-miaoqinglang@huawei.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] net: qrtr: fix null-ptr-deref in qrtr_ns_remove | expand |
From: Qinglang Miao <miaoqinglang@huawei.com> Date: Tue, 5 Jan 2021 13:57:54 +0800 > A null-ptr-deref bug is reported by Hulk Robot like this: > -------------- > KASAN: null-ptr-deref in range [0x0000000000000128-0x000000000000012f] > Call Trace: > qrtr_ns_remove+0x22/0x40 [ns] > qrtr_proto_fini+0xa/0x31 [qrtr] > __x64_sys_delete_module+0x337/0x4e0 > do_syscall_64+0x34/0x80 > entry_SYSCALL_64_after_hwframe+0x44/0xa9 > RIP: 0033:0x468ded > -------------- > > When qrtr_ns_init fails in qrtr_proto_init, qrtr_ns_remove which would > be called later on would raise a null-ptr-deref because qrtr_ns.workqueue > has been destroyed. > > Fix it by making qrtr_ns_init have a return value and adding a check in > qrtr_proto_init. > > Reported-by: Hulk Robot <hulkci@huawei.com> > Signed-off-by: Qinglang Miao <miaoqinglang@huawei.com> > --- > v1->v2: remove redundant braces for single statement blocks. Applied.
Hi Markus, I'd like to take some of your advice in this patch, but I noticed that this one has been applied. Some of your advice would be considered kindly on my future work. Thanks. 在 2021/1/5 21:14, Markus Elfring 写道: >> A null-ptr-deref bug is reported by Hulk Robot like this: > > Can it be clearer to use the term “null pointer dereference” for the final commit message? This advice is too detailed for 'null-ptr-deref' is known as a general phrase like 'use-after-free' for kernel developer, I think.> > >> -------------- > > I suggest to choose an other character for drawing such a text line. It's an acceptable advice, thanks. > > >> Fix it by making … > > Would you like to replace this wording by the tag “Fixes”? Sorry, I didn't get your words. 'Fix it by' follows the solution 'Fixes' follows the commit which brought the problem. In fact, I do considered using 'Fixes' on this one, but it's hard to tell which specific commit brought this null pointer dereference. > > Will an other imperative wording variant be helpful for this change description? > > > … >> +++ b/net/qrtr/qrtr.c >> @@ -1287,13 +1287,19 @@ static int __init qrtr_proto_init(void) > … >> +err_sock: >> + sock_unregister(qrtr_family.family); >> +err_proto: >> + proto_unregister(&qrtr_proto); >> return rc; >> } > > Would it be clearer to use the labels “unregister_sock” and “unregister_proto”? In fact, The reason I use 'err_sock' rather than 'unregister_sock' is to keep same in 'net/qrtr/ns.c'. I agree with you that “unregister_sock” is better in normal case. > > Regards, > Markus > . >
在 2021/1/6 16:09, Markus Elfring 写道: >>>> A null-ptr-deref bug is reported by Hulk Robot like this: >>> >>> Can it be clearer to use the term “null pointer dereference” for the final commit message? >> This advice is too detailed for 'null-ptr-deref' is known as a general phrase > > This key word was provided already by the referenced KASAN report. > Yep, you're right. 'null-ptr-deref' is not really proper here. > >> like 'use-after-free' for kernel developer, I think. > I suggest to reconsider the use of abbreviations at some places. > > >>>> Fix it by making … >>> >>> Would you like to replace this wording by the tag “Fixes”? >> Sorry, I didn't get your words. >> >> 'Fix it by' follows the solution > > I propose to specify the desired adjustments without such a prefix > in the change description. Sorry, I can understand what you means, but I still disagree with this one, for: 1. 'Fix it by' is everywhere in kernel commit message. 2. I think adding it or not makes no change for understanding. 3. I'm not sure this is an official proposal. > > >> In fact, I do considered using 'Fixes' on this one, > > Thanks for such information. > > >> but it's hard to tell which specific commit brought this null pointer dereference. > > This aspect is unfortunate here. > > Regards, > Markus > . > Thanks anyway, I shall pay more attention to commit message. ;D
On Wed, Jan 06, 2021 at 05:46:22PM +0800, Qinglang Miao wrote: > > > 在 2021/1/6 16:09, Markus Elfring 写道: > > > > > A null-ptr-deref bug is reported by Hulk Robot like this: > > > > > > > > Can it be clearer to use the term “null pointer dereference” for the final commit message? > > > This advice is too detailed for 'null-ptr-deref' is known as a general phrase > > > > This key word was provided already by the referenced KASAN report. > > > Yep, you're right. 'null-ptr-deref' is not really proper here. > > > > > like 'use-after-free' for kernel developer, I think. > > I suggest to reconsider the use of abbreviations at some places. > > > > > > > > Fix it by making … > > > > > > > > Would you like to replace this wording by the tag “Fixes”? > > > Sorry, I didn't get your words. > > > > > > 'Fix it by' follows the solution > > > > I propose to specify the desired adjustments without such a prefix > > in the change description. > Sorry, I can understand what you means, but I still disagree with this one, > for: > > 1. 'Fix it by' is everywhere in kernel commit message. > 2. I think adding it or not makes no change for understanding. > 3. I'm not sure this is an official proposal. > Feel free to ignore Markus... :/ We have asked him over and over to stop sending these sort of advice but he refused and eventually he was banned from the mailing lists. The rest of us can't see his messages to you unless we're included personally in the CC list. regards, dan carpenter
diff --git a/net/qrtr/ns.c b/net/qrtr/ns.c index 56aaf8cb6..8d00dfe81 100644 --- a/net/qrtr/ns.c +++ b/net/qrtr/ns.c @@ -755,7 +755,7 @@ static void qrtr_ns_data_ready(struct sock *sk) queue_work(qrtr_ns.workqueue, &qrtr_ns.work); } -void qrtr_ns_init(void) +int qrtr_ns_init(void) { struct sockaddr_qrtr sq; int ret; @@ -766,7 +766,7 @@ void qrtr_ns_init(void) ret = sock_create_kern(&init_net, AF_QIPCRTR, SOCK_DGRAM, PF_QIPCRTR, &qrtr_ns.sock); if (ret < 0) - return; + return ret; ret = kernel_getsockname(qrtr_ns.sock, (struct sockaddr *)&sq); if (ret < 0) { @@ -797,12 +797,13 @@ void qrtr_ns_init(void) if (ret < 0) goto err_wq; - return; + return 0; err_wq: destroy_workqueue(qrtr_ns.workqueue); err_sock: sock_release(qrtr_ns.sock); + return ret; } EXPORT_SYMBOL_GPL(qrtr_ns_init); diff --git a/net/qrtr/qrtr.c b/net/qrtr/qrtr.c index f4ab3ca6d..b34358282 100644 --- a/net/qrtr/qrtr.c +++ b/net/qrtr/qrtr.c @@ -1287,13 +1287,19 @@ static int __init qrtr_proto_init(void) return rc; rc = sock_register(&qrtr_family); - if (rc) { - proto_unregister(&qrtr_proto); - return rc; - } + if (rc) + goto err_proto; - qrtr_ns_init(); + rc = qrtr_ns_init(); + if (rc) + goto err_sock; + return 0; + +err_sock: + sock_unregister(qrtr_family.family); +err_proto: + proto_unregister(&qrtr_proto); return rc; } postcore_initcall(qrtr_proto_init); diff --git a/net/qrtr/qrtr.h b/net/qrtr/qrtr.h index dc2b67f17..3f2d28696 100644 --- a/net/qrtr/qrtr.h +++ b/net/qrtr/qrtr.h @@ -29,7 +29,7 @@ void qrtr_endpoint_unregister(struct qrtr_endpoint *ep); int qrtr_endpoint_post(struct qrtr_endpoint *ep, const void *data, size_t len); -void qrtr_ns_init(void); +int qrtr_ns_init(void); void qrtr_ns_remove(void);
A null-ptr-deref bug is reported by Hulk Robot like this: -------------- KASAN: null-ptr-deref in range [0x0000000000000128-0x000000000000012f] Call Trace: qrtr_ns_remove+0x22/0x40 [ns] qrtr_proto_fini+0xa/0x31 [qrtr] __x64_sys_delete_module+0x337/0x4e0 do_syscall_64+0x34/0x80 entry_SYSCALL_64_after_hwframe+0x44/0xa9 RIP: 0033:0x468ded -------------- When qrtr_ns_init fails in qrtr_proto_init, qrtr_ns_remove which would be called later on would raise a null-ptr-deref because qrtr_ns.workqueue has been destroyed. Fix it by making qrtr_ns_init have a return value and adding a check in qrtr_proto_init. Reported-by: Hulk Robot <hulkci@huawei.com> Signed-off-by: Qinglang Miao <miaoqinglang@huawei.com> --- v1->v2: remove redundant braces for single statement blocks. net/qrtr/ns.c | 7 ++++--- net/qrtr/qrtr.c | 16 +++++++++++----- net/qrtr/qrtr.h | 2 +- 3 files changed, 16 insertions(+), 9 deletions(-)