On Mon, Aug 16, 2010 at 02:33:01PM -0500, Jamie Strandboge wrote:

On Mon, 2010-08-16 at 17:14 +0100, Daniel P. Berrange wrote:

...

You can't blindly de-reference data.file.path - The 'file' struct is inside a union and is only valid for certain types of character device VIR_DOMAIN_CHR_TYPE_PTY, TYPE_DEV, TYPE_FILE and TYPE_PIPE. The existing code for serial devices is broken too & can crash due to this

Hrmm. I always did it this. Maybe something changed and I missed it. Regardless, thanks for this feedback! :) Attached is an updated patch.

-- Jamie Strandboge | http://www.canonical.com

Author: Jamie Strandboge <jamie@canonical.com> Description: Check for VIR_DOMAIN_CHR_TYPE in serial ports and add 'rw' for defined serial ports, parallel ports and channels Bug-Ubuntu: LP: #578527, LP: #609055

Index: libvirt-0.8.3/src/security/virt-aa-helper.c =================================================================== --- libvirt-0.8.3.orig/src/security/virt-aa-helper.c 2010-08-16 14:10:42.000000000 -0500 +++ libvirt-0.8.3/src/security/virt-aa-helper.c 2010-08-16 14:14:15.000000000 -0500 @@ -875,15 +875,44 @@ }

for (i = 0; i < ctl->def->nserials; i++) - if (ctl->def->serials[i] && ctl->def->serials[i]->data.file.path) + if (ctl->def->serials[i] && + (ctl->def->serials[i]->type == VIR_DOMAIN_CHR_TYPE_PTY || + ctl->def->serials[i]->type == VIR_DOMAIN_CHR_TYPE_DEV || + ctl->def->serials[i]->type == VIR_DOMAIN_CHR_TYPE_FILE || + ctl->def->serials[i]->type == VIR_DOMAIN_CHR_TYPE_PIPE) && + ctl->def->serials[i]->data.file.path) if (vah_add_file(&buf, - ctl->def->serials[i]->data.file.path, "w") != 0) + ctl->def->serials[i]->data.file.path, "rw") != 0) goto clean;

if (ctl->def->console && ctl->def->console->data.file.path) - if (vah_add_file(&buf, ctl->def->console->data.file.path, "w") != 0) + if (vah_add_file(&buf, ctl->def->console->data.file.path, "rw") != 0) goto clean;

+ for (i = 0 ; i < ctl->def->nparallels; i++) + if (ctl->def->parallels[i] && + (ctl->def->parallels[i]->type == VIR_DOMAIN_CHR_TYPE_PTY || + ctl->def->parallels[i]->type == VIR_DOMAIN_CHR_TYPE_DEV || + ctl->def->parallels[i]->type == VIR_DOMAIN_CHR_TYPE_FILE || + ctl->def->parallels[i]->type == VIR_DOMAIN_CHR_TYPE_PIPE) && + ctl->def->parallels[i]->data.file.path) + if (vah_add_file(&buf, + ctl->def->parallels[i]->data.file.path, + "rw") != 0) + goto clean; + + for (i = 0 ; i < ctl->def->nchannels; i++) + if (ctl->def->channels[i] && + (ctl->def->channels[i]->type == VIR_DOMAIN_CHR_TYPE_PTY || + ctl->def->channels[i]->type == VIR_DOMAIN_CHR_TYPE_DEV || + ctl->def->channels[i]->type == VIR_DOMAIN_CHR_TYPE_FILE || + ctl->def->channels[i]->type == VIR_DOMAIN_CHR_TYPE_PIPE) && + ctl->def->channels[i]->data.file.path) + if (vah_add_file(&buf, + ctl->def->channels[i]->data.file.path, + "rw") != 0) + goto clean; + if (ctl->def->os.kernel) if (vah_add_file(&buf, ctl->def->os.kernel, "r") != 0) goto clean; Index: libvirt-0.8.3/tests/virt-aa-helper-test =================================================================== --- libvirt-0.8.3.orig/tests/virt-aa-helper-test 2010-08-16 14:10:42.000000000 -0500 +++ libvirt-0.8.3/tests/virt-aa-helper-test 2010-08-16 14:10:42.000000000 -0500 @@ -246,6 +246,9 @@ cat "$template_xml" | sed "s,###UUID###,$uuid,g" | sed "s,###DISK###,$disk1,g" | sed "s,</devices>,<serial type='pty'><target port='0'/></serial></devices>,g" > "$test_xml" testme "0" "serial (pty)" "-r -u $valid_uuid" "$test_xml"

+cat "$template_xml" | sed "s,###UUID###,$uuid,g" | sed "s,###DISK###,$disk1,g" | sed "s,</devices>,<serial type='dev'><source path='/dev/ttyS0'/><target port='0'/></serial></devices>,g" > "$test_xml" +testme "0" "serial (dev)" "-r -u $valid_uuid" "$test_xml" + cat "$template_xml" | sed "s,###UUID###,$uuid,g" | sed "s,###DISK###,$disk1,g" | sed "s,</devices>,<console type='file'><source path='$tmpdir/console.log'/><target port='0'/></console></devices>,g" > "$test_xml" touch "$tmpdir/console.log" testme "0" "console" "-r -u $valid_uuid" "$test_xml" @@ -253,6 +256,16 @@ cat "$template_xml" | sed "s,###UUID###,$uuid,g" | sed "s,###DISK###,$disk1,g" | sed "s,</devices>,<console type='pty'><target port='0'/></console></devices>,g" > "$test_xml" testme "0" "console (pty)" "-r -u $valid_uuid" "$test_xml"

+cat "$template_xml" | sed "s,###UUID###,$uuid,g" | sed "s,###DISK###,$disk1,g" | sed "s,</devices>,<parallel type='pty'><source path='/dev/pts/0'/><target port='0'/></parallel></devices>,g" > "$test_xml" +testme "0" "parallel (pty)" "-r -u $valid_uuid" "$test_xml" + +cat "$template_xml" | sed "s,###UUID###,$uuid,g" | sed "s,###DISK###,$disk1,g" | sed "s,</devices>,<channel type='unix'><source mode='bind' path='$tmpdir/guestfwd'/><target type='guestfwd' address='10.0.2.1' port='4600'/></channel></devices>,g" > "$test_xml" +touch "$tmpdir/guestfwd" +testme "0" "channel (unix)" "-r -u $valid_uuid" "$test_xml" + +cat "$template_xml" | sed "s,###UUID###,$uuid,g" | sed "s,###DISK###,$disk1,g" | sed "s,</devices>,<channel type='pty'><target type='virtio'/></channel></devices>,g" > "$test_xml" +testme "0" "channel (pty)" "-r -u $valid_uuid" "$test_xml" + cat "$template_xml" | sed "s,###UUID###,$uuid,g" | sed "s,###DISK###,$disk1,g" | sed "s,</os>,<kernel>$tmpdir/kernel</kernel></os>,g" > "$test_xml" touch "$tmpdir/kernel" testme "0" "kernel" "-r -u $valid_uuid" "$test_xml"

ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|