[libvirt] [PATCH v3 11/18] security_selinux: Remember old labels

[Michal Prívozník]

On 12/20/18 12:39 AM, John Ferlan wrote:
> 
> 
> On 12/12/18 7:40 AM, Michal Privoznik wrote:
>> Similarly to what I did in DAC driver, this also requires the
>> same SELinux label to be used for shared paths. If a path is
>> already in use by a domain (or domains) then and the domain we
>> are starting now wants to access the path it has to have the same
>> SELinux label. This might look too restrictive as the new label
>> can still guarantee access to already running domains but in
>> reality it is very unlikely and usually an admin mistake.
>>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>>  src/security/security_selinux.c | 177 +++++++++++++++++++++++---------
>>  1 file changed, 130 insertions(+), 47 deletions(-)
>>
> 
> [...]
> 
>>  static int
>> @@ -1362,7 +1429,8 @@ getContext(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
>>   * errors that the caller(s) are already dealing with */
>>  static int
>>  virSecuritySELinuxRestoreFileLabel(virSecurityManagerPtr mgr,
>> -                                   const char *path)
>> +                                   const char *path,
>> +                                   bool recall)
>>  {
>>      bool privileged = virSecurityManagerGetPrivileged(mgr);
>>      struct stat buf;
>> @@ -1386,26 +1454,35 @@ virSecuritySELinuxRestoreFileLabel(virSecurityManagerPtr mgr,
>>          goto cleanup;
>>      }
>>  
>> -    if (stat(newpath, &buf) != 0) {
>> -        VIR_WARN("cannot stat %s: %s", newpath,
>> -                 virStrerror(errno, ebuf, sizeof(ebuf)));
>> -        goto cleanup;
>> -    }
>> -
>> -    if (getContext(mgr, newpath, buf.st_mode, &fcon) < 0) {
>> -        /* Any user created path likely does not have a default label,
>> -         * which makes this an expected non error
>> -         */
>> -        VIR_WARN("cannot lookup default selinux label for %s", newpath);
>> -        ret = 0;
>> -        goto cleanup;
>> -    }
>> -
>> -    if ((rc = virSecuritySELinuxTransactionAppend(path, fcon, false, true)) < 0)
>> +    if ((rc = virSecuritySELinuxTransactionAppend(path, NULL, false, true)) < 0)
>>          return -1;
>>      else if (rc > 0)
>>          return 0;
>>  
> 
> Since you've touched the code, Coverity looks again and determines that
> @newpath can be leaked above

Ah, right. this should have been "goto cleanup" instead of "return -1"
and "{ret = 0; goto cleanup}" instead of "return 0".

Michal