linux-kernel - Re: [PATCH v6 04/10] s390: vfio-ap: filter CRYCB bits for unavailable queue devices

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: <a587f900-352b-ae82-0c86-6c0fb173315e@linux.ibm.com>
Date:   Fri, 20 Sep 2019 11:44:05 -0400
From:   Tony Krowiak <akrowiak@...ux.ibm.com>
To:     Halil Pasic <pasic@...ux.ibm.com>
Cc:     linux-s390@...r.kernel.org, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org, freude@...ux.ibm.com, borntraeger@...ibm.com,
        cohuck@...hat.com, mjrosato@...ux.ibm.com, pmorel@...ux.ibm.com,
        alex.williamson@...hat.com, kwankhede@...dia.com,
        jjherne@...ux.ibm.com
Subject: Re: [PATCH v6 04/10] s390: vfio-ap: filter CRYCB bits for unavailable
 queue devices

On 9/20/19 10:24 AM, Tony Krowiak wrote:
> On 9/19/19 6:34 AM, Halil Pasic wrote:
>> On Fri, 13 Sep 2019 17:26:52 -0400
>> Tony Krowiak <akrowiak@...ux.ibm.com> wrote:
>>
>>> +static void vfio_ap_mdev_get_crycb_matrix(struct ap_matrix_mdev 
>>> *matrix_mdev)
>>> +{
>>> +    unsigned long apid, apqi;
>>> +    unsigned long masksz = BITS_TO_LONGS(AP_DEVICES) *
>>> +                   sizeof(unsigned long);
>>> +
>>> +    memset(matrix_mdev->crycb.apm, 0, masksz);
>>> +    memset(matrix_mdev->crycb.apm, 0, masksz);
>>> +    memcpy(matrix_mdev->crycb.adm, matrix_mdev->matrix.adm, masksz);
>>> +
>>> +    for_each_set_bit_inv(apid, matrix_mdev->matrix.apm,
>>> +                 matrix_mdev->matrix.apm_max + 1) {
>>> +        for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm,
>>> +                     matrix_mdev->matrix.aqm_max + 1) {
>>> +            if (vfio_ap_find_queue(AP_MKQID(apid, apqi))) {
>>> +                if (!test_bit_inv(apid, matrix_mdev->crycb.apm))
>>> +                    set_bit_inv(apid,
>>> +                            matrix_mdev->crycb.apm);
>>> +                if (!test_bit_inv(apqi, matrix_mdev->crycb.aqm))
>>> +                    set_bit_inv(apqi,
>>> +                            matrix_mdev->crycb.aqm);
>>> +            }
>>> +        }
>>> +    }
>>> +}
>>
>> Even with the discussed typo fixed (zero crycb.aqm) this procedure does
>> not make sense to me. :(
>>
>> If in doubt please consider the following example:
>> matrix_mdev->matrix.apm and matrix_mdev->matrix.aqm have both just bits
>> 0 and 1 set (i.e. first byte 0xC0 the rest of the bytes 0x0). Queues
>> bound to the vfio_ap driver (0,0), (0,1), (1,0); not bound to vfio_ap is
>> however (1,1). If I read this correctly this filtering logic would grant
>> access to (1,1) which seems to contradict with the stated intention.
> 
> Yep, I see your point. I'll have to rework this code.

As I see it, we have two choices here:

1. Do not set bit 1 in the APM of the guest's CRYCB because queue
    01.0001 is not bound to the vfio_ap device driver. This would
    preclude guest access to any domain in adapter 1 - i.e., the
    guest would have access to queues 00.0000 and 00.0001.

2. Do not set bit 1 in the AQM of the guest's CRYCB because queue
    01.0001 is not bound to the vfio_ap device driver. This would
    preclude guest access to domain 1 in both adapters - i.e., the
    guest would have access to queues 00.0000 and 01.0000.

There are ramifications for either choice. For example, if only one
adapter is assigned to the mdev, then option 1 will result in the
guest not having access to any AP queues. Likewise, the guest will
not get access to any AP queues if only one domain is assigned to
the mdev. Neither choice is optimal, but option 1 seems to make sense
because it somewhat models the behavior of the host system. For example,
only AP adapters can be configured online/offline and in order to
add/remove domains, an adapter must first be configured offline.

> 
>>
>> Regards,
>> Halil
>>
>>
>>
>