linux-kernel - Re: [PATCH] iommufd/device: Enforce reserved IOVA also when attached to hwpt

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: <ZrEQt5r978B+Pgex@Asurada-Nvidia>
Date: Mon, 5 Aug 2024 11:03:41 -0700
From: Nicolin Chen <nicolinc@...dia.com>
To: "Tian, Kevin" <kevin.tian@...el.com>
CC: "jgg@...dia.com" <jgg@...dia.com>, "Liu, Yi L" <yi.l.liu@...el.com>,
	"iommu@...ts.linux.dev" <iommu@...ts.linux.dev>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] iommufd/device: Enforce reserved IOVA also when attached
 to hwpt_nested

On Mon, Aug 05, 2024 at 07:40:45AM +0000, Tian, Kevin wrote:
> > From: Nicolin Chen <nicolinc@...dia.com>
> > Sent: Friday, August 2, 2024 1:35 PM
> >
> >  static int
> > -iommufd_group_do_replace_paging(struct iommufd_group *igroup,
> > -                             struct iommufd_hwpt_paging *hwpt_paging)
> > +iommufd_group_do_replace_reserved_iova(struct iommufd_group *igroup,
> > +                                    struct iommufd_hw_pagetable *hwpt)
> >  {
> > +     struct iommufd_hwpt_paging *hwpt_paging =
> > to_hwpt_paging(hwpt);
> >       struct iommufd_hw_pagetable *old_hwpt = igroup->hwpt;
> >       struct iommufd_device *cur;
> >       int rc;
> >
> >       lockdep_assert_held(&igroup->lock);
> >
> > -     if (!hwpt_is_paging(old_hwpt) ||
> > -         hwpt_paging->ioas != to_hwpt_paging(old_hwpt)->ioas) {
> > +     if (!hwpt_paging)
> > +             return 0;
> > +
> > +     if (iommufd_hw_pagetable_compare_ioas(old_hwpt, hwpt)) {
> 
> hmm this change is broken. In this helper:
> 
>         if (!old_hwpt_paging || !new_hwpt_paging)
>                 return false;
>         return old_hwpt_paging->ioas != new_hwpt_paging->ioas;
> 
> Obviously the original code wants to enforce reserved regions if
> new_hwpt is paging && old_hwpt is not paging, but this change
> skips this scenario.

Hmm..I think that is the intention of this patch?

The original code does that because it didn't enforce reserved
region (to the parent paging hwpt) when attaching the group to
a nested one. Now, it does. So, we basically check whether the
associated ioas has changed or not. Right?

Perhaps I should have added a line of comments to highlight it
getting the parent hwpt pointer now for a nested hwpt.

> >
> >       rc = iommufd_hwpt_replace_device(idev, hwpt, old_hwpt);
> >       if (rc)
> >               goto err_unresv;
> >
> > -     if (hwpt_is_paging(old_hwpt) &&
> > -         (!hwpt_is_paging(hwpt) ||
> > -          to_hwpt_paging(hwpt)->ioas != to_hwpt_paging(old_hwpt)->ioas))
> > -             iommufd_group_remove_reserved_iova(igroup,
> > -                                                to_hwpt_paging(old_hwpt));
> > +     if (iommufd_hw_pagetable_compare_ioas(old_hwpt, hwpt))
> > +             iommufd_group_remove_reserved_iova(igroup, old_hwpt);
> 
> this is also broken.
> 
> Probably it's clearer to continue open-coding those conditions in
> iommufd_group_do_replace_reserved_iova() and
> iommufd_group_remove_reserved_iova().

Here it basically compares the ioas too. The original code too,
while having an additional hwpt_is_paging() check on both hwpts
because of the same reason.

Thanks
Nicolin