Add kernelCTF CVE-2024-45016_lts_cos_mitigation#261
Add kernelCTF CVE-2024-45016_lts_cos_mitigation#261hexfoureight wants to merge 12 commits intogoogle:masterfrom
Conversation
JordyZomer
added
the
kCTF: vuln OK
koczkatamas
left a comment
koczkatamas
left a comment
There was a problem hiding this comment.
Hey! I've left some questions, change requests and also run our AI tool on the source code. Please take a look and make changes if necessary. Thanks!
| } | ||
|
|
||
| void tc_add_fl (int clid) { | ||
| basic_filter_msg.tm.tcm_parent = clid & 0xffff0000; |
There was a problem hiding this comment.
Usage of magic number 0xffff0000.
Recommendation: Define a constant such as #define TC_H_MAJ_MASK 0xffff0000.
AI-suggested fix (do not apply blindly, but can be helpful for inspiration):
basic_filter_msg.tm.tcm_parent = clid & TC_H_MAJ_MASK;Read more about this violation in the 'Name and/or comment numeric constants' section of the style guide.
This comment is AI-generated. Although it was manually checked, it can still contain mistakes, please double-check it and feel free to push back if you think it's wrong.
| if ((nread = read(nl_sock_fd, msgbuf + tread, sizeof(msgbuf))) == -1) | ||
| err_exit("[-] read"); | ||
| tread += nread; | ||
| } while (nread != 20); |
There was a problem hiding this comment.
Usage of magic number 20. Explain why do we stop at this read?
Recommendation: Use sizeof(struct nlmsghdr) or a specific define.
AI-suggested fix (do not apply blindly, but can be helpful for inspiration):
} while (nread != NLMSG_HDRLEN);Read more about this violation in the 'Name and/or comment numeric constants' section of the style guide.
This comment is AI-generated. Although it was manually checked, it can still contain mistakes, please double-check it and feel free to push back if you think it's wrong.
| } | ||
|
|
||
| if (off != -1) { | ||
| memcpy(buf, msgbuf + off + 56, 8); // latency |
There was a problem hiding this comment.
Could you describe what structure's fields you are describing here? Are they dynamically generated fields and they don't have a kernel representation?
Asking because we'd like to understand how to modify these offsets when we'd like to target a different kernel version.
Read more about this violation in the 'Sprayed and leaked structures' section of the style guide.
| long stab_parms_buf[7], stab_slot_buf[5]; | ||
|
|
||
| void setup_stab_read (int handle, long parms_kaddr, long slot_kaddr) { | ||
| int flags = 0x80; |
There was a problem hiding this comment.
Usage of magic number 0x80. What does this field mean? Is there a kernel or user-space define for this which can be used instead of this maybe?
Recommendation: Define a constant #define STAB_FLAG 0x80.
AI-suggested fix (do not apply blindly, but can be helpful for inspiration):
int flags = STAB_FLAG;Read more about this violation in the 'Name and/or comment numeric constants' section of the style guide.
|
|
||
| void setup_stab_read (int handle, long parms_kaddr, long slot_kaddr) { | ||
| int flags = 0x80; | ||
| memcpy((char *)stab_parms_buf + 9, &flags, 4); |
There was a problem hiding this comment.
Usage of magic numbers 9 and 4.
Recommendation: Use defined constants for offsets and sizes.
AI-suggested fix (do not apply blindly, but can be helpful for inspiration):
memcpy((char *)stab_parms_buf + STAB_PARMS_FLAGS_OFF, &flags, sizeof(flags));Read more about this violation in the 'Name and/or comment numeric constants' section of the style guide.
This comment is AI-generated. Although it was manually checked, it can still contain mistakes, please double-check it and feel free to push back if you think it's wrong.
| int parent, n1, n2, netem_spray1[NETEM_SPRAY], netem_spray2[NETEM_SPRAY]; | ||
|
|
||
| hfsc_qdisc_msg.def = 1; | ||
| tc_add_qd(&hfsc_qdisc_msg, -1, 0x150000); |
There was a problem hiding this comment.
Hardcoded magic handles for QDiscs.
Recommendation: Define constants for the handles.
AI-suggested fix (do not apply blindly, but can be helpful for inspiration):
#define ROOT_HFSC_HANDLE 0x150000
tc_add_qd(&hfsc_qdisc_msg, -1, ROOT_HFSC_HANDLE);Read more about this violation in the 'Name and/or comment numeric constants' section of the style guide.
This comment is AI-generated. Although it was manually checked, it can still contain mistakes, please double-check it and feel free to push back if you think it's wrong.
| tc_add_qd(&stall_tbf_qdisc_msg, tbfp, 0); | ||
|
|
||
| /* Choose netem handles */ | ||
| for (int i = 0x10000000, j = 0; j < NETEM_SPRAY; i += 0x10000) { |
There was a problem hiding this comment.
Hardcoded spray start handle 0x10000000.
Recommendation: Define a constant for the spray base handle.
AI-suggested fix (do not apply blindly, but can be helpful for inspiration):
#define SPRAY_HANDLE_BASE 0x10000000Read more about this violation in the 'Name and/or comment numeric constants' section of the style guide.
This comment is AI-generated. Although it was manually checked, it can still contain mistakes, please double-check it and feel free to push back if you think it's wrong.
|
|
||
| ``` | ||
| y,p: &n2->latency | ||
| s: &n1->latency + 32 |
There was a problem hiding this comment.
What's at +32 or why do we need this offset?
| Due to the limitation of the rebalancing write mentioned above, the fake qdisc linked to by `n2->hash.next` will be at a misaligned offset, limiting which of its fields we can set. Therefore this qdisc will only be used to link to the fake qdisc that is actually used for the read. Here are the `y`,`p`,`s`,`c` addresses for the rebalancing write: | ||
|
|
||
| ``` | ||
| y: &n1->latency + 32 |
There was a problem hiding this comment.
What are at offsets +32, +32 and +8 or why do we need this alignment?
| The rebalancing write primitive is used to replace the root FSC `hfsc_class` of the interface's root qdisc: | ||
|
|
||
| ``` | ||
| y: &n1->slot + 8 |
There was a problem hiding this comment.
What are at offsets +8, -16 and +32 or why do we need this alignment?
3 participants
No description provided.