Re: [PATCH] Fix return value from fstat calls

["Tkaczyk, Mariusz" <mariusz.tkaczyk@linux.intel.com>]

+ linux raid

Mariusz

On 11.08.2021 15:03, Tkaczyk, Mariusz wrote:
> On 10.08.2021 17:15, Nigel Croxon wrote:
>> To meet requirements of Common Criteria certification vulnerablility
>> assessment. Static code analysis has been run and found the following
>> errors:
>> check_return: Calling "fstat(fd, &dstb)" without checking return value.
>> This library function may fail and return an error code.
>>
>> Changes are to add a test to the return value from fstat calls.
>>
> 
> Hi Nigel,
> You introduce three different errors, repeated many times across code.
> Could you make it generic using function or macro?
> 
>> diff --git a/Assemble.c b/Assemble.c
>> index 0df46244..cae3224b 100644
>> --- a/Assemble.c
>> +++ b/Assemble.c
>> @@ -649,7 +649,14 @@ static int load_devices(struct devs *devices, char *devmap,
>>               /* prepare useful information in info structures */
>>               struct stat stb2;
>>               int err;
>> -            fstat(mdfd, &stb2);
>> +
>> +            if (fstat(mdfd, &stb2) != 0) {
>> +                pr_err("fstat failed for %s: %s\n",devname, strerror(errno));
>> +                close(mdfd);
>> +                free(devices);
>> +                free(devmap);
>> +                return -1;
>> +            }
> another new case with direct error handling, could you use goto statement?
> 
> 
>> @@ -760,7 +767,17 @@ static int load_devices(struct devs *devices, char *devmap,
>>               tst->ss->getinfo_super(tst, content, devmap + devcnt * 
>> content->array.raid_disks);
>>           }
>> -        fstat(dfd, &stb);
>> +        if (fstat(dfd, &stb) != 0) {
>> +            pr_err("fstat failed for %s: %s - aborting\n",devname, 
>> strerror(errno));
>> +            close(dfd);
>> +            close(mdfd);
>> +            free(devices);
>> +            free(devmap);
>> +            tst->ss->free_super(tst);
>> +            free(tst);
>> +            *stp = st;
>> +            return -1;
>> +        }
> Same here, I know that it is implemented this way, but we should take care about
> code quality, this is our common interest.
> 
>> diff --git a/Dump.c b/Dump.c
>> index 736bcb60..d1a8bb86 100644
>> --- a/Dump.c
>> +++ b/Dump.c
>> @@ -112,7 +112,14 @@ int Dump_metadata(char *dev, char *dir, struct context *c,
>>       }
>>       if (c->verbose >= 0)
>>           printf("%s saved as %s.\n", dev, fname);
>> -    fstat(fd, &dstb);
>> +
>> +    if (fstat(fd, &dstb) != 0) {
>> +        pr_err("fstat failed from %s: %s\n",fname, strerror(errno));
>> +        close(fd);
>> +        close(fl);
>> +        free(fname);
>> +        return 1;
>> +    }
> Same here.
> 
>> @@ -200,7 +207,11 @@ int Restore_metadata(char *dev, char *dir, struct context 
>> *c,
>>           char *chosen = NULL;
>>           unsigned int chosen_inode = 0;
>> -        fstat(fd, &dstb);
>> +        if (fstat(fd, &dstb) != 0) {
>> +            pr_err("fstat failed for %s: %s\n",dev, strerror(errno));
>> +            close(fd);
>> +            return 1;
>> +        }
>>           while (d && (de = readdir(d)) != NULL) {
>>               if (de->d_name[0] == '.')
>> diff --git a/Grow.c b/Grow.c
>> index 7506ab46..4c3ec9c1 100644
>> --- a/Grow.c
>> +++ b/Grow.c
>> @@ -1163,9 +1163,17 @@ int reshape_open_backup_file(char *backup_file,
>>        * way this will not notice, but it is better than
>>        * nothing.
>>        */
>> -    fstat(*fdlist, &stb);
>> +    if (fstat(*fdlist, &stb) != 0) {
>> +        pr_err("fstat failed for %s: %s\n",devname, strerror(errno));
>> +        close(*fdlist);
>> +        return 0;
>> +    }
>>       dev = stb.st_dev;
>> -    fstat(fd, &stb);
>> +    if (fstat(fd, &stb) != 0) {
>> +        pr_err("fstat failed for %s: %s\n",devname, strerror(errno));
>> +        close(*fdlist);
>> +        return 0;
>> +    }
>>       if (stb.st_rdev == dev) {
>>           pr_err("backup file must NOT be on the array being reshaped.\n");
>>           close(*fdlist);
> Same error handling duplicated.
> 
> Thanks,
> Mariusz
>