From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MvPWy-0005IY-5i for qemu-devel@nongnu.org; Wed, 07 Oct 2009 01:57:44 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MvPWs-0005Hb-T4 for qemu-devel@nongnu.org; Wed, 07 Oct 2009 01:57:43 -0400 Received: from [199.232.76.173] (port=58891 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MvPWs-0005HS-M9 for qemu-devel@nongnu.org; Wed, 07 Oct 2009 01:57:38 -0400 Received: from mx1.redhat.com ([209.132.183.28]:42837) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MvPWs-0007UC-1l for qemu-devel@nongnu.org; Wed, 07 Oct 2009 01:57:38 -0400 Message-ID: <4ACC2BE6.2080205@redhat.com> Date: Wed, 07 Oct 2009 01:49:26 -0400 From: john cooper MIME-Version: 1.0 References: <200909212039.01126.rusty@rustcorp.com.au> <4AB7A01A.3000206@redhat.com> <4AB8992B.7070709@redhat.com> <4AB8DD53.7070806@redhat.com> <4AB8DECA.3090908@redhat.com> <4AB8E88C.4040103@redhat.com> <4AB980E6.2070203@codemonkey.ws> <4AB9AA8E.7060800@third-harmonic.com> <4AC1A4AD.10509@redhat.com> <4ACA1527.9050305@third-harmonic.com> <20091005195409.GB3399@redhat.com> In-Reply-To: <20091005195409.GB3399@redhat.com> Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH] fix virtio_blk serial pci config breakage, v2 List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: john.cooper@redhat.com, Rusty Russell , qemu-devel@nongnu.org, Vadim Rozenfeld , jens.axboe@oracle.com, Avi Kivity Michael S. Tsirkin wrote: >> + put_le16(p + 0, 0x0); /* ATA device */ >> + padstr((char *)(p + 23), QEMU_VERSION, 8); /* firmware revision */ > > QEMU version is currently a string like "0.11.50" which is exactly 8 > bytes. What if someone makes it longer? padstr will not 0 > terminate string, and only partial data will be there. This code treats the field similar to the logic from which it derives (hw/ide.c) in that the field need not be nul terminated. Quiet truncation to 8 bytes can occur here and in the existing usage but in a practical sense I don't see much of a recourse. We can flag a warning but the data is realistically a best-effort attempt to provide relevant information in this field. IOW overflowing this field probably isn't justification alone to modify a too long qemu version string. > Also, identify is pre-initialized to 0, isn't it? > So just strcpy should be enough, here and elsewhere, > no need to roll our own padstr. Actually this is an oversight in the local padstr() which should be padding the balance of the field with ' ' vs. '\0'. >> + memcpy(req->elem.in_sg[0].iov_base, s->identify, >> + req->elem.in_sg[0].iov_len); > > Is this safe? Can guest make iov_len bigger than size of s->identity? Good point, a malicious/buggy guest can. The memcpy length should be capped. >> + virtio_identify_template(s); >> + strncpy((char *)&s->identify[VIRTIO_BLK_ID_SN], >> + (char *)drive_get_serial(bs), VIRTIO_BLK_ID_SN_BYTES); > > This can silently truncate the serial, can't it? Yes, it is the same disposition as ide/scsi's treatment of the S/N. My concern was of keeping the behavior consistent. Thanks, -john -- john.cooper@redhat.com