[PATCH] drivers/vfio: Fix a redundant copy bug

[PATCH] drivers/vfio: Fix a redundant copy bug

[Wenwen Wang]

In vfio_spapr_iommu_eeh_ioctl(), if the ioctl command is VFIO_EEH_PE_OP,
the user-space buffer 'arg' is copied to the kernel object 'op' and the
'argsz' and 'flags' fields of 'op' are checked. If the check fails, an
error code EINVAL is returned. Otherwise, 'op.op' is further checked
through a switch statement to invoke related handlers. If 'op.op' is
VFIO_EEH_PE_INJECT_ERR, the whole user-space buffer 'arg' is copied again
to 'op' to obtain the err information. However, in the following execution
of this case, the fields of 'op', except the field 'err', are actually not
used. That is, the second copy has a redundant part. Therefore, for both
performance and security reasons, the redundant part of the second copy
should be removed.

This patch removes such a part in the second copy. It only copies the 'err'
information from the buffer 'arg'.

Signed-off-by: Wenwen Wang <wang6495@umn.edu>
---
 drivers/vfio/vfio_spapr_eeh.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/vfio/vfio_spapr_eeh.c b/drivers/vfio/vfio_spapr_eeh.c
index 38edeb4..5bc4b60 100644
--- a/drivers/vfio/vfio_spapr_eeh.c
+++ b/drivers/vfio/vfio_spapr_eeh.c
@@ -86,10 +86,10 @@ long vfio_spapr_iommu_eeh_ioctl(struct iommu_group *group,
 			ret = eeh_pe_configure(pe);
 			break;
 		case VFIO_EEH_PE_INJECT_ERR:
-			minsz = offsetofend(struct vfio_eeh_pe_op, err.mask);
-			if (op.argsz < minsz)
+			if (op.argsz < sizeof(op))
 				return -EINVAL;
-			if (copy_from_user(&op, (void __user *)arg, minsz))
+			if (copy_from_user(&op.err, (char __user *)arg +
+						minsz, sizeof(op.err)))
 				return -EFAULT;
 
 			ret = eeh_pe_inject_err(pe, op.err.type, op.err.func,
-- 
2.7.4

Re: [PATCH] drivers/vfio: Fix a redundant copy bug

[Alex Williamson]

Hi,

On Sun,  7 Oct 2018 09:44:25 -0500
Wenwen Wang <wang6495@umn.edu> wrote:

> In vfio_spapr_iommu_eeh_ioctl(), if the ioctl command is VFIO_EEH_PE_OP,
> the user-space buffer 'arg' is copied to the kernel object 'op' and the
> 'argsz' and 'flags' fields of 'op' are checked. If the check fails, an
> error code EINVAL is returned. Otherwise, 'op.op' is further checked
> through a switch statement to invoke related handlers. If 'op.op' is
> VFIO_EEH_PE_INJECT_ERR, the whole user-space buffer 'arg' is copied again
> to 'op' to obtain the err information. However, in the following execution
> of this case, the fields of 'op', except the field 'err', are actually not
> used. That is, the second copy has a redundant part. Therefore, for both
> performance and security reasons, the redundant part of the second copy
> should be removed.

Redundant, yes.  Performance-wise it's 12 bytes on a non-performance
path, so theoretically yes, but in practice maybe it's a simplicity
trade-off.  Security?  I don't see it, please explain.

> This patch removes such a part in the second copy. It only copies the 'err'
> information from the buffer 'arg'.
> 
> Signed-off-by: Wenwen Wang <wang6495@umn.edu>
> ---
>  drivers/vfio/vfio_spapr_eeh.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/vfio/vfio_spapr_eeh.c b/drivers/vfio/vfio_spapr_eeh.c
> index 38edeb4..5bc4b60 100644
> --- a/drivers/vfio/vfio_spapr_eeh.c
> +++ b/drivers/vfio/vfio_spapr_eeh.c
> @@ -86,10 +86,10 @@ long vfio_spapr_iommu_eeh_ioctl(struct iommu_group *group,
>  			ret = eeh_pe_configure(pe);
>  			break;
>  		case VFIO_EEH_PE_INJECT_ERR:
> -			minsz = offsetofend(struct vfio_eeh_pe_op, err.mask);
> -			if (op.argsz < minsz)
> +			if (op.argsz < sizeof(op))
>  				return -EINVAL;

The original code is written such that new operations can be added,
possibly with new entries in the struct vfio_eeh_pe_op union, which
might change sizeof(op) to be more than necessary for a
VFIO_EEH_PE_INJECT_ERR op.  Existing userspace suddenly wouldn't work
without effectively reverting this change.  This is a subtle dependency
that is not worth the above code change, imo.

> -			if (copy_from_user(&op, (void __user *)arg, minsz))
> +			if (copy_from_user(&op.err, (char __user *)arg +
> +						minsz, sizeof(op.err)))
>  				return -EFAULT;

Please rework with the assumption that the union in struct
vfio_eeh_pe_op can be expanded and must not break existing userspace.
Thanks,

Alex

Re: [PATCH] drivers/vfio: Fix a redundant copy bug

[Wenwen Wang]

On Mon, Oct 8, 2018 at 11:43 AM Alex Williamson
<alex.williamson@redhat.com> wrote:
>
> Hi,
>
> On Sun,  7 Oct 2018 09:44:25 -0500
> Wenwen Wang <wang6495@umn.edu> wrote:
>
> > In vfio_spapr_iommu_eeh_ioctl(), if the ioctl command is VFIO_EEH_PE_OP,
> > the user-space buffer 'arg' is copied to the kernel object 'op' and the
> > 'argsz' and 'flags' fields of 'op' are checked. If the check fails, an
> > error code EINVAL is returned. Otherwise, 'op.op' is further checked
> > through a switch statement to invoke related handlers. If 'op.op' is
> > VFIO_EEH_PE_INJECT_ERR, the whole user-space buffer 'arg' is copied again
> > to 'op' to obtain the err information. However, in the following execution
> > of this case, the fields of 'op', except the field 'err', are actually not
> > used. That is, the second copy has a redundant part. Therefore, for both
> > performance and security reasons, the redundant part of the second copy
> > should be removed.
>
> Redundant, yes.  Performance-wise it's 12 bytes on a non-performance
> path, so theoretically yes, but in practice maybe it's a simplicity
> trade-off.  Security?  I don't see it, please explain.
>
> > This patch removes such a part in the second copy. It only copies the 'err'
> > information from the buffer 'arg'.
> >
> > Signed-off-by: Wenwen Wang <wang6495@umn.edu>
> > ---
> >  drivers/vfio/vfio_spapr_eeh.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/vfio/vfio_spapr_eeh.c b/drivers/vfio/vfio_spapr_eeh.c
> > index 38edeb4..5bc4b60 100644
> > --- a/drivers/vfio/vfio_spapr_eeh.c
> > +++ b/drivers/vfio/vfio_spapr_eeh.c
> > @@ -86,10 +86,10 @@ long vfio_spapr_iommu_eeh_ioctl(struct iommu_group *group,
> >                       ret = eeh_pe_configure(pe);
> >                       break;
> >               case VFIO_EEH_PE_INJECT_ERR:
> > -                     minsz = offsetofend(struct vfio_eeh_pe_op, err.mask);
> > -                     if (op.argsz < minsz)
> > +                     if (op.argsz < sizeof(op))
> >                               return -EINVAL;
>
> The original code is written such that new operations can be added,
> possibly with new entries in the struct vfio_eeh_pe_op union, which
> might change sizeof(op) to be more than necessary for a
> VFIO_EEH_PE_INJECT_ERR op.  Existing userspace suddenly wouldn't work
> without effectively reverting this change.  This is a subtle dependency
> that is not worth the above code change, imo.
>
> > -                     if (copy_from_user(&op, (void __user *)arg, minsz))
> > +                     if (copy_from_user(&op.err, (char __user *)arg +
> > +                                             minsz, sizeof(op.err)))
> >                               return -EFAULT;
>
> Please rework with the assumption that the union in struct
> vfio_eeh_pe_op can be expanded and must not break existing userspace.

Thanks for your suggestion. I will resubmit the patch.

Wenwen

> Thanks,
>
> Alex