| Message ID | 20240805140840.1606239-7-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 13: B1 Line exceeds max length (152>80): "bluez-5.77/src/shared/gatt-db.c:612:2: assignment: Assigning: "len" = "uuid_to_le(uuid, value)". The value of "len" is now between 0 and 31 (inclusive)." 14: B1 Line exceeds max length (206>80): "bluez-5.77/src/shared/gatt-db.c:614:2: overrun-buffer-arg: Overrunning array "value" of 16 bytes by passing it to a function which accesses it at byte offset 30 using argument "len" (which evaluates to 31)." 15: B3 Line contains hard tab characters (\t): "612| len = uuid_to_le(uuid, value);" 17: B3 Line contains hard tab characters (\t): "614|-> service->attributes[0] = new_attribute(service, handle, type, value," 18: B3 Line contains hard tab characters (\t): "615| len);" 19: B3 Line contains hard tab characters (\t): "616| if (!service->attributes[0]) {" |
| tedd_an/IncrementalBuild | success | Incremental Build PASS |
Hi Bastien, On Mon, Aug 5, 2024 at 10:09 AM Bastien Nocera <hadess@hadess.net> wrote: > > uuid_to_le() returns one of the possible values from bt_uuid_len(). > bt_uuid_len() returns "type / 8". > type is a value between 0 and 128, but could be something else > depending on the validity of the UUID that's parsed. So an invalid > value of type between 128 and 256 would trigger an overrun. > > Add a check to make sure that an invalid type isn't used to calculate > the length. > > Error: OVERRUN (CWE-119): [#def6] [important] > bluez-5.77/src/shared/gatt-db.c:612:2: assignment: Assigning: "len" = "uuid_to_le(uuid, value)". The value of "len" is now between 0 and 31 (inclusive). > bluez-5.77/src/shared/gatt-db.c:614:2: overrun-buffer-arg: Overrunning array "value" of 16 bytes by passing it to a function which accesses it at byte offset 30 using argument "len" (which evaluates to 31). > 612| len = uuid_to_le(uuid, value); > 613| > 614|-> service->attributes[0] = new_attribute(service, handle, type, value, > 615| len); > 616| if (!service->attributes[0]) { > --- > src/shared/gatt-db.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c > index b35763410d17..cd0eba6bf1d0 100644 > --- a/src/shared/gatt-db.c > +++ b/src/shared/gatt-db.c > @@ -560,9 +560,14 @@ static int uuid_to_le(const bt_uuid_t *uuid, uint8_t *dst) > return bt_uuid_len(uuid); > } > > - bt_uuid_to_uuid128(uuid, &uuid128); > - bswap_128(&uuid128.value.u128, dst); > - return bt_uuid_len(&uuid128); > + if (uuid->type == BT_UUID32 || BT_UUID32 is not really valid in LE, so Id leave this check to be: switch (uuid->type) { case BT_UUID16: put_le16(uuid->value.u16, dst); return bt_uuid_len(uuid); case BT_UUID128: bt_uuid_to_uuid128(uuid, &uuid128); bswap_128(&uuid128.value.u128, dst); return bt_uuid_len(&uuid128); } return 0; We do however have bt_uuid_to_le with the only difference being that it is more generic and it doesn't return the actual bytes written to the dst, anyway we could replace the code above with: diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c index b35763410d17..71976de48569 100644 --- a/src/shared/gatt-db.c +++ b/src/shared/gatt-db.c @@ -553,16 +553,9 @@ bool gatt_db_isempty(struct gatt_db *db) static int uuid_to_le(const bt_uuid_t *uuid, uint8_t *dst) { - bt_uuid_t uuid128; + bt_uuid_to_le(uuid, dst); - if (uuid->type == BT_UUID16) { - put_le16(uuid->value.u16, dst); - return bt_uuid_len(uuid); - } - - bt_uuid_to_uuid128(uuid, &uuid128); - bswap_128(&uuid128.value.u128, dst); - return bt_uuid_len(&uuid128); + return bt_uuid_len(uuid) == 4 ? 16 : bt_uuid_len(uuid); } > + uuid->type == BT_UUID128) { > + bt_uuid_to_uuid128(uuid, &uuid128); > + bswap_128(&uuid128.value.u128, dst); > + return bt_uuid_len(&uuid128); > + } > + > + return 0; > } > > static bool le_to_uuid(const uint8_t *src, size_t len, bt_uuid_t *uuid) > -- > 2.45.2 > >
On Tue, 2024-08-06 at 15:08 -0400, Luiz Augusto von Dentz wrote: > Hi Bastien, > > On Mon, Aug 5, 2024 at 10:09 AM Bastien Nocera <hadess@hadess.net> > wrote: > > > > uuid_to_le() returns one of the possible values from bt_uuid_len(). > > bt_uuid_len() returns "type / 8". > > type is a value between 0 and 128, but could be something else > > depending on the validity of the UUID that's parsed. So an invalid > > value of type between 128 and 256 would trigger an overrun. > > > > Add a check to make sure that an invalid type isn't used to > > calculate > > the length. > > > > Error: OVERRUN (CWE-119): [#def6] [important] > > bluez-5.77/src/shared/gatt-db.c:612:2: assignment: Assigning: "len" > > = "uuid_to_le(uuid, value)". The value of "len" is now between 0 > > and 31 (inclusive). > > bluez-5.77/src/shared/gatt-db.c:614:2: overrun-buffer-arg: > > Overrunning array "value" of 16 bytes by passing it to a function > > which accesses it at byte offset 30 using argument "len" (which > > evaluates to 31). > > 612| len = uuid_to_le(uuid, value); > > 613| > > 614|-> service->attributes[0] = new_attribute(service, handle, > > type, value, > > 615| > > len); > > 616| if (!service->attributes[0]) { > > --- > > src/shared/gatt-db.c | 11 ++++++++--- > > 1 file changed, 8 insertions(+), 3 deletions(-) > > > > diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c > > index b35763410d17..cd0eba6bf1d0 100644 > > --- a/src/shared/gatt-db.c > > +++ b/src/shared/gatt-db.c > > @@ -560,9 +560,14 @@ static int uuid_to_le(const bt_uuid_t *uuid, > > uint8_t *dst) > > return bt_uuid_len(uuid); > > } > > > > - bt_uuid_to_uuid128(uuid, &uuid128); > > - bswap_128(&uuid128.value.u128, dst); > > - return bt_uuid_len(&uuid128); > > + if (uuid->type == BT_UUID32 || > > BT_UUID32 is not really valid in LE, so Id leave this check to be: > > switch (uuid->type) { > case BT_UUID16: > put_le16(uuid->value.u16, dst); > return bt_uuid_len(uuid); > case BT_UUID128: > bt_uuid_to_uuid128(uuid, &uuid128); > bswap_128(&uuid128.value.u128, dst); > return bt_uuid_len(&uuid128); > } > > return 0; > > We do however have bt_uuid_to_le with the only difference being that > it is more generic and it doesn't return the actual bytes written to > the dst, anyway we could replace the code above with: > > diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c > index b35763410d17..71976de48569 100644 > --- a/src/shared/gatt-db.c > +++ b/src/shared/gatt-db.c > @@ -553,16 +553,9 @@ bool gatt_db_isempty(struct gatt_db *db) > > static int uuid_to_le(const bt_uuid_t *uuid, uint8_t *dst) > { > - bt_uuid_t uuid128; > + bt_uuid_to_le(uuid, dst); > > - if (uuid->type == BT_UUID16) { > - put_le16(uuid->value.u16, dst); > - return bt_uuid_len(uuid); > - } > - > - bt_uuid_to_uuid128(uuid, &uuid128); > - bswap_128(&uuid128.value.u128, dst); > - return bt_uuid_len(&uuid128); > + return bt_uuid_len(uuid) == 4 ? 16 : bt_uuid_len(uuid); Or this with error checking? static int uuid_to_le(const bt_uuid_t *uuid, uint8_t *dst) { if (bt_uuid_to_le(uuid, dst) < 0) return 0; return bt_uuid_len(uuid) == 4 ? 16 : bt_uuid_len(uuid); } > } > > > > + uuid->type == BT_UUID128) { > > + bt_uuid_to_uuid128(uuid, &uuid128); > > + bswap_128(&uuid128.value.u128, dst); > > + return bt_uuid_len(&uuid128); > > + } > > + > > + return 0; > > } > > > > static bool le_to_uuid(const uint8_t *src, size_t len, bt_uuid_t > > *uuid) > > -- > > 2.45.2 > > > > > >
Hi Bastien, On Wed, Aug 7, 2024 at 11:39 AM Bastien Nocera <hadess@hadess.net> wrote: > > On Tue, 2024-08-06 at 15:08 -0400, Luiz Augusto von Dentz wrote: > > Hi Bastien, > > > > On Mon, Aug 5, 2024 at 10:09 AM Bastien Nocera <hadess@hadess.net> > > wrote: > > > > > > uuid_to_le() returns one of the possible values from bt_uuid_len(). > > > bt_uuid_len() returns "type / 8". > > > type is a value between 0 and 128, but could be something else > > > depending on the validity of the UUID that's parsed. So an invalid > > > value of type between 128 and 256 would trigger an overrun. > > > > > > Add a check to make sure that an invalid type isn't used to > > > calculate > > > the length. > > > > > > Error: OVERRUN (CWE-119): [#def6] [important] > > > bluez-5.77/src/shared/gatt-db.c:612:2: assignment: Assigning: "len" > > > = "uuid_to_le(uuid, value)". The value of "len" is now between 0 > > > and 31 (inclusive). > > > bluez-5.77/src/shared/gatt-db.c:614:2: overrun-buffer-arg: > > > Overrunning array "value" of 16 bytes by passing it to a function > > > which accesses it at byte offset 30 using argument "len" (which > > > evaluates to 31). > > > 612| len = uuid_to_le(uuid, value); > > > 613| > > > 614|-> service->attributes[0] = new_attribute(service, handle, > > > type, value, > > > 615| > > > len); > > > 616| if (!service->attributes[0]) { > > > --- > > > src/shared/gatt-db.c | 11 ++++++++--- > > > 1 file changed, 8 insertions(+), 3 deletions(-) > > > > > > diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c > > > index b35763410d17..cd0eba6bf1d0 100644 > > > --- a/src/shared/gatt-db.c > > > +++ b/src/shared/gatt-db.c > > > @@ -560,9 +560,14 @@ static int uuid_to_le(const bt_uuid_t *uuid, > > > uint8_t *dst) > > > return bt_uuid_len(uuid); > > > } > > > > > > - bt_uuid_to_uuid128(uuid, &uuid128); > > > - bswap_128(&uuid128.value.u128, dst); > > > - return bt_uuid_len(&uuid128); > > > + if (uuid->type == BT_UUID32 || > > > > BT_UUID32 is not really valid in LE, so Id leave this check to be: > > > > switch (uuid->type) { > > case BT_UUID16: > > put_le16(uuid->value.u16, dst); > > return bt_uuid_len(uuid); > > case BT_UUID128: > > bt_uuid_to_uuid128(uuid, &uuid128); > > bswap_128(&uuid128.value.u128, dst); > > return bt_uuid_len(&uuid128); > > } > > > > return 0; > > > > We do however have bt_uuid_to_le with the only difference being that > > it is more generic and it doesn't return the actual bytes written to > > the dst, anyway we could replace the code above with: > > > > diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c > > index b35763410d17..71976de48569 100644 > > --- a/src/shared/gatt-db.c > > +++ b/src/shared/gatt-db.c > > @@ -553,16 +553,9 @@ bool gatt_db_isempty(struct gatt_db *db) > > > > static int uuid_to_le(const bt_uuid_t *uuid, uint8_t *dst) > > { > > - bt_uuid_t uuid128; > > + bt_uuid_to_le(uuid, dst); > > > > - if (uuid->type == BT_UUID16) { > > - put_le16(uuid->value.u16, dst); > > - return bt_uuid_len(uuid); > > - } > > - > > - bt_uuid_to_uuid128(uuid, &uuid128); > > - bswap_128(&uuid128.value.u128, dst); > > - return bt_uuid_len(&uuid128); > > + return bt_uuid_len(uuid) == 4 ? 16 : bt_uuid_len(uuid); > > Or this with error checking? > > static int uuid_to_le(const bt_uuid_t *uuid, uint8_t *dst) > { > if (bt_uuid_to_le(uuid, dst) < 0) > return 0; > return bt_uuid_len(uuid) == 4 ? 16 : bt_uuid_len(uuid); > } Yep, we should check the return of bt_uuid_to_le. > > } > > > > > > > + uuid->type == BT_UUID128) { > > > + bt_uuid_to_uuid128(uuid, &uuid128); > > > + bswap_128(&uuid128.value.u128, dst); > > > + return bt_uuid_len(&uuid128); > > > + } > > > + > > > + return 0; > > > } > > > > > > static bool le_to_uuid(const uint8_t *src, size_t len, bt_uuid_t > > > *uuid) > > > -- > > > 2.45.2 > > > > > > > > > > >
diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c index b35763410d17..cd0eba6bf1d0 100644 --- a/src/shared/gatt-db.c +++ b/src/shared/gatt-db.c @@ -560,9 +560,14 @@ static int uuid_to_le(const bt_uuid_t *uuid, uint8_t *dst) return bt_uuid_len(uuid); } - bt_uuid_to_uuid128(uuid, &uuid128); - bswap_128(&uuid128.value.u128, dst); - return bt_uuid_len(&uuid128); + if (uuid->type == BT_UUID32 || + uuid->type == BT_UUID128) { + bt_uuid_to_uuid128(uuid, &uuid128); + bswap_128(&uuid128.value.u128, dst); + return bt_uuid_len(&uuid128); + } + + return 0; } static bool le_to_uuid(const uint8_t *src, size_t len, bt_uuid_t *uuid)
uuid_to_le() returns one of the possible values from bt_uuid_len(). bt_uuid_len() returns "type / 8". type is a value between 0 and 128, but could be something else depending on the validity of the UUID that's parsed. So an invalid value of type between 128 and 256 would trigger an overrun. Add a check to make sure that an invalid type isn't used to calculate the length. Error: OVERRUN (CWE-119): [#def6] [important] bluez-5.77/src/shared/gatt-db.c:612:2: assignment: Assigning: "len" = "uuid_to_le(uuid, value)". The value of "len" is now between 0 and 31 (inclusive). bluez-5.77/src/shared/gatt-db.c:614:2: overrun-buffer-arg: Overrunning array "value" of 16 bytes by passing it to a function which accesses it at byte offset 30 using argument "len" (which evaluates to 31). 612| len = uuid_to_le(uuid, value); 613| 614|-> service->attributes[0] = new_attribute(service, handle, type, value, 615| len); 616| if (!service->attributes[0]) { --- src/shared/gatt-db.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)