| Message ID | 20230314180308.17909-1-romanosauce57@gmail.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | ima: Fix potential NULL pointer access in ima_match_rules() | expand |
On Tue, 2023-03-14 at 21:03 +0300, Roman Danilov wrote: > In ima_match_rules(), when ima_lsm_copy_rule() fails, NULL pointer > is assigned to lsm_rule. After that, in the next step of the loop > NULL pointer is dereferenced in lsm_rule->lsm[i].rule. I must being missing something. The next step of the loop tests whether rule_reinitialized is set before accessing lsm_rule- >lsm[i].rule. > > As far as ima_match_rules() is not designed to return error code, > add __GFP_NOFAIL to make sure memory allocation succeeds. Using __GFP_NOFAIL here would be safer. > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > Fixes: c7423dbdbc9e ("ima: Handle -ESTALE returned by ima_filter_rule_match()") > Signed-off-by: Roman Danilov <romanosauce57@gmail.com> > Reviewed-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
Tested-by: GUO Zihua <guozihua@huawei.com>
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 3ca8b7348c2e..1b6bfcbcdeac 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -401,7 +401,8 @@ static void ima_free_rule(struct ima_rule_entry *entry) kfree(entry); } -static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry) +static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry, + gfp_t gfp) { struct ima_rule_entry *nentry; int i; @@ -410,7 +411,7 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry) * Immutable elements are copied over as pointers and data; only * lsm rules can change */ - nentry = kmemdup(entry, sizeof(*nentry), GFP_KERNEL); + nentry = kmemdup(entry, sizeof(*nentry), gfp); if (!nentry) return NULL; @@ -438,7 +439,7 @@ static int ima_lsm_update_rule(struct ima_rule_entry *entry) int i; struct ima_rule_entry *nentry; - nentry = ima_lsm_copy_rule(entry); + nentry = ima_lsm_copy_rule(entry, GFP_KERNEL); if (!nentry) return -ENOMEM; @@ -664,11 +665,10 @@ static bool ima_match_rules(struct ima_rule_entry *rule, } if (rc == -ESTALE && !rule_reinitialized) { - lsm_rule = ima_lsm_copy_rule(rule); - if (lsm_rule) { - rule_reinitialized = true; - goto retry; - } + lsm_rule = ima_lsm_copy_rule(rule, + GFP_KERNEL | __GFP_NOFAIL); + rule_reinitialized = true; + goto retry; } if (!rc) { result = false;
In ima_match_rules(), when ima_lsm_copy_rule() fails, NULL pointer is assigned to lsm_rule. After that, in the next step of the loop NULL pointer is dereferenced in lsm_rule->lsm[i].rule. As far as ima_match_rules() is not designed to return error code, add __GFP_NOFAIL to make sure memory allocation succeeds. Found by Linux Verification Center (linuxtesting.org) with SVACE. Fixes: c7423dbdbc9e ("ima: Handle -ESTALE returned by ima_filter_rule_match()") Signed-off-by: Roman Danilov <romanosauce57@gmail.com> Reviewed-by: Alexey Khoroshilov <khoroshilov@ispras.ru> --- security/integrity/ima/ima_policy.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)