linux-kernel - Re: [RFC 3/3] vdpa/mlx5: take iommu_lock at dup

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJaqyWdrg1nkBSdOmZU=+Nns8UEqoyX7C+wqyk+dOPtv8UW22Q@mail.gmail.com>
Date:   Mon, 16 Jan 2023 11:39:47 +0100
From:   Eugenio Perez Martin <eperezma@...hat.com>
To:     Eli Cohen <elic@...dia.com>
Cc:     "mst@...hat.com" <mst@...hat.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Parav Pandit <parav@...dia.com>,
        "lulu@...hat.com" <lulu@...hat.com>,
        "jasowang@...hat.com" <jasowang@...hat.com>,
        "virtualization@...ts.linux-foundation.org" 
        <virtualization@...ts.linux-foundation.org>,
        "sgarzare@...hat.com" <sgarzare@...hat.com>,
        "si-wei.liu@...cle.com" <si-wei.liu@...cle.com>
Subject: Re: [RFC 3/3] vdpa/mlx5: take iommu_lock at dup_iotlb

On Mon, Jan 16, 2023 at 8:13 AM Eli Cohen <elic@...dia.com> wrote:
>
> > From: Eugenio Pérez <eperezma@...hat.com>
> > Sent: Thursday, 12 January 2023 16:22
> > To: mst@...hat.com; Eli Cohen <elic@...dia.com>
> > Cc: linux-kernel@...r.kernel.org; Parav Pandit <parav@...dia.com>;
> > lulu@...hat.com; jasowang@...hat.com; virtualization@...ts.linux-
> > foundation.org; sgarzare@...hat.com; si-wei.liu@...cle.com
> > Subject: [RFC 3/3] vdpa/mlx5: take iommu_lock at dup_iotlb
> >
> > Both iommu changes and lookup are protected by mlx5_vdpa_net->reslock at
> > this moment, but:
> > * These iotlb changes / queries are not in the fast data path.
> > * reslock belongs to netdev, while dup_iotlb seems generic.
> > * It's located in a different file than the lock it needs to hold
> >
> > Justifies the lock acquisition.
> >
>
> Following this reasoning we should take the spinlock wherever we reference an iotlb.

vring.c:iotlb_translate takes it.

> Question if it make sense that the iotlb could change while it is being referenced.
> Can you identify a specific case for this?
>

Not at this moment, because both are protected by
mlx5_vdpa_net->reslock before access or change iotlb. So this would
require changes to be exploitable, that's true.

However, to take the lock is the expected usage for vringh, so future
changes to either mlx or vringh could miss it.

Thanks!

> > Fixes: 5262912ef3cf ("vdpa/mlx5: Add support for control VQ and MAC
> > setting")
> > Signed-off-by: Eugenio Pérez <eperezma@...hat.com>
> > ---
> >  drivers/vdpa/mlx5/core/mr.c | 13 +++++++++----
> >  1 file changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/vdpa/mlx5/core/mr.c b/drivers/vdpa/mlx5/core/mr.c
> > index 878ee94efa78..e9c8a7f8ee1d 100644
> > --- a/drivers/vdpa/mlx5/core/mr.c
> > +++ b/drivers/vdpa/mlx5/core/mr.c
> > @@ -454,13 +454,15 @@ static int dup_iotlb(struct mlx5_vdpa_dev *mvdev,
> > struct vhost_iotlb *src)
> >  {
> >       struct vhost_iotlb_map *map;
> >       u64 start = 0, last = ULLONG_MAX;
> > -     int err;
> > +     int err = 0;
> > +
> > +     spin_lock(&mvdev->cvq.iommu_lock);
> >
> >       vhost_iotlb_reset(mvdev->cvq.iotlb);
> >
> >       if (!src) {
> >               err = vhost_iotlb_add_range(mvdev->cvq.iotlb, start, last,
> > start, VHOST_ACCESS_RW);
> > -             return err;
> > +             goto out;
> >       }
> >
> >       for (map = vhost_iotlb_itree_first(src, start, last); map;
> > @@ -468,9 +470,12 @@ static int dup_iotlb(struct mlx5_vdpa_dev *mvdev,
> > struct vhost_iotlb *src)
> >               err = vhost_iotlb_add_range(mvdev->cvq.iotlb, map->start,
> > map->last,
> >                                           map->addr, map->perm);
> >               if (err)
> > -                     return err;
> > +                     goto out;
> >       }
> > -     return 0;
> > +
> > +out:
> > +     spin_unlock(&mvdev->cvq.iommu_lock);
> > +     return err;
> >  }
> >
> >  static void prune_iotlb(struct mlx5_vdpa_dev *mvdev)
> > --
> > 2.31.1
>