diff mbox series

monitor: fix buffer overflow when terminal width > 255

Message ID 20240914-fix-log-buffer-overflow-v1-1-733cb4fff673@gmail.com (mailing list archive)
State Superseded
Headers show
Series monitor: fix buffer overflow when terminal width > 255 | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
tedd_an/CheckPatch success CheckPatch PASS
tedd_an/GitLint success Gitlint PASS
tedd_an/BuildEll success Build ELL PASS
tedd_an/BluezMake success Bluez Make PASS
tedd_an/MakeCheck success Bluez Make Check PASS
tedd_an/MakeDistcheck success Make Distcheck PASS
tedd_an/CheckValgrind success Check Valgrind PASS
tedd_an/CheckSmatch warning CheckSparse WARNING monitor/packet.c: note: in included file:monitor/display.h:82:26: warning: Variable length array is used.monitor/packet.c:1868:26: warning: Variable length array is used.monitor/packet.c: note: in included file:monitor/bt.h:3606:52: warning: array of flexible structuresmonitor/bt.h:3594:40: warning: array of flexible structures
tedd_an/bluezmakeextell success Make External ELL PASS
tedd_an/IncrementalBuild success Incremental Build PASS
tedd_an/ScanBuild warning ScanBuild: monitor/packet.c:529:1: warning: Potential leak of memory pointed to by 'line' } ^ 1 warning generated.

Commit Message

Celeste Liu Sept. 14, 2024, 2:09 p.m. UTC
In current code, we create line buffer with size 256, which can contains
255 ASCII characters. But in modern system, terminal can have larger
width. It may cause buffer overflow in snprintf() text.

We need allocate line buffer with size which can contains one line in
terminal. The size should be difficult to calculate because of multibyte
characters, but our code using line buffer assumed all characters has
1 byte size (e.g. when we put packet text into line buffer via
snprintf(), we calculate max size by 1B * col.), so it's safe to
allocate line buffer with col + 1.

Signed-off-by: Celeste Liu <CoelacanthusHex@gmail.com>
---
 monitor/packet.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)


---
base-commit: 41f943630d9a03c40e95057b2ac3d96470b9c71e
change-id: 20240914-fix-log-buffer-overflow-9aa5e61ee5b8

Best regards,

Comments

bluez.test.bot@gmail.com Sept. 14, 2024, 4:02 p.m. UTC | #1
This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=890395

---Test result---

Test Summary:
CheckPatch                    PASS      0.49 seconds
GitLint                       PASS      0.35 seconds
BuildEll                      PASS      24.73 seconds
BluezMake                     PASS      1696.44 seconds
MakeCheck                     PASS      13.31 seconds
MakeDistcheck                 PASS      179.41 seconds
CheckValgrind                 PASS      256.32 seconds
CheckSmatch                   WARNING   358.63 seconds
bluezmakeextell               PASS      120.73 seconds
IncrementalBuild              PASS      1407.21 seconds
ScanBuild                     WARNING   1016.29 seconds

Details
##############################
Test: CheckSmatch - WARNING
Desc: Run smatch tool with source
Output:
monitor/packet.c: note: in included file:monitor/display.h:82:26: warning: Variable length array is used.monitor/packet.c:1868:26: warning: Variable length array is used.monitor/packet.c: note: in included file:monitor/bt.h:3606:52: warning: array of flexible structuresmonitor/bt.h:3594:40: warning: array of flexible structures
##############################
Test: ScanBuild - WARNING
Desc: Run Scan Build
Output:
monitor/packet.c:529:1: warning: Potential leak of memory pointed to by 'line'
}
^
1 warning generated.



---
Regards,
Linux Bluetooth
Celeste Liu Sept. 14, 2024, 4:12 p.m. UTC | #2
On 2024-09-15 00:02, bluez.test.bot@gmail.com wrote:
> This is automated email and please do not reply to this email!
> 
> Dear submitter,
> 
> Thank you for submitting the patches to the linux bluetooth mailing list.
> This is a CI test results with your patch series:
> PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=890395
> 
> ---Test result---
> 
> Test Summary:
> CheckPatch                    PASS      0.49 seconds
> GitLint                       PASS      0.35 seconds
> BuildEll                      PASS      24.73 seconds
> BluezMake                     PASS      1696.44 seconds
> MakeCheck                     PASS      13.31 seconds
> MakeDistcheck                 PASS      179.41 seconds
> CheckValgrind                 PASS      256.32 seconds
> CheckSmatch                   WARNING   358.63 seconds
> bluezmakeextell               PASS      120.73 seconds
> IncrementalBuild              PASS      1407.21 seconds
> ScanBuild                     WARNING   1016.29 seconds
> 
> Details
> ##############################
> Test: CheckSmatch - WARNING
> Desc: Run smatch tool with source
> Output:
> monitor/packet.c: note: in included file:monitor/display.h:82:26: warning: Variable length array is used.monitor/packet.c:1868:26: warning: Variable length array is used.monitor/packet.c: note: in included file:monitor/bt.h:3606:52: warning: array of flexible structuresmonitor/bt.h:3594:40: warning: array of flexible structures

It's the code already there before I touch.

> ##############################
> Test: ScanBuild - WARNING
> Desc: Run Scan Build
> Output:
> monitor/packet.c:529:1: warning: Potential leak of memory pointed to by 'line'

v2 has been sent. Add forgot free() and send prefix "bluez".

> }
> ^
> 1 warning generated.
> 
> 
> 
> ---
> Regards,
> Linux Bluetooth
>
diff mbox series

Patch

diff --git a/monitor/packet.c b/monitor/packet.c
index c2599fe6864ab44d657c121fcc3ceecc1ebc52a6..3a21909116b341f782bcaf47c0cb3b880cb3a288 100644
--- a/monitor/packet.c
+++ b/monitor/packet.c
@@ -376,7 +376,8 @@  static void print_packet(struct timeval *tv, struct ucred *cred, char ident,
 					const char *text, const char *extra)
 {
 	int col = num_columns();
-	char line[256], ts_str[96], pid_str[140];
+	char ts_str[96], pid_str[140];
+	char *line = (char *) malloc(sizeof(char) * col + 1);
 	int n, ts_len = 0, ts_pos = 0, len = 0, pos = 0;
 	static size_t last_frame;