2021-02-16 20:59:47

by Inga Stotland

[permalink] [raw]
Subject: [PATCH BlueZ] mesh: Combine common functions for NetKeys and AppKeys

No change in functioanality, code tightening.
---
mesh/keyring.c | 120 +++++++++++++++++++++----------------------------
1 file changed, 50 insertions(+), 70 deletions(-)

diff --git a/mesh/keyring.c b/mesh/keyring.c
index 0b74ee914..025703574 100644
--- a/mesh/keyring.c
+++ b/mesh/keyring.c
@@ -33,31 +33,45 @@ const char *dev_key_dir = "/dev_keys";
const char *app_key_dir = "/app_keys";
const char *net_key_dir = "/net_keys";

-bool keyring_put_net_key(struct mesh_node *node, uint16_t net_idx,
- struct keyring_net_key *key)
+static int open_key_file(struct mesh_node *node, const char *key_dir,
+ uint16_t idx, bool is_wrt, int flags)
{
const char *node_path;
- char key_file[PATH_MAX];
- bool result = false;
- int fd;
+ char fname[PATH_MAX];

- if (!node || !key)
- return false;
+ if (!node)
+ return -1;

node_path = node_get_storage_dir(node);

- if (strlen(node_path) + strlen(net_key_dir) + 1 + 3 >= PATH_MAX)
- return false;
+ if (strlen(node_path) + strlen(key_dir) + 1 + 3 >= PATH_MAX)
+ return -1;

- snprintf(key_file, PATH_MAX, "%s%s", node_path, net_key_dir);
+ if (is_wrt) {
+ snprintf(fname, PATH_MAX, "%s%s", node_path, key_dir);
+ mkdir(fname, 0755);
+ }

- mkdir(key_file, 0755);
+ snprintf(fname, PATH_MAX, "%s%s/%3.3x", node_path, key_dir, idx);

- snprintf(key_file, PATH_MAX, "%s%s/%3.3x", node_path, net_key_dir,
- net_idx);
- l_debug("Put Net Key %s", key_file);
+ if (is_wrt)
+ return open(fname, flags, S_IRUSR | S_IWUSR);
+ else
+ return open(fname, flags);
+}
+
+bool keyring_put_net_key(struct mesh_node *node, uint16_t net_idx,
+ struct keyring_net_key *key)
+{
+ bool result = false;
+ int fd;
+
+ if (!key)
+ return false;
+
+ fd = open_key_file(node, net_key_dir, net_idx, true,
+ O_WRONLY | O_CREAT | O_TRUNC);

- fd = open(key_file, O_WRONLY | O_CREAT | O_TRUNC, S_IRUSR | S_IWUSR);
if (fd < 0)
return false;

@@ -72,28 +86,14 @@ bool keyring_put_net_key(struct mesh_node *node, uint16_t net_idx,
bool keyring_put_app_key(struct mesh_node *node, uint16_t app_idx,
uint16_t net_idx, struct keyring_app_key *key)
{
- const char *node_path;
- char key_file[PATH_MAX];
bool result = false;
int fd;

- if (!node || !key)
+ if (!key)
return false;

- node_path = node_get_storage_dir(node);
-
- if (strlen(node_path) + strlen(app_key_dir) + 1 + 3 >= PATH_MAX)
- return false;
+ fd = open_key_file(node, app_key_dir, app_idx, false, O_RDWR);

- snprintf(key_file, PATH_MAX, "%s%s", node_path, app_key_dir);
-
- mkdir(key_file, 0755);
-
- snprintf(key_file, PATH_MAX, "%s%s/%3.3x", node_path, app_key_dir,
- app_idx);
- l_debug("Put App Key %s", key_file);
-
- fd = open(key_file, O_RDWR);
if (fd >= 0) {
struct keyring_app_key old_key;

@@ -105,12 +105,12 @@ bool keyring_put_app_key(struct mesh_node *node, uint16_t app_idx,
}

lseek(fd, 0, SEEK_SET);
- } else {
- fd = open(key_file, O_WRONLY | O_CREAT | O_TRUNC,
- S_IRUSR | S_IWUSR);
- if (fd < 0)
- return false;
- }
+ } else
+ fd = open_key_file(node, app_key_dir, app_idx, true,
+ O_WRONLY | O_CREAT | O_TRUNC);
+
+ if (fd < 0)
+ return false;

if (write(fd, key, sizeof(*key)) == sizeof(*key))
result = true;
@@ -212,8 +212,7 @@ bool keyring_put_remote_dev_key(struct mesh_node *node, uint16_t unicast,
dev_key_dir, unicast + i);
l_debug("Put Dev Key %s", key_file);

- fd = open(key_file, O_WRONLY | O_CREAT | O_TRUNC,
- S_IRUSR | S_IWUSR);
+ fd = open(key_file, O_WRONLY | O_CREAT | O_TRUNC, 0600);
if (fd >= 0) {
if (write(fd, dev_key, 16) != 16)
result = false;
@@ -226,24 +225,19 @@ bool keyring_put_remote_dev_key(struct mesh_node *node, uint16_t unicast,
return result;
}

-bool keyring_get_net_key(struct mesh_node *node, uint16_t net_idx,
- struct keyring_net_key *key)
+static bool get_key(struct mesh_node *node, const char *key_dir,
+ uint16_t key_idx, void *key, ssize_t sz)
{
- const char *node_path;
- char key_file[PATH_MAX];
bool result = false;
int fd;

- if (!node || !key)
+ if (!key)
return false;

- node_path = node_get_storage_dir(node);
- snprintf(key_file, PATH_MAX, "%s%s/%3.3x", node_path, net_key_dir,
- net_idx);
+ fd = open_key_file(node, key_dir, key_idx, false, O_RDONLY);

- fd = open(key_file, O_RDONLY);
if (fd >= 0) {
- if (read(fd, key, sizeof(*key)) == sizeof(*key))
+ if (read(fd, key, sz) == sz)
result = true;

close(fd);
@@ -252,30 +246,16 @@ bool keyring_get_net_key(struct mesh_node *node, uint16_t net_idx,
return result;
}

+bool keyring_get_net_key(struct mesh_node *node, uint16_t net_idx,
+ struct keyring_net_key *key)
+{
+ return get_key(node, net_key_dir, net_idx, key, sizeof(*key));
+}
+
bool keyring_get_app_key(struct mesh_node *node, uint16_t app_idx,
struct keyring_app_key *key)
{
- const char *node_path;
- char key_file[PATH_MAX];
- bool result = false;
- int fd;
-
- if (!node || !key)
- return false;
-
- node_path = node_get_storage_dir(node);
- snprintf(key_file, PATH_MAX, "%s%s/%3.3x", node_path, app_key_dir,
- app_idx);
-
- fd = open(key_file, O_RDONLY);
- if (fd >= 0) {
- if (read(fd, key, sizeof(*key)) == sizeof(*key))
- result = true;
-
- close(fd);
- }
-
- return result;
+ return get_key(node, app_key_dir, app_idx, key, sizeof(*key));
}

bool keyring_get_remote_dev_key(struct mesh_node *node, uint16_t unicast,
--
2.26.2


2021-02-16 21:35:08

by bluez.test.bot

[permalink] [raw]
Subject: RE: [BlueZ] mesh: Combine common functions for NetKeys and AppKeys

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=434249

---Test result---

##############################
Test: CheckPatch - FAIL
Output:
mesh: Combine common functions for NetKeys and AppKeys
WARNING:SYMBOLIC_PERMS: Symbolic permissions 'S_IRUSR | S_IWUSR' are not preferred. Consider using octal permissions '0600'.
#52: FILE: mesh/keyring.c:58:
+ return open(fname, flags, S_IRUSR | S_IWUSR);

- total: 0 errors, 1 warnings, 183 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.

"[PATCH] mesh: Combine common functions for NetKeys and AppKeys" has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: CheckGitLint - PASS

##############################
Test: CheckBuild - PASS

##############################
Test: MakeCheck - PASS



---
Regards,
Linux Bluetooth

2021-02-17 01:07:00

by Brian Gix

[permalink] [raw]
Subject: Re: [PATCH BlueZ] mesh: Combine common functions for NetKeys and AppKeys

Hi Inga,

On Tue, 2021-02-16 at 12:52 -0800, Inga Stotland wrote:
> No change in functioanality, code tightening.
> ---
> mesh/keyring.c | 120 +++++++++++++++++++++----------------------------
> 1 file changed, 50 insertions(+), 70 deletions(-)
>
> diff --git a/mesh/keyring.c b/mesh/keyring.c
> index 0b74ee914..025703574 100644
> --- a/mesh/keyring.c
> +++ b/mesh/keyring.c
> @@ -33,31 +33,45 @@ const char *dev_key_dir = "/dev_keys";
> const char *app_key_dir = "/app_keys";
> const char *net_key_dir = "/net_keys";
>
> -bool keyring_put_net_key(struct mesh_node *node, uint16_t net_idx,
> - struct keyring_net_key *key)
> +static int open_key_file(struct mesh_node *node, const char *key_dir,
> + uint16_t idx, bool is_wrt, int flags)
> {
> const char *node_path;
> - char key_file[PATH_MAX];
> - bool result = false;
> - int fd;
> + char fname[PATH_MAX];
>
> - if (!node || !key)
> - return false;
> + if (!node)
> + return -1;
>
> node_path = node_get_storage_dir(node);
>
> - if (strlen(node_path) + strlen(net_key_dir) + 1 + 3 >= PATH_MAX)
> - return false;
> + if (strlen(node_path) + strlen(key_dir) + 1 + 3 >= PATH_MAX)
> + return -1;
>
> - snprintf(key_file, PATH_MAX, "%s%s", node_path, net_key_dir);
> + if (is_wrt) {
> + snprintf(fname, PATH_MAX, "%s%s", node_path, key_dir);
> + mkdir(fname, 0755);
> + }
>
> - mkdir(key_file, 0755);
> + snprintf(fname, PATH_MAX, "%s%s/%3.3x", node_path, key_dir, idx);
>
> - snprintf(key_file, PATH_MAX, "%s%s/%3.3x", node_path, net_key_dir,
> - net_idx);
> - l_debug("Put Net Key %s", key_file);
> + if (is_wrt)
> + return open(fname, flags, S_IRUSR | S_IWUSR);
> + else
> + return open(fname, flags);
> +}
> +
> +bool keyring_put_net_key(struct mesh_node *node, uint16_t net_idx,
> + struct keyring_net_key *key)
> +{
> + bool result = false;
> + int fd;
> +
> + if (!key)
> + return false;
> +
> + fd = open_key_file(node, net_key_dir, net_idx, true,
> + O_WRONLY | O_CREAT | O_TRUNC);
>
> - fd = open(key_file, O_WRONLY | O_CREAT | O_TRUNC, S_IRUSR | S_IWUSR);
> if (fd < 0)
> return false;
>
> @@ -72,28 +86,14 @@ bool keyring_put_net_key(struct mesh_node *node, uint16_t net_idx,
> bool keyring_put_app_key(struct mesh_node *node, uint16_t app_idx,
> uint16_t net_idx, struct keyring_app_key *key)
> {
> - const char *node_path;
> - char key_file[PATH_MAX];
> bool result = false;
> int fd;
>
> - if (!node || !key)
> + if (!key)
> return false;
>
> - node_path = node_get_storage_dir(node);
> -
> - if (strlen(node_path) + strlen(app_key_dir) + 1 + 3 >= PATH_MAX)
> - return false;
> + fd = open_key_file(node, app_key_dir, app_idx, false, O_RDWR);

keyring_put_app_key writes to the file, but you are setting is_wrt to false. Is that what you want? Or maybe,
since this matches the earlier functionaility, "is_wrt" should actually be "is_create"... Since we aren't
*creating* the file, the S_IRUSR | S_IWUSR permissions do not need to be set.

>
> - snprintf(key_file, PATH_MAX, "%s%s", node_path, app_key_dir);
> -
> - mkdir(key_file, 0755);
> -
> - snprintf(key_file, PATH_MAX, "%s%s/%3.3x", node_path, app_key_dir,
> - app_idx);
> - l_debug("Put App Key %s", key_file);
> -
> - fd = open(key_file, O_RDWR);
> if (fd >= 0) {
> struct keyring_app_key old_key;
>
> @@ -105,12 +105,12 @@ bool keyring_put_app_key(struct mesh_node *node, uint16_t app_idx,
> }
>
> lseek(fd, 0, SEEK_SET);
> - } else {
> - fd = open(key_file, O_WRONLY | O_CREAT | O_TRUNC,
> - S_IRUSR | S_IWUSR);
> - if (fd < 0)
> - return false;
> - }
> + } else
> + fd = open_key_file(node, app_key_dir, app_idx, true,
> + O_WRONLY | O_CREAT | O_TRUNC);
> +
> + if (fd < 0)
> + return false;
>
> if (write(fd, key, sizeof(*key)) == sizeof(*key))
> result = true;
> @@ -212,8 +212,7 @@ bool keyring_put_remote_dev_key(struct mesh_node *node, uint16_t unicast,
> dev_key_dir, unicast + i);
> l_debug("Put Dev Key %s", key_file);
>
> - fd = open(key_file, O_WRONLY | O_CREAT | O_TRUNC,
> - S_IRUSR | S_IWUSR);
> + fd = open(key_file, O_WRONLY | O_CREAT | O_TRUNC, 0600);

So are we using S_IRUSR | S_IWUSR or 0600?

> if (fd >= 0) {
> if (write(fd, dev_key, 16) != 16)
> result = false;
> @@ -226,24 +225,19 @@ bool keyring_put_remote_dev_key(struct mesh_node *node, uint16_t unicast,
> return result;
> }
>
> -bool keyring_get_net_key(struct mesh_node *node, uint16_t net_idx,
> - struct keyring_net_key *key)
> +static bool get_key(struct mesh_node *node, const char *key_dir,
> + uint16_t key_idx, void *key, ssize_t sz)
> {
> - const char *node_path;
> - char key_file[PATH_MAX];
> bool result = false;
> int fd;
>
> - if (!node || !key)
> + if (!key)
> return false;
>
> - node_path = node_get_storage_dir(node);
> - snprintf(key_file, PATH_MAX, "%s%s/%3.3x", node_path, net_key_dir,
> - net_idx);
> + fd = open_key_file(node, key_dir, key_idx, false, O_RDONLY);
>
> - fd = open(key_file, O_RDONLY);
> if (fd >= 0) {
> - if (read(fd, key, sizeof(*key)) == sizeof(*key))
> + if (read(fd, key, sz) == sz)
> result = true;
>
> close(fd);
> @@ -252,30 +246,16 @@ bool keyring_get_net_key(struct mesh_node *node, uint16_t net_idx,
> return result;
> }
>
> +bool keyring_get_net_key(struct mesh_node *node, uint16_t net_idx,
> + struct keyring_net_key *key)
> +{
> + return get_key(node, net_key_dir, net_idx, key, sizeof(*key));
> +}
> +
> bool keyring_get_app_key(struct mesh_node *node, uint16_t app_idx,
> struct keyring_app_key *key)
> {
> - const char *node_path;
> - char key_file[PATH_MAX];
> - bool result = false;
> - int fd;
> -
> - if (!node || !key)
> - return false;
> -
> - node_path = node_get_storage_dir(node);
> - snprintf(key_file, PATH_MAX, "%s%s/%3.3x", node_path, app_key_dir,
> - app_idx);
> -
> - fd = open(key_file, O_RDONLY);
> - if (fd >= 0) {
> - if (read(fd, key, sizeof(*key)) == sizeof(*key))
> - result = true;
> -
> - close(fd);
> - }
> -
> - return result;
> + return get_key(node, app_key_dir, app_idx, key, sizeof(*key));
> }
>
> bool keyring_get_remote_dev_key(struct mesh_node *node, uint16_t unicast,