: cls_fw: fix NULL pointer dereference
Subsystems (3)
Fix a regression from my nfmark mask patch for cls_fw.
Thomas, Jamal, do you have an idea what this "old method" stuff is used for? It seems it is only used during the below mentioned race.
[NET_SCHED]: cls_fw: fix NULL pointer dereference
When the first fw classifier is initialized, there is a small window between the ->init() and ->change() calls, during which the classifier is active but not entirely set up and tp->root is still NULL (->init() does nothing).
When a packet is queued during this window a NULL pointer dereference occurs in fw_classify() when trying to dereference head->mask;
---
commit 07aac6f7b7e43bc1bb960b2f41a02e81d4e25ead
tree 523108861c92ec7e513fbc8561a57b5e1c56c1eb
parent d916faace3efc0bf19fe9a615a1ab8fa1a24cd93
author Patrick McHardy <[email protected]> Mon, 04 Dec 2006 16:29:07 +0100 committer Patrick McHardy <[email protected]> Mon, 04 Dec 2006 16:29:07 +0100
net/sched/cls_fw.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
net/sched/cls_fw.c +2 -1
@@ -101,9 +101,10 @@ static int fw_classify(struct sk_buff *sstruct fw_head *head = (struct fw_head *)tp->root;struct fw_filter *f;int r; -u32 id = skb->mark & head->mask; +u32 id = skb->mark;if (head !=NULL ) { + id &= head->mask;for (f=head->ht[fw_hash (id)]; f; f=f->next) {if (f->id == id) { *res = f->res;
Quoted 5 lines · Patrick McHardy
Fix a regression from my nfmark mask patch for
cls_fw.Thomas, Jamal, do you have an idea what this "old method" stuff is used for? It seems it is only used during the below mentioned race.
AFAIK, that has been there forever. Alexey may know. I am not sure if removing it will break any scripts etc.
cheers,
jamal
* jamal <[email protected]> 2006-12-04 11:25
Quoted 9 lines · Patrick McHardy
Quoted 5 lines
Fix a regression from my nfmark mask patch for
cls_fw.Thomas, Jamal, do you have an idea what this "old method" stuff is used for? It seems it is only used during the below mentioned race.
AFAIK, that has been there forever. Alexey may know. I am not sure if removing it will break any scripts etc.
You mean the scripts get upset when the kernel oopses?
Very good spotting Patrick!
Quoted 10 lines · Thomas Graf
* jamal <[email protected]> 2006-12-04 11:25
Quoted 8 lines · Patrick McHardy
Quoted 6 lines
Fix a regression from my nfmark mask patch for
cls_fw.Thomas, Jamal, do you have an idea what this "old method" stuff is used for? It seems it is only used during the below mentioned race.
Quoted 5 lines
Quoted 2 lines
AFAIK, that has been there forever. Alexey may know. I am not sure if removing it will break any scripts etc.
You mean the scripts get upset when the kernel oopses?
Well, it won't oops without my broken patch :)
It just seems this code is entirely useless and the only thing it does is cause short term unexpected behaviour during the race I mentioned.
One thing we should probably do is to move the tp->root allocation to the init function in cls_fw and the others implementing it as dummy to at least close the race between ->init and ->change. I'll look into that as a follow-up patch.
* Patrick McHardy <[email protected]> 2006-12-04 17:39
Quoted 3 lines
It just seems this code is entirely useless and the only thing it does is cause short term unexpected behaviour during the race I mentioned.
Yes, the whole head == NULL branch should be removed.
Quoted 5 lines
One thing we should probably do is to move the
tp->rootallocation to the init function incls_fwand the others implementing it as dummy to at least close the race between ->init and ->change. I'll look into that as a follow-up patch.
Right, allocating the head in init with a mask of 0xFFFFFFFF and then allow the user to overwrite it seems to make most sense.
From: Patrick McHardy <[email protected]>
Date: Mon, 04 Dec 2006 16:34:46 +0100
Quoted 11 lines
[
NET_SCHED]:cls_fw: fix NULL pointer dereferenceWhen the first fw classifier is initialized, there is a small window between the ->init() and ->change() calls, during which the classifier is active but not entirely set up and
tp->rootis still NULL (->init() does nothing).When a packet is queued during this window a NULL pointer dereference occurs in
fw_classify()when trying to dereferencehead->mask;Signed-off-by Patrick McHardy <[email protected]>
I've applied this, thanks Patrick.
Quoted 6 lines · Patrick McHardy
Fix a regression from my nfmark mask patch for
cls_fw.Thomas, Jamal, do you have an idea what this "old method" stuff is used for? It seems it is only used during the below mentioned race.
Sorry for eavesdropping, but have a look at htb_classify starting comment. It is also used by unofficial but quite popular IPMARK target.
Jarek P.
Quoted 10 lines · Jarek Poplawski
Quoted 3 lines · Patrick McHardy
Thomas, Jamal, do you have an idea what this "old method" stuff is used for? It seems it is only used during the below mentioned race.
Sorry for eavesdropping, but have a look at
htb_classifystarting comment. It is also used by unofficial but quite popular IPMARK target.
Yes I know, I just didn't see how it could be configured to really use that code. But while trying to explain the flow that would always lead to tp->root != NULL in this mail, I noticed I missed something :)
At the top of fw_change:
if (!opt)
return handle ? -EINVAL : 0;which happens when adding a fw classifier without specifying any arguments. My previous fix is still enough, but we can't remove this of course.