linux-kernel - Re: [PATCH 2/3] iommufd: Add check on user response code

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: <b4758476-6de2-4d3e-91b2-10599a37d00e@linux.intel.com>
Date: Fri, 12 Jul 2024 10:54:11 +0800
From: Baolu Lu <baolu.lu@...ux.intel.com>
To: Jason Gunthorpe <jgg@...pe.ca>
Cc: baolu.lu@...ux.intel.com, Kevin Tian <kevin.tian@...el.com>,
 Joerg Roedel <joro@...tes.org>, Will Deacon <will@...nel.org>,
 Robin Murphy <robin.murphy@....com>, Nicolin Chen <nicolinc@...dia.com>,
 Yi Liu <yi.l.liu@...el.com>, iommu@...ts.linux.dev,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] iommufd: Add check on user response code

On 7/12/24 7:39 AM, Jason Gunthorpe wrote:
> On Wed, Jul 10, 2024 at 04:33:40PM +0800, Lu Baolu wrote:
>> The response code from user space is only allowed to be SUCCESS or
>> INVALID. All other values are treated by the device as a response
>> code of Response Failure according to PCI spec, section 10.4.2.1.
>> This response disables the Page Request Interface for the Function.
>>
>> Add a check in iommufd_fault_fops_write() to avoid invalid response
>> code.
>>
>> Fixes: 07838f7fd529 ("iommufd: Add iommufd fault object")
>> Signed-off-by: Lu Baolu<baolu.lu@...ux.intel.com>
>> ---
>>   drivers/iommu/iommufd/fault.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/iommu/iommufd/fault.c b/drivers/iommu/iommufd/fault.c
>> index 54d6cd20a673..044b9b97da31 100644
>> --- a/drivers/iommu/iommufd/fault.c
>> +++ b/drivers/iommu/iommufd/fault.c
>> @@ -305,6 +305,12 @@ static ssize_t iommufd_fault_fops_write(struct file *filep, const char __user *b
>>   		if (rc)
>>   			break;
>>   
>> +		if (response.code != IOMMUFD_PAGE_RESP_SUCCESS &&
>> +		    response.code != IOMMUFD_PAGE_RESP_INVALID) {
>> +			rc = -EINVAL;
>> +			break;
>> +		}
> 
> I added this:
> 
> 		static_assert(IOMMUFD_PAGE_RESP_SUCCESS ==
> 			      IOMMU_PAGE_RESP_SUCCESS);
> 		static_assert(IOMMUFD_PAGE_RESP_INVALID ==
> 			      IOMMU_PAGE_RESP_INVALID);

Above change cause below build warning:

drivers/iommu/iommufd/fault.c: In function ‘iommufd_fault_fops_write’:
drivers/iommu/iommufd/fault.c:308:57: warning: comparison between ‘enum 
iommufd_page_response_code’ and ‘enum iommu_page_response_code’ 
[-Wenum-compare]
   308 |                 static_assert(IOMMUFD_PAGE_RESP_SUCCESS ==
       |                                                         ^~
./include/linux/build_bug.h:78:56: note: in definition of macro 
‘__static_assert’
    78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg)
       |                                                        ^~~~
drivers/iommu/iommufd/fault.c:308:17: note: in expansion of macro 
‘static_assert’
   308 |                 static_assert(IOMMUFD_PAGE_RESP_SUCCESS ==
       |                 ^~~~~~~~~~~~~
drivers/iommu/iommufd/fault.c:310:57: warning: comparison between ‘enum 
iommufd_page_response_code’ and ‘enum iommu_page_response_code’ 
[-Wenum-compare]
   310 |                 static_assert(IOMMUFD_PAGE_RESP_INVALID ==
       |                                                         ^~
./include/linux/build_bug.h:78:56: note: in definition of macro 
‘__static_assert’
    78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg)
       |                                                        ^~~~
drivers/iommu/iommufd/fault.c:310:17: note: in expansion of macro 
‘static_assert’
   310 |                 static_assert(IOMMUFD_PAGE_RESP_INVALID ==
       |                 ^~~~~~~~~~~~~

Perhaps convert them to 'int' before compare?

diff --git a/drivers/iommu/iommufd/fault.c b/drivers/iommu/iommufd/fault.c
index 29f522819759..a643d5c7c535 100644
--- a/drivers/iommu/iommufd/fault.c
+++ b/drivers/iommu/iommufd/fault.c
@@ -305,10 +305,10 @@ static ssize_t iommufd_fault_fops_write(struct 
file *filep, const char __user *b
                 if (rc)
                         break;

-               static_assert(IOMMUFD_PAGE_RESP_SUCCESS ==
-                             IOMMU_PAGE_RESP_SUCCESS);
-               static_assert(IOMMUFD_PAGE_RESP_INVALID ==
-                             IOMMU_PAGE_RESP_INVALID);
+               static_assert((int)IOMMUFD_PAGE_RESP_SUCCESS ==
+                             (int)IOMMU_PAGE_RESP_SUCCESS);
+               static_assert((int)IOMMUFD_PAGE_RESP_INVALID ==
+                             (int)IOMMU_PAGE_RESP_INVALID);
                 if (response.code != IOMMUFD_PAGE_RESP_SUCCESS &&
                     response.code != IOMMUFD_PAGE_RESP_INVALID) {
                         rc = -EINVAL;

Thanks,
baolu