linux-kernel - Re: [PATCH v18 08/18] s390/vfio-ap: allow assignment of unavailable AP queues to mdev device

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: <97681738-50a1-976d-9f0f-be326eab7202@linux.ibm.com>
Date:   Thu, 3 Mar 2022 10:39:58 -0500
From:   "Jason J. Herne" <jjherne@...ux.ibm.com>
To:     Tony Krowiak <akrowiak@...ux.ibm.com>, linux-s390@...r.kernel.org,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org
Cc:     freude@...ux.ibm.com, borntraeger@...ibm.com, cohuck@...hat.com,
        mjrosato@...ux.ibm.com, pasic@...ux.ibm.com,
        alex.williamson@...hat.com, kwankhede@...dia.com,
        fiuczy@...ux.ibm.com
Subject: Re: [PATCH v18 08/18] s390/vfio-ap: allow assignment of unavailable
 AP queues to mdev device

On 2/14/22 19:50, Tony Krowiak wrote:
>   /**
> - * vfio_ap_mdev_verify_no_sharing - verifies that the AP matrix is not configured
> + * vfio_ap_mdev_verify_no_sharing - verify APQNs are not shared by matrix mdevs
>    *
> - * @matrix_mdev: the mediated matrix device
> + * @mdev_apm: mask indicating the APIDs of the APQNs to be verified
> + * @mdev_aqm: mask indicating the APQIs of the APQNs to be verified
>    *
> - * Verifies that the APQNs derived from the cross product of the AP adapter IDs
> - * and AP queue indexes comprising the AP matrix are not configured for another
> + * Verifies that each APQN derived from the Cartesian product of a bitmap of
> + * AP adapter IDs and AP queue indexes is not configured for any matrix
>    * mediated device. AP queue sharing is not allowed.
>    *
> - * Return: 0 if the APQNs are not shared; otherwise returns -EADDRINUSE.
> + * Return: 0 if the APQNs are not shared; otherwise return -EADDRINUSE.
>    */
> -static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev)
> +static int vfio_ap_mdev_verify_no_sharing(unsigned long *mdev_apm,
> +					  unsigned long *mdev_aqm)
>   {
> -	struct ap_matrix_mdev *lstdev;
> +	struct ap_matrix_mdev *matrix_mdev;
>   	DECLARE_BITMAP(apm, AP_DEVICES);
>   	DECLARE_BITMAP(aqm, AP_DOMAINS);
>   
> -	list_for_each_entry(lstdev, &matrix_dev->mdev_list, node) {
> -		if (matrix_mdev == lstdev)
> +	list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
> +		/*
> +		 * If the input apm and aqm belong to the matrix_mdev's matrix,
> +		 * then move on to the next.
> +		 */
> +		if (mdev_apm == matrix_mdev->matrix.apm &&
> +		    mdev_aqm == matrix_mdev->matrix.aqm)
>   			continue;

We may have a problem here. This check seems like it exists to stop you from
comparing an mdev's apm/aqm with itself. Obviously comparing an mdev's newly
updated apm/aqm with itself would cause a false positive sharing check, right?
If this is the case, I think the comment should be changed to reflect that.

Aside from the comment, what stops this particular series of if statements from
allowing us to configure a second mdev with the exact same apm/aqm values as an
existing mdev? If we do, then this check's continue will short circuit the rest
of the function thereby allowing that 2nd mdev even though it should be a
sharing violation.


-- 
-- Jason J. Herne (jjherne@...ux.ibm.com)