| Message ID | 20240805140840.1606239-9-hadess@hadess.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix a number of static analysis issues #6 | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/CheckPatch | success | CheckPatch PASS |
| tedd_an/GitLint | fail | WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search 4: B1 Line exceeds max length (205>80): "bluez-5.77/monitor/control.c:1094:2: tainted_data_return: Called function "recv(data->fd, data->buf + data->offset, 1490UL - data->offset, MSG_DONTWAIT)", and a possible return value may be less than zero." 5: B1 Line exceeds max length (144>80): "bluez-5.77/monitor/control.c:1094:2: assign: Assigning: "len" = "recv(data->fd, data->buf + data->offset, 1490UL - data->offset, MSG_DONTWAIT)"." 6: B1 Line exceeds max length (119>80): "bluez-5.77/monitor/control.c:1099:2: overflow: The expression "data->offset" is considered to have possibly overflowed." 7: B1 Line exceeds max length (165>80): "bluez-5.77/monitor/control.c:1115:3: overflow: The expression "data->offset -= pktlen + 6" is deemed overflowed because at least one of its arguments has overflowed." 8: B1 Line exceeds max length (265>80): "bluez-5.77/monitor/control.c:1118:4: overflow_sink: "data->offset", which might have underflowed, is passed to "memmove(data->buf, data->buf + 6 + pktlen, data->offset)". [Note: The source code implementation of the function has been overridden by a builtin model.]" 10: B3 Line contains hard tab characters (\t): "1117| if (data->offset > 0)" 11: B3 Line contains hard tab characters (\t): "1118|-> memmove(data->buf, data->buf + MGMT_HDR_SIZE + pktlen," 12: B3 Line contains hard tab characters (\t): "1119| data->offset);" 13: B3 Line contains hard tab characters (\t): "1120| }" |
| tedd_an/IncrementalBuild | success | Incremental Build PASS |
Hi Bastien, On Mon, Aug 5, 2024 at 10:09 AM Bastien Nocera <hadess@hadess.net> wrote: > > Error: INTEGER_OVERFLOW (CWE-190): [#def4] [important] > bluez-5.77/monitor/control.c:1094:2: tainted_data_return: Called function "recv(data->fd, data->buf + data->offset, 1490UL - data->offset, MSG_DONTWAIT)", and a possible return value may be less than zero. > bluez-5.77/monitor/control.c:1094:2: assign: Assigning: "len" = "recv(data->fd, data->buf + data->offset, 1490UL - data->offset, MSG_DONTWAIT)". > bluez-5.77/monitor/control.c:1099:2: overflow: The expression "data->offset" is considered to have possibly overflowed. > bluez-5.77/monitor/control.c:1115:3: overflow: The expression "data->offset -= pktlen + 6" is deemed overflowed because at least one of its arguments has overflowed. > bluez-5.77/monitor/control.c:1118:4: overflow_sink: "data->offset", which might have underflowed, is passed to "memmove(data->buf, data->buf + 6 + pktlen, data->offset)". [Note: The source code implementation of the function has been overridden by a builtin model.] > 1116| > 1117| if (data->offset > 0) > 1118|-> memmove(data->buf, data->buf + MGMT_HDR_SIZE + pktlen, > 1119| data->offset); > 1120| } > --- > monitor/control.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/monitor/control.c b/monitor/control.c > index 009cf15209f0..62857b4b84de 100644 > --- a/monitor/control.c > +++ b/monitor/control.c > @@ -18,6 +18,7 @@ > #include <stdbool.h> > #include <stddef.h> > #include <errno.h> > +#include <limits.h> > #include <unistd.h> > #include <stdlib.h> > #include <string.h> > @@ -1091,9 +1092,14 @@ static void client_callback(int fd, uint32_t events, void *user_data) > return; > } > > + if (sizeof(data->buf) <= data->offset) > + return; > + > len = recv(data->fd, data->buf + data->offset, > sizeof(data->buf) - data->offset, MSG_DONTWAIT); > - if (len < 0) > + if (len < 0 || > + len > UINT16_MAX || > + UINT16_MAX - data->offset > len) > return; Not really sure why we would be using UINT16_MAX here instead of BTSNOOP_MAX_PACKET_SIZE or sizeof(data->buf), and perhaps we should really check that len > sizeof(data->buf) - data->offset since that implicitly guarantees data->offset + len <= sizeof(data->buf), anyway this is again suggesting that a syscall can return a bigger value than it was asked for. > data->offset += len; > -- > 2.45.2 > >
diff --git a/monitor/control.c b/monitor/control.c index 009cf15209f0..62857b4b84de 100644 --- a/monitor/control.c +++ b/monitor/control.c @@ -18,6 +18,7 @@ #include <stdbool.h> #include <stddef.h> #include <errno.h> +#include <limits.h> #include <unistd.h> #include <stdlib.h> #include <string.h> @@ -1091,9 +1092,14 @@ static void client_callback(int fd, uint32_t events, void *user_data) return; } + if (sizeof(data->buf) <= data->offset) + return; + len = recv(data->fd, data->buf + data->offset, sizeof(data->buf) - data->offset, MSG_DONTWAIT); - if (len < 0) + if (len < 0 || + len > UINT16_MAX || + UINT16_MAX - data->offset > len) return; data->offset += len;