Re: [PATCH] tcm_vhost: Fix vhost_scsi_target structure alignment

["Nicholas A. Bellinger" <nab@xxxxxxxxxxxxxxx>]

On Sun, 2012-08-19 at 11:52 +0300, Michael S. Tsirkin wrote:
> On Sat, Aug 18, 2012 at 10:44:16PM +0000, Nicholas A. Bellinger wrote:
> > From: Michael S. Tsirkin <mst@xxxxxxxxxx>
> > 
> > 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
> > 
> > Simplest solution is probably just to make the padding
> > explicit.
> > 
> > Reported-by: Michael S. Tsirkin <mst@xxxxxxxxxx>
> > Signed-off-by: Michael S. Tsirkin <mst@xxxxxxxxxx>
> > Signed-off-by: Nicholas Bellinger <nab@xxxxxxxxxxxxxxx>
> 
> One other minor change I would do is verify that
> reserved field is set to 0, return -EOPNOTSUPP if not.
> This will make sure userspace zeroes out this field,
> and also allow future extensions to use this
> field with an easy way to test for support.
> 
> Not a must, but this is what happens for
> the padding field in struct vhost_memory,
> so it is better to be consistent I think.
> 

Makes sense to me..

Pushing the following -v2 patch into target-pending/master.

Thanks MST!

diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
index 74b2eda..1a63be4 100644
--- a/drivers/vhost/tcm_vhost.c
+++ b/drivers/vhost/tcm_vhost.c
@@ -995,11 +995,15 @@ static long vhost_scsi_ioctl(struct file *f, unsigned int ioctl,
        case VHOST_SCSI_SET_ENDPOINT:
                if (copy_from_user(&backend, argp, sizeof backend))
                        return -EFAULT;
+               if (backend.reserved != 0)
+                       return -EOPNOTSUPP;

                return vhost_scsi_set_endpoint(vs, &backend);
        case VHOST_SCSI_CLEAR_ENDPOINT:
                if (copy_from_user(&backend, argp, sizeof backend))
                        return -EFAULT;
+               if (backend.reserved != 0)
+                       return -EOPNOTSUPP;

                return vhost_scsi_clear_endpoint(vs, &backend);
        case VHOST_SCSI_GET_ABI_VERSION:
diff --git a/drivers/vhost/tcm_vhost.h b/drivers/vhost/tcm_vhost.h
index e9d5c02..d9e9355 100644
--- a/drivers/vhost/tcm_vhost.h
+++ b/drivers/vhost/tcm_vhost.h
@@ -93,6 +93,7 @@ struct vhost_scsi_target {
        int abi_version;
        char vhost_wwpn[TRANSPORT_IQN_LEN];
        unsigned short vhost_tpgt;
+       unsigned short reserved;
 };

 /* VHOST_SCSI specific defines */
-- 
1.7.2.5

--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html