linux-kernel - Re: [GIT PULL] tcm_vhost: Initial merge of vhost level target fabric driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Sat, 18 Aug 2012 15:31:05 -0700
From:	"Nicholas A. Bellinger" <nab@...ux-iscsi.org>
To:	"Michael S. Tsirkin" <mst@...hat.com>
Cc:	target-devel <target-devel@...r.kernel.org>,
	kvm-devel <kvm@...r.kernel.org>,
	linux-scsi <linux-scsi@...r.kernel.org>,
	qemu-devel <qemu-devel@...gnu.org>,
	Stefan Hajnoczi <stefanha@...il.com>,
	Anthony Liguori <anthony@...emonkey.ws>,
	Paolo Bonzini <pbonzini@...hat.com>,
	Christoph Hellwig <hch@....de>, Hannes Reinecke <hare@...e.de>,
	Zhi Yong Wu <wuzhy@...ibm.com>,
	LKML <linux-kernel@...r.kernel.org>,
	lf-virt <virtualization@...ts.linux-foundation.org>,
	Jens Axboe <axboe@...nel.dk>
Subject: Re: [GIT PULL] tcm_vhost: Initial merge of vhost level target
 fabric driver

On Sat, 2012-08-18 at 23:04 +0300, Michael S. Tsirkin wrote:
> Hi Nicholas,
> I just noticed this problem in the interface:
> 
> +#include <linux/vhost.h>
> +
> +/*
> + * Used by QEMU userspace to ensure a consistent vhost-scsi ABI.
> + *
> + * ABI Rev 0: July 2012 version starting point for v3.6-rc merge
> candidate +
> + *            RFC-v2 vhost-scsi userspace.  Add GET_ABI_VERSION ioctl
> usage
> + */
> +
> +#define VHOST_SCSI_ABI_VERSION 0
> +
> +struct vhost_scsi_target {
> +       int abi_version;
> +       unsigned char vhost_wwpn[TRANSPORT_IQN_LEN];
> +       unsigned short vhost_tpgt;
> +};
> +
> 
> Here TRANSPORT_IQN_LEN is 224, which is a multiple of 4.
> Since vhost_tpgt is 2 bytes and abi_version is 4, the total size would
> be 230.  But gcc needs struct size be aligned to first field size, which
> is 4 bytes, so it pads the structure by extra 2 bytes to the total of
> 232.
> 
> This padding is very undesirable in an ABI:
> - it can not be initialized easily
> - it can not be checked easily
> - it can leak information between kernel and userspace
> 

Hmmmm, yes.  Very good reasons to avoid ABI ambiguity  ..

> Simplest solution is probably just to make the padding
> explicit:
> 
> +struct vhost_scsi_target {
> +       int abi_version;
> +       unsigned char vhost_wwpn[TRANSPORT_IQN_LEN];
> +       unsigned short vhost_tpgt;
> +       unsigned short reserved;
> +};
> +
> 
> I think we should fix this buglet before it goes out to users.
> 

<nod>, fixing this up in target-pending/master now w/ your reported-by
+signoff, and will change vhost-scsi's copy of these defs for next
week's RFC-v3 posting.

Thanks MST!

--nab

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/