linux-kernel - Re: [PATCH v1] dmaengine: idxd: Narrow the restriction on BATCH to ver. 1 only

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

Message-ID: <87v7rmyqfg.fsf@intel.com>
Date: Wed, 02 Apr 2025 18:38:59 -0700
From: Vinicius Costa Gomes <vinicius.gomes@...el.com>
To: Fenghua Yu <fenghuay@...dia.com>, Dave Jiang <dave.jiang@...el.com>,
 Vinod Koul <vkoul@...nel.org>, dmaengine@...r.kernel.org,
 linux-kernel@...r.kernel.org
Cc: Anil Keshavamurthy <anil.s.keshavamurthy@...el.com>
Subject: Re: [PATCH v1] dmaengine: idxd: Narrow the restriction on BATCH to
 ver. 1 only

Hi Fenghua,

Fenghua Yu <fenghuay@...dia.com> writes:

> Hi, Vinicius,
>
> On 3/12/25 15:15, Vinicius Costa Gomes wrote:
>> Allow BATCH operations to be submitted and the capability to be
>> exposed for DSA version 2 (or later) devices.
>>
>> DSA version 2 devices allow safe submission of BATCH operations.
>>
>> Signed-off-by: Anil Keshavamurthy <anil.s.keshavamurthy@...el.com>
>> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@...el.com>
>> ---
>>   drivers/dma/idxd/cdev.c  | 6 ++++--
>>   drivers/dma/idxd/sysfs.c | 6 ++++--
>>   2 files changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/dma/idxd/cdev.c b/drivers/dma/idxd/cdev.c
>> index ff94ee892339..6a1dc15ee485 100644
>> --- a/drivers/dma/idxd/cdev.c
>> +++ b/drivers/dma/idxd/cdev.c
>> @@ -439,10 +439,12 @@ static int idxd_submit_user_descriptor(struct idxd_user_context *ctx,
>>   	 * DSA devices are capable of indirect ("batch") command submission.
>>   	 * On devices where direct user submissions are not safe, we cannot
>>   	 * allow this since there is no good way for us to verify these
>> -	 * indirect commands.
>> +	 * indirect commands. Narrow the restriction of operations with the
>> +	 * BATCH opcode to only DSA version 1 devices.
>>   	 */
>>   	if (is_dsa_dev(idxd_dev) && descriptor.opcode == DSA_OPCODE_BATCH &&
>> -		!wq->idxd->user_submission_safe)
>> +	    wq->idxd->hw.version == DEVICE_VERSION_1 &&
>> +	    !wq->idxd->user_submission_safe)
>>   		return -EINVAL;
>>   	/*
>>   	 * As per the programming specification, the completion address must be
>> diff --git a/drivers/dma/idxd/sysfs.c b/drivers/dma/idxd/sysfs.c
>> index 6af493f6ba77..9f0701021af0 100644
>> --- a/drivers/dma/idxd/sysfs.c
>> +++ b/drivers/dma/idxd/sysfs.c
>> @@ -1208,9 +1208,11 @@ static ssize_t op_cap_show_common(struct device *dev, char *buf, unsigned long *
>>   
>>   		/* On systems where direct user submissions are not safe, we need to clear out
>>   		 * the BATCH capability from the capability mask in sysfs since we cannot support
>> -		 * that command on such systems.
>> +		 * that command on such systems. Narrow the restriction of operations with the
>> +		 * BATCH opcode to only DSA version 1 devices.
>>   		 */
>> -		if (i == DSA_OPCODE_BATCH/64 && !confdev_to_idxd(dev)->user_submission_safe)
>> +		if (i == DSA_OPCODE_BATCH/64 && !confdev_to_idxd(dev)->user_submission_safe &&
>> +		    confdev_to_idxd(dev)->hw.version == DEVICE_VERSION_1)
>>   			clear_bit(DSA_OPCODE_BATCH % 64, &val);
>>   
>>   		pos += sysfs_emit_at(buf, pos, "%*pb", 64, &val)
>
> Maybe folder the DEVICE_VERSION_1 check into user_submission_safe variable?
>
> This way patch is a bit smaller, a bit faster in run-time,  and easier 
> to be extend in case there are other restriction changes in the future?
>
> diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
> index 86075cdc4420..80f95cb815c8 100644
> --- a/drivers/dma/idxd/init.c
> +++ b/drivers/dma/idxd/init.c
> @@ -1258,7 +1258,8 @@ int idxd_pci_probe_alloc(struct idxd_device *idxd, 
> struct pci_dev *pdev,
>                   idxd->hw.version);
>
>          if (data)
> -               idxd->user_submission_safe = data->user_submission_safe;
> +               idxd->user_submission_safe = data->user_submission_safe |
> +                                            (idxd->hw.version != 
> DEVICE_VERSION_1);
>

I don't think so, this would lift the restriction that we have on mmap()
for regular userspace applications.

The reality is that the "only" change is that the submission of
operations with the BATCH opcode is now allowed for regular applications
(via write()) with V2 devices, mmap() should still be restricted.


Cheers,
-- 
Vinicius