On 7/30/24 16:04, Andrea Bolognani wrote:
> On Mon, Jul 29, 2024 at 11:31:36AM GMT, Michal Privoznik wrote:
>> +static int
>> +qemuProcessPreparePstore(virDomainObj *vm)
>> +{
>> + virDomainPstoreDef *pstore = vm->def->pstore;
>> + VIR_AUTOCLOSE fd = -1;
>> +
>> + if (!pstore)
>> + return 0;
>> +
>> + switch (pstore->backend) {
>> + case VIR_DOMAIN_PSTORE_BACKEND_ACPI_ERST:
>> + if ((fd = open(pstore->path, O_WRONLY | O_CREAT, 0600)) < 0) {
>> + virReportSystemError(errno,
>> + _("cannot create file '%1$s'"),
>> + pstore->path);
>> + return -1;
>> + }
>> +
>> + if (ftruncate(fd, pstore->size) < 0) {
>> + virReportSystemError(errno,
>> + _("Failed to truncate file '%1$s'"),
>> + pstore->path);
>> + return -1;
>> + }
>
> The size is stored in KiB but ftruncate(2) expect it in bytes, so you
> need to multiply it by 1024 or QEMU will (correctly) complain about
> the size mismatch on startup.
>
>> + if (VIR_CLOSE(fd) < 0) {
>> + virReportSystemError(errno,
>> + _("Unable to save '%1$s'"),
>> + pstore->path);
>> + return -1;
>> + }
>> +
>> +
>> + break;
>
> Unnecessary empty line.
>
> Even with the fix mentioned above applied, I still get an error on
> startup on my SELinux-enabled system. Running `setenforce 0` makes it
> go away, but clearly that's not acceptable.
>
> The diff below seems to do the trick, but I'm not entirely confident
> that it's the right fix rather than a mere kludge.
>
>
>
> diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> index ba0ce8fb9d..31df4d22db 100644
> --- a/src/security/security_selinux.c
> +++ b/src/security/security_selinux.c
> @@ -3341,7 +3341,7 @@ virSecuritySELinuxSetAllLabel(virSecurityManager *mgr,
>
> if (def->pstore &&
> virSecuritySELinuxSetFilecon(mgr, def->pstore->path,
> - data->content_context, true) < 0)
> + secdef->imagelabel, true) < 0)
Ah, correct. I don't run SELinux enabled system, hence I did not catch
this in my testing. The thing is, pstore is somewhat similar to NVRAM
store - QEMU must be able to not just read it but also write to it.
Hence, it's different to other files around this line (kernel image,
initrd, DTB, SLIC table) - those are RO. Looking into what NVRAM would
use - it in fact boils down to secdef->imagelabel.
I'll post this as a separate patch.
Thanks for catching this and prompt review!
Michal
