Hi all,

While reading drivers/iommu/ I noticed two places that look
like a past the end iterator pattern. I would appreciate it
if you could take a look and let me know whether these are
real issues and whether they are worth fixing.

The first is iommu_insert_resv_region() in drivers/iommu/iommu.c
(linux-7.1-rc1, around line 873):

    list_for_each_entry(iter, regions, list) {
            if (nr->start < iter->start ||
                (nr->start == iter->start && nr->type <= iter->type))
                    break;
    }
    list_add_tail(&nr->list, &iter->list);

The second is viommu_add_resv_mem() in drivers/iommu/virtio-iommu.c
(linux-7.1-rc1, around line 523):

    list_for_each_entry(next, &vdev->resv_regions, list) {
            if (next->start > region->start)
                    break;
    }
    list_add_tail(&region->list, &next->list);

In both cases, when the loop walks all entries without break,
the iterator has gone one step past the last entry. &iter->list
then aliases the list head via container_of offset cancellation,
so the insert lands at the list tail. That is the intended
behaviour, but the access is undefined per C11.

Jakob Koschel cleaned up many such sites in 2022, for example
commits 99d8ae4ec8a (tracing: Remove usage of list iterator
variable after the loop), 2966a9918df (clockevents: Use dedicated
list iterator variable) and dc1acd5c946 (dlm: replace usage of
found with dedicated list iterator variable). These two sites
in drivers/iommu/ were not covered.

A candidate fix would track an explicit insert_before pointer
initialised to the list head and overwritten to &iter->list
only when the loop breaks early. The observable behaviour is
unchanged.

If this is intentional or already known, please disregard.
Otherwise, I am happy to send a [PATCH] or to leave the fix to
you. Thank you for your time, and sorry for the noise if this
is not actually worth fixing or has already been spotted.

Thanks,
Maoyi Xie
https://maoyixie.com/

Reply via email to