loadpolicy: Verifies memory allocation during policy loading

Commit Message

Yunseong Kim May 27, 2024, 12:54 p.m. UTC

From: Yunseong Kim <[e-mail]>

memory allocation failure handling in the loadpolicy module.

Signed-off-by: Yunseong Kim <[e-mail]>
---
 tools/flask/utils/loadpolicy.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Jan Beulich June 19, 2024, 12:04 p.m. UTC | #1

On 27.05.2024 14:54, [e-mail] wrote:
> --- a/tools/flask/utils/loadpolicy.c
> +++ b/tools/flask/utils/loadpolicy.c
> @@ -58,6 +58,11 @@ int main (int argCnt, const char *args[])
>      }
>  
>      polMemCp = malloc(info.st_size);
> +    if (!polMemCp) {
> +        fprintf(stderr, "Error occurred allocating %ld bytes\n", info.st_size);
> +        ret = -ENOMEM;

I don't think -ENOMEM is valid to use here. See neighboring code. Nevertheless
it is correct that a check should be here.

As to %ld - is that portably usable with an off_t value?

In any event, Daniel, really your turn to review / ack. I'm looking at this
merely because I found this and another bugfix still sit in waiting-for-ack
state.

Jan

Daniel P. Smith June 19, 2024, 2:32 p.m. UTC | #2

On 6/19/24 08:04, Jan Beulich wrote:
> On 27.05.2024 14:54, [e-mail] wrote:
>> --- a/tools/flask/utils/loadpolicy.c
>> +++ b/tools/flask/utils/loadpolicy.c
>> @@ -58,6 +58,11 @@ int main (int argCnt, const char *args[])
>>       }
>>   
>>       polMemCp = malloc(info.st_size);
>> +    if (!polMemCp) {
>> +        fprintf(stderr, "Error occurred allocating %ld bytes\n", info.st_size);
>> +        ret = -ENOMEM;
> 
> I don't think -ENOMEM is valid to use here. See neighboring code. Nevertheless
> it is correct that a check should be here.
> 
> As to %ld - is that portably usable with an off_t value?
> 
> In any event, Daniel, really your turn to review / ack. I'm looking at this
> merely because I found this and another bugfix still sit in waiting-for-ack
> state.

I saw this but was on the fence of whether it really required my ack 
since it was more of a toolstack code fix versus an XSM relevant change.

With that said, and to expand on Jan's comment regarding ENOMEM, the 
utility does not currently differentiate main's return code. Unless the 
tools maintainer wants to start changing this, I would suggest setting 
ret to -1.

As to the '%ld', aligning with Jan's first comment, perhaps you might 
consider just reporting `strerror(errno)` similar to the other error 
handling checks. NB: it is likely errno will be set to -ENOMEM, so by 
doing this you will end up notifying ENOMEM occurred as you were 
attempting to do by providing it with `ret`. Additionally, then you 
won't have to deal with portability concerns over off_t.

V/r,
Daniel P. Smith

Patch

diff --git a/tools/flask/utils/loadpolicy.c b/tools/flask/utils/loadpolicy.c
index 76710a256c..7f6bab4dcd 100644
--- a/tools/flask/utils/loadpolicy.c
+++ b/tools/flask/utils/loadpolicy.c
@@ -58,6 +58,11 @@  int main (int argCnt, const char *args[])
     }
 
     polMemCp = malloc(info.st_size);
+    if (!polMemCp) {
+        fprintf(stderr, "Error occurred allocating %ld bytes\n", info.st_size);
+        ret = -ENOMEM;
+        goto cleanup;
+    }
 
 #ifdef USE_MMAP
     polMem = mmap(NULL, info.st_size, PROT_READ, MAP_SHARED, polFd, 0);