* [PATCH 1/2] KEYS: trusted: Use common error handling code in trusted_update()
@ 2017-11-10 20:29 ` SF Markus Elfring
0 siblings, 0 replies; 15+ messages in thread
From: SF Markus Elfring @ 2017-11-10 20:29 UTC (permalink / raw)
To: linux-security-module
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 10 Nov 2017 20:50:15 +0100
Adjust jump targets so that a bit of exception handling can be better
reused at the end of this function.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
security/keys/trusted.c | 44 ++++++++++++++++++++------------------------
1 file changed, 20 insertions(+), 24 deletions(-)
diff --git a/security/keys/trusted.c b/security/keys/trusted.c
index bd85315cbfeb..fd06d0c5323b 100644
--- a/security/keys/trusted.c
+++ b/security/keys/trusted.c
@@ -1078,30 +1078,18 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep)
if (!datablob)
return -ENOMEM;
new_o = trusted_options_alloc();
- if (!new_o) {
- ret = -ENOMEM;
- goto out;
- }
+ if (!new_o)
+ goto e_nomem;
+
new_p = trusted_payload_alloc(key);
- if (!new_p) {
- ret = -ENOMEM;
- goto out;
- }
+ if (!new_p)
+ goto e_nomem;
memcpy(datablob, prep->data, datalen);
datablob[datalen] = '\0';
ret = datablob_parse(datablob, new_p, new_o);
- if (ret != Opt_update) {
- ret = -EINVAL;
- kzfree(new_p);
- goto out;
- }
-
- if (!new_o->keyhandle) {
- ret = -EINVAL;
- kzfree(new_p);
- goto out;
- }
+ if (ret != Opt_update || !new_o->keyhandle)
+ goto e_inval;
/* copy old key values, and reseal with new pcrs */
new_p->migratable = p->migratable;
@@ -1113,23 +1101,31 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep)
ret = key_seal(new_p, new_o);
if (ret < 0) {
pr_info("trusted_key: key_seal failed (%d)\n", ret);
- kzfree(new_p);
- goto out;
+ goto free_payload;
}
if (new_o->pcrlock) {
ret = pcrlock(new_o->pcrlock);
if (ret < 0) {
pr_info("trusted_key: pcrlock failed (%d)\n", ret);
- kzfree(new_p);
- goto out;
+ goto free_payload;
}
}
rcu_assign_keypointer(key, new_p);
call_rcu(&p->rcu, trusted_rcu_free);
-out:
+free_data:
kzfree(datablob);
kzfree(new_o);
return ret;
+
+e_nomem:
+ ret = -ENOMEM;
+ goto free_data;
+
+e_inval:
+ ret = -EINVAL;
+free_payload:
+ kzfree(new_p);
+ goto free_data;
}
/*
--
2.15.0
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 1/2] KEYS: trusted: Use common error handling code in trusted_update()
@ 2017-11-10 20:29 ` SF Markus Elfring
0 siblings, 0 replies; 15+ messages in thread
From: SF Markus Elfring @ 2017-11-10 20:29 UTC (permalink / raw)
To: keyrings, linux-integrity, linux-security-module, David Howells,
James Morris, Mimi Zohar, Serge E. Hallyn
Cc: LKML, kernel-janitors
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 10 Nov 2017 20:50:15 +0100
Adjust jump targets so that a bit of exception handling can be better
reused at the end of this function.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
security/keys/trusted.c | 44 ++++++++++++++++++++------------------------
1 file changed, 20 insertions(+), 24 deletions(-)
diff --git a/security/keys/trusted.c b/security/keys/trusted.c
index bd85315cbfeb..fd06d0c5323b 100644
--- a/security/keys/trusted.c
+++ b/security/keys/trusted.c
@@ -1078,30 +1078,18 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep)
if (!datablob)
return -ENOMEM;
new_o = trusted_options_alloc();
- if (!new_o) {
- ret = -ENOMEM;
- goto out;
- }
+ if (!new_o)
+ goto e_nomem;
+
new_p = trusted_payload_alloc(key);
- if (!new_p) {
- ret = -ENOMEM;
- goto out;
- }
+ if (!new_p)
+ goto e_nomem;
memcpy(datablob, prep->data, datalen);
datablob[datalen] = '\0';
ret = datablob_parse(datablob, new_p, new_o);
- if (ret != Opt_update) {
- ret = -EINVAL;
- kzfree(new_p);
- goto out;
- }
-
- if (!new_o->keyhandle) {
- ret = -EINVAL;
- kzfree(new_p);
- goto out;
- }
+ if (ret != Opt_update || !new_o->keyhandle)
+ goto e_inval;
/* copy old key values, and reseal with new pcrs */
new_p->migratable = p->migratable;
@@ -1113,23 +1101,31 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep)
ret = key_seal(new_p, new_o);
if (ret < 0) {
pr_info("trusted_key: key_seal failed (%d)\n", ret);
- kzfree(new_p);
- goto out;
+ goto free_payload;
}
if (new_o->pcrlock) {
ret = pcrlock(new_o->pcrlock);
if (ret < 0) {
pr_info("trusted_key: pcrlock failed (%d)\n", ret);
- kzfree(new_p);
- goto out;
+ goto free_payload;
}
}
rcu_assign_keypointer(key, new_p);
call_rcu(&p->rcu, trusted_rcu_free);
-out:
+free_data:
kzfree(datablob);
kzfree(new_o);
return ret;
+
+e_nomem:
+ ret = -ENOMEM;
+ goto free_data;
+
+e_inval:
+ ret = -EINVAL;
+free_payload:
+ kzfree(new_p);
+ goto free_data;
}
/*
--
2.15.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 1/2] KEYS: trusted: Use common error handling code in trusted_update()
2017-11-10 20:29 ` SF Markus Elfring
(?)
@ 2017-11-10 20:52 ` Julia Lawall
-1 siblings, 0 replies; 15+ messages in thread
From: Julia Lawall @ 2017-11-10 20:52 UTC (permalink / raw)
To: SF Markus Elfring
Cc: keyrings, linux-integrity, linux-security-module, David Howells,
James Morris, Mimi Zohar, Serge E. Hallyn, LKML, kernel-janitors
On Fri, 10 Nov 2017, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 10 Nov 2017 20:50:15 +0100
>
> Adjust jump targets so that a bit of exception handling can be better
> reused at the end of this function.
Unless there is a strong motivation for doing otherwise, the goal should
be to make the code understandable and safe. Understandable means that
issues specific to the error that occurred should be up at the place where
the error occurs, ie any prints or any setting of return code. Safe means
that cleanup code should appear once in a cascade at the end of the
function, to minimize the chance that anything will be overlooked.
Moving the ret assignments to the end of the function and adding the
backward jumps doesn't make the code more understandable. A lot of mental
effort is required to strace through the spaghetti code to find out what
exactly will be the impact of a given failure.
On the other hand, moving the kzalloc of new_p to the end of the function
could be helpful, because it reduces the chance that new error handling
code, if any turns out to be needed, will forget this operation.
By why not just follow standard practice and free the structures in a
cascade in the inverse of the order in which they are allocated at the end
of the function? There can be a descriptive label for each thing that
needs to be freed.
julia
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
> security/keys/trusted.c | 44 ++++++++++++++++++++------------------------
> 1 file changed, 20 insertions(+), 24 deletions(-)
>
> diff --git a/security/keys/trusted.c b/security/keys/trusted.c
> index bd85315cbfeb..fd06d0c5323b 100644
> --- a/security/keys/trusted.c
> +++ b/security/keys/trusted.c
> @@ -1078,30 +1078,18 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep)
> if (!datablob)
> return -ENOMEM;
> new_o = trusted_options_alloc();
> - if (!new_o) {
> - ret = -ENOMEM;
> - goto out;
> - }
> + if (!new_o)
> + goto e_nomem;
> +
> new_p = trusted_payload_alloc(key);
> - if (!new_p) {
> - ret = -ENOMEM;
> - goto out;
> - }
> + if (!new_p)
> + goto e_nomem;
>
> memcpy(datablob, prep->data, datalen);
> datablob[datalen] = '\0';
> ret = datablob_parse(datablob, new_p, new_o);
> - if (ret != Opt_update) {
> - ret = -EINVAL;
> - kzfree(new_p);
> - goto out;
> - }
> -
> - if (!new_o->keyhandle) {
> - ret = -EINVAL;
> - kzfree(new_p);
> - goto out;
> - }
> + if (ret != Opt_update || !new_o->keyhandle)
> + goto e_inval;
>
> /* copy old key values, and reseal with new pcrs */
> new_p->migratable = p->migratable;
> @@ -1113,23 +1101,31 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep)
> ret = key_seal(new_p, new_o);
> if (ret < 0) {
> pr_info("trusted_key: key_seal failed (%d)\n", ret);
> - kzfree(new_p);
> - goto out;
> + goto free_payload;
> }
> if (new_o->pcrlock) {
> ret = pcrlock(new_o->pcrlock);
> if (ret < 0) {
> pr_info("trusted_key: pcrlock failed (%d)\n", ret);
> - kzfree(new_p);
> - goto out;
> + goto free_payload;
> }
> }
> rcu_assign_keypointer(key, new_p);
> call_rcu(&p->rcu, trusted_rcu_free);
> -out:
> +free_data:
> kzfree(datablob);
> kzfree(new_o);
> return ret;
> +
> +e_nomem:
> + ret = -ENOMEM;
> + goto free_data;
> +
> +e_inval:
> + ret = -EINVAL;
> +free_payload:
> + kzfree(new_p);
> + goto free_data;
> }
>
> /*
> --
> 2.15.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH 1/2] KEYS: trusted: Use common error handling code in trusted_update()
@ 2017-11-10 20:52 ` Julia Lawall
0 siblings, 0 replies; 15+ messages in thread
From: Julia Lawall @ 2017-11-10 20:52 UTC (permalink / raw)
To: linux-security-module
On Fri, 10 Nov 2017, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 10 Nov 2017 20:50:15 +0100
>
> Adjust jump targets so that a bit of exception handling can be better
> reused at the end of this function.
Unless there is a strong motivation for doing otherwise, the goal should
be to make the code understandable and safe. Understandable means that
issues specific to the error that occurred should be up at the place where
the error occurs, ie any prints or any setting of return code. Safe means
that cleanup code should appear once in a cascade at the end of the
function, to minimize the chance that anything will be overlooked.
Moving the ret assignments to the end of the function and adding the
backward jumps doesn't make the code more understandable. A lot of mental
effort is required to strace through the spaghetti code to find out what
exactly will be the impact of a given failure.
On the other hand, moving the kzalloc of new_p to the end of the function
could be helpful, because it reduces the chance that new error handling
code, if any turns out to be needed, will forget this operation.
By why not just follow standard practice and free the structures in a
cascade in the inverse of the order in which they are allocated at the end
of the function? There can be a descriptive label for each thing that
needs to be freed.
julia
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
> security/keys/trusted.c | 44 ++++++++++++++++++++------------------------
> 1 file changed, 20 insertions(+), 24 deletions(-)
>
> diff --git a/security/keys/trusted.c b/security/keys/trusted.c
> index bd85315cbfeb..fd06d0c5323b 100644
> --- a/security/keys/trusted.c
> +++ b/security/keys/trusted.c
> @@ -1078,30 +1078,18 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep)
> if (!datablob)
> return -ENOMEM;
> new_o = trusted_options_alloc();
> - if (!new_o) {
> - ret = -ENOMEM;
> - goto out;
> - }
> + if (!new_o)
> + goto e_nomem;
> +
> new_p = trusted_payload_alloc(key);
> - if (!new_p) {
> - ret = -ENOMEM;
> - goto out;
> - }
> + if (!new_p)
> + goto e_nomem;
>
> memcpy(datablob, prep->data, datalen);
> datablob[datalen] = '\0';
> ret = datablob_parse(datablob, new_p, new_o);
> - if (ret != Opt_update) {
> - ret = -EINVAL;
> - kzfree(new_p);
> - goto out;
> - }
> -
> - if (!new_o->keyhandle) {
> - ret = -EINVAL;
> - kzfree(new_p);
> - goto out;
> - }
> + if (ret != Opt_update || !new_o->keyhandle)
> + goto e_inval;
>
> /* copy old key values, and reseal with new pcrs */
> new_p->migratable = p->migratable;
> @@ -1113,23 +1101,31 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep)
> ret = key_seal(new_p, new_o);
> if (ret < 0) {
> pr_info("trusted_key: key_seal failed (%d)\n", ret);
> - kzfree(new_p);
> - goto out;
> + goto free_payload;
> }
> if (new_o->pcrlock) {
> ret = pcrlock(new_o->pcrlock);
> if (ret < 0) {
> pr_info("trusted_key: pcrlock failed (%d)\n", ret);
> - kzfree(new_p);
> - goto out;
> + goto free_payload;
> }
> }
> rcu_assign_keypointer(key, new_p);
> call_rcu(&p->rcu, trusted_rcu_free);
> -out:
> +free_data:
> kzfree(datablob);
> kzfree(new_o);
> return ret;
> +
> +e_nomem:
> + ret = -ENOMEM;
> + goto free_data;
> +
> +e_inval:
> + ret = -EINVAL;
> +free_payload:
> + kzfree(new_p);
> + goto free_data;
> }
>
> /*
> --
> 2.15.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 1/2] KEYS: trusted: Use common error handling code in trusted_update()
@ 2017-11-10 20:52 ` Julia Lawall
0 siblings, 0 replies; 15+ messages in thread
From: Julia Lawall @ 2017-11-10 20:52 UTC (permalink / raw)
To: SF Markus Elfring
Cc: keyrings, linux-integrity, linux-security-module, David Howells,
James Morris, Mimi Zohar, Serge E. Hallyn, LKML, kernel-janitors
On Fri, 10 Nov 2017, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 10 Nov 2017 20:50:15 +0100
>
> Adjust jump targets so that a bit of exception handling can be better
> reused at the end of this function.
Unless there is a strong motivation for doing otherwise, the goal should
be to make the code understandable and safe. Understandable means that
issues specific to the error that occurred should be up at the place where
the error occurs, ie any prints or any setting of return code. Safe means
that cleanup code should appear once in a cascade at the end of the
function, to minimize the chance that anything will be overlooked.
Moving the ret assignments to the end of the function and adding the
backward jumps doesn't make the code more understandable. A lot of mental
effort is required to strace through the spaghetti code to find out what
exactly will be the impact of a given failure.
On the other hand, moving the kzalloc of new_p to the end of the function
could be helpful, because it reduces the chance that new error handling
code, if any turns out to be needed, will forget this operation.
By why not just follow standard practice and free the structures in a
cascade in the inverse of the order in which they are allocated at the end
of the function? There can be a descriptive label for each thing that
needs to be freed.
julia
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
> security/keys/trusted.c | 44 ++++++++++++++++++++------------------------
> 1 file changed, 20 insertions(+), 24 deletions(-)
>
> diff --git a/security/keys/trusted.c b/security/keys/trusted.c
> index bd85315cbfeb..fd06d0c5323b 100644
> --- a/security/keys/trusted.c
> +++ b/security/keys/trusted.c
> @@ -1078,30 +1078,18 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep)
> if (!datablob)
> return -ENOMEM;
> new_o = trusted_options_alloc();
> - if (!new_o) {
> - ret = -ENOMEM;
> - goto out;
> - }
> + if (!new_o)
> + goto e_nomem;
> +
> new_p = trusted_payload_alloc(key);
> - if (!new_p) {
> - ret = -ENOMEM;
> - goto out;
> - }
> + if (!new_p)
> + goto e_nomem;
>
> memcpy(datablob, prep->data, datalen);
> datablob[datalen] = '\0';
> ret = datablob_parse(datablob, new_p, new_o);
> - if (ret != Opt_update) {
> - ret = -EINVAL;
> - kzfree(new_p);
> - goto out;
> - }
> -
> - if (!new_o->keyhandle) {
> - ret = -EINVAL;
> - kzfree(new_p);
> - goto out;
> - }
> + if (ret != Opt_update || !new_o->keyhandle)
> + goto e_inval;
>
> /* copy old key values, and reseal with new pcrs */
> new_p->migratable = p->migratable;
> @@ -1113,23 +1101,31 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep)
> ret = key_seal(new_p, new_o);
> if (ret < 0) {
> pr_info("trusted_key: key_seal failed (%d)\n", ret);
> - kzfree(new_p);
> - goto out;
> + goto free_payload;
> }
> if (new_o->pcrlock) {
> ret = pcrlock(new_o->pcrlock);
> if (ret < 0) {
> pr_info("trusted_key: pcrlock failed (%d)\n", ret);
> - kzfree(new_p);
> - goto out;
> + goto free_payload;
> }
> }
> rcu_assign_keypointer(key, new_p);
> call_rcu(&p->rcu, trusted_rcu_free);
> -out:
> +free_data:
> kzfree(datablob);
> kzfree(new_o);
> return ret;
> +
> +e_nomem:
> + ret = -ENOMEM;
> + goto free_data;
> +
> +e_inval:
> + ret = -EINVAL;
> +free_payload:
> + kzfree(new_p);
> + goto free_data;
> }
>
> /*
> --
> 2.15.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: KEYS: trusted: Use common error handling code in trusted_update()
2017-11-10 20:52 ` Julia Lawall
(?)
@ 2017-11-11 9:37 ` SF Markus Elfring
-1 siblings, 0 replies; 15+ messages in thread
From: SF Markus Elfring @ 2017-11-11 9:37 UTC (permalink / raw)
To: Julia Lawall, keyrings, linux-integrity, linux-security-module
Cc: David Howells, James Morris, Mimi Zohar, Serge E. Hallyn, LKML,
kernel-janitors
> Safe means that cleanup code should appear once in a cascade at the end
> of the function, to minimize the chance that anything will be overlooked.
I find that the control flow of this function implementation does not fit
to the mentioned ideal so far.
> Moving the ret assignments to the end of the function and adding the
> backward jumps doesn't make the code more understandable.
Is this structure required if you would like to achieve something
in the shown software design direction?
> On the other hand, moving the kzalloc of new_p to the end of the function
> could be helpful,
Why do you think that the movement of this function call can finally work
in the concrete software situation?
> because it reduces the chance that new error handling code,
> if any turns out to be needed, will forget this operation.
Your expectation can be nice.
> By why not just follow standard practice and free the structures in a
> cascade in the inverse of the order in which they are allocated at the end
> of the function?
This is still happening here partly, isn't it?
> There can be a descriptive label for each thing that needs to be freed.
Which identifiers would you find more appropriate in comparison to
my suggestion?
* e_inval
* e_nomem
* free_data
* free_payload
Regards,
Markus
^ permalink raw reply [flat|nested] 15+ messages in thread
* KEYS: trusted: Use common error handling code in trusted_update()
@ 2017-11-11 9:37 ` SF Markus Elfring
0 siblings, 0 replies; 15+ messages in thread
From: SF Markus Elfring @ 2017-11-11 9:37 UTC (permalink / raw)
To: linux-security-module
> Safe means that cleanup code should appear once in a cascade at the end
> of the function, to minimize the chance that anything will be overlooked.
I find that the control flow of this function implementation does not fit
to the mentioned ideal so far.
> Moving the ret assignments to the end of the function and adding the
> backward jumps doesn't make the code more understandable.
Is this structure required if you would like to achieve something
in the shown software design direction?
> On the other hand, moving the kzalloc of new_p to the end of the function
> could be helpful,
Why do you think that the movement of this function call can finally work
in the concrete software situation?
> because it reduces the chance that new error handling code,
> if any turns out to be needed, will forget this operation.
Your expectation can be nice.
> By why not just follow standard practice and free the structures in a
> cascade in the inverse of the order in which they are allocated at the end
> of the function?
This is still happening here partly, isn't it?
> There can be a descriptive label for each thing that needs to be freed.
Which identifiers would you find more appropriate in comparison to
my suggestion?
* e_inval
* e_nomem
* free_data
* free_payload
Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: KEYS: trusted: Use common error handling code in trusted_update()
@ 2017-11-11 9:37 ` SF Markus Elfring
0 siblings, 0 replies; 15+ messages in thread
From: SF Markus Elfring @ 2017-11-11 9:37 UTC (permalink / raw)
To: Julia Lawall, keyrings, linux-integrity, linux-security-module
Cc: David Howells, James Morris, Mimi Zohar, Serge E. Hallyn, LKML,
kernel-janitors
> Safe means that cleanup code should appear once in a cascade at the end
> of the function, to minimize the chance that anything will be overlooked.
I find that the control flow of this function implementation does not fit
to the mentioned ideal so far.
> Moving the ret assignments to the end of the function and adding the
> backward jumps doesn't make the code more understandable.
Is this structure required if you would like to achieve something
in the shown software design direction?
> On the other hand, moving the kzalloc of new_p to the end of the function
> could be helpful,
Why do you think that the movement of this function call can finally work
in the concrete software situation?
> because it reduces the chance that new error handling code,
> if any turns out to be needed, will forget this operation.
Your expectation can be nice.
> By why not just follow standard practice and free the structures in a
> cascade in the inverse of the order in which they are allocated at the end
> of the function?
This is still happening here partly, isn't it?
> There can be a descriptive label for each thing that needs to be freed.
Which identifiers would you find more appropriate in comparison to
my suggestion?
* e_inval
* e_nomem
* free_data
* free_payload
Regards,
Markus
^ permalink raw reply [flat|nested] 15+ messages in thread