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: <5111574CD4361AE7+ecedd63e-e04a-4ada-be16-0e290498c7c4@uniontech.com>
Date: Fri, 25 Oct 2024 19:14:18 +0800
From: 缑同琛 <goutongchen@...ontech.com>
To: Robin Murphy <robin.murphy@....com>,
 linux-arm-kernel@...ts.infradead.org, iommu@...ts.linux.dev,
 linux-kernel@...r.kernel.org, Will Deacon <will@...nel.org>,
 Joerg Roedel <joro@...tes.org>, Jason Gunthorpe <jgg@...pe.ca>,
 Rob Clark <robdclark@...omium.org>,
 Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Cc: 苟浩 <gouhao@...ontech.com>
Subject: Re: [PATCH] iommu/arm-smmu: Add judgment on the size and granule
 parameters passed in


在 2024/10/24 19:59, Robin Murphy 写道:
> On 24/10/2024 11:02 am, goutongchen wrote:
>> In the arm_smmu_tlb_inv_range_s1 and arm_smmu_tlb_inv_range_s2
>> functions, the size and granule passed in must be judged.
>> It must be ensured that the passed in parameter is not 0 and
>> the size is an integer multiple of the granule, otherwise it
>> will cause an infinite while loop.
>>
>> This was encountered during testing, and was initially triggered
>> by passing in a size value of 0, causing the kernel to crash.
>>
>> [    8.214378][  T675] xhci_hcd 0000:0b:00.0: new USB bus registered, 
>> assigned bus number 1
>> [   68.246185][    C0] rcu: INFO: rcu_sched self-detected stall on CPU
>> [   68.246866][    C0] rcu:     0-....: (5999 ticks this GP) 
>> idle=796c/1/0x4000000000000000 softirq=161/161 fqs=2999
>> [   68.247924][    C0] rcu:     (t=6000 jiffies g=-699 q=1 ncpus=128)
>> [   68.248452][    C0] CPU: 0 PID: 675 Comm: kworker/0:2 Not tainted 
>> 6.6.0-25.02.2500.002.uos25.aarch64 #1
>> [   68.249237][    C0] Hardware name: Inspur     CS5260F 
>> /CS5260F          , BIOS 4.0.16 05/31/22 16:53:51
>> [   68.250029][    C0] Workqueue: events work_for_cpu_fn
>> [   68.250497][    C0] pstate: a0000005 (NzCv daif -PAN -UAO -TCO 
>> -DIT -SSBS BTYPE=--)
>> [   68.251188][    C0] pc : arm_smmu_tlb_inv_range_s1+0xf0/0x158
>> [   68.251704][    C0] lr : arm_smmu_tlb_inv_walk_s1+0x44/0x68
>> [   68.252189][    C0] sp : ffff80008044b780
>> [   68.252530][    C0] x29: ffff80008044b780 x28: 0000000000000000 
>> x27: 0000000000001000
>> [   68.253212][    C0] x26: 0000000000000600 x25: 0000000000000001 
>> x24: 0000000000000600
>> [   68.253857][    C0] x23: 0000000000000000 x22: 0000000000001000 
>> x21: fffffc64e2e59000
>> [   68.254544][    C0] x20: 0000000039c1d1a6 x19: ffff3fe944c40280 
>> x18: 0000000000000000
>> [   68.255240][    C0] x17: 626d756e20737562 x16: ffffb0e7e08c1008 
>> x15: 0000000000000000
>> [   68.255930][    C0] x14: ffff3ef8c1ea15cd x13: ffff3ef8c1ea15cb 
>> x12: fffffcfba30e3880
>> [   68.256538][    C0] x11: 00000000ffff7fff x10: ffff80008044b6b0 x9 
>> : ffffb0e7decd1b5c
>> [   68.257126][    C0] x8 : 0000000000000dc0 x7 : ffff3ee8c4148000 x6 
>> : ffff3ee8c4148000
>> [   68.257822][    C0] x5 : 0000000000000008 x4 : 0000000000000000 x3 
>> : ffff3fe944c40800
>> [   68.258497][    C0] x2 : 0000000000000010 x1 : 0000000000000020 x0 
>> : ffffb0e7dfd6c3d0
>> [   68.259185][    C0] Call trace:
>> [   68.259451][    C0]  arm_smmu_tlb_inv_range_s1+0xf0/0x158
>> [   68.259933][    C0]  arm_smmu_tlb_inv_walk_s1+0x44/0x68
>> [   68.260384][    C0]  __arm_lpae_map+0x1f0/0x2c0
>
> Huh? This invalidation path is for mapping a block entry, and the size 
> is the size of that block per ARM_LPAE_BLOCK_SIZE(lvl, data) - how can 
> it possibly be 0?
>
> Thanks,
> Robin.
>

Thank you for your reply. First of all, as you said, under normal 
circumstances, the size will not be 0, but I think it is better to limit 
it here.

Because there is no guarantee that the parameter value passed in meets 
the requirements, if a value that does not meet the requirements is set 
manually during the call, it will fall into an infinite loop and cause 
the kernel to fail to start, which is very fatal.

It is precisely because we actually encountered such an error and this 
problem that we submitted this patch to the upstream.


Thanks,

goutongchen.


>> [   68.260796][    C0] arm_lpae_map_pages+0xec/0x150
>> [   68.261215][    C0]  arm_smmu_map_pages+0x48/0x130
>> [   68.261654][    C0]  __iommu_map+0x134/0x2a8
>> [   68.262098][    C0]  iommu_map_sg+0xb8/0x1c8
>> [   68.262500][    C0] 
>> __iommu_dma_alloc_noncontiguous.constprop.0+0x180/0x270
>> [   68.263145][    C0]  iommu_dma_alloc+0x178/0x238
>> [   68.263557][    C0]  dma_alloc_attrs+0xf8/0x108
>> [   68.263962][    C0]  xhci_mem_init+0x1e8/0x6d0
>> [   68.264372][    C0]  xhci_init+0x88/0x1d0
>> [   68.264736][    C0]  xhci_gen_setup+0x284/0x468
>> [   68.265121][    C0]  xhci_pci_setup+0x60/0x1f8
>> [   68.265506][    C0]  usb_add_hcd+0x278/0x650
>> [   68.265860][    C0]  usb_hcd_pci_probe+0x218/0x458
>> [   68.266256][    C0]  xhci_pci_probe+0x7c/0x270
>> [   68.266660][    C0]  local_pci_probe+0x48/0xb8
>> [   68.267074][    C0]  work_for_cpu_fn+0x24/0x40
>> [   68.267548][    C0]  process_one_work+0x170/0x3c0
>> [   68.267999][    C0]  worker_thread+0x234/0x3b8
>> [   68.268383][    C0]  kthread+0xf0/0x108
>> [   68.268704][    C0]  ret_from_fork+0x10/0x20
>>
>> Signed-off-by: goutongchen <goutongchen@...ontech.com>
>> ---
>>   drivers/iommu/arm/arm-smmu/arm-smmu.c | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c 
>> b/drivers/iommu/arm/arm-smmu/arm-smmu.c
>> index 8321962b3714..16b2e9ec0e60 100644
>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
>> @@ -282,6 +282,13 @@ static void arm_smmu_tlb_inv_range_s1(unsigned 
>> long iova, size_t size,
>>       struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
>>       int idx = cfg->cbndx;
>>   +    if (size == 0 || granule == 0 || (size % granule) != 0) {
>> +        dev_err(smmu->dev,
>> +                 "The size or granule passed in is err. size=%lu, 
>> granule=%lu\n",
>> +                 size, granule);
>> +        return;
>> +    }
>> +
>>       if (smmu->features & ARM_SMMU_FEAT_COHERENT_WALK)
>>           wmb();
>>   @@ -309,6 +316,13 @@ static void arm_smmu_tlb_inv_range_s2(unsigned 
>> long iova, size_t size,
>>       struct arm_smmu_device *smmu = smmu_domain->smmu;
>>       int idx = smmu_domain->cfg.cbndx;
>>   +    if (size == 0 || granule == 0 || (size % granule) != 0) {
>> +        dev_err(smmu->dev,
>> +                 "The size or granule passed in is err. size=%lu, 
>> granule=%lu\n",
>> +                 size, granule);
>> +        return;
>> +    }
>> +
>>       if (smmu->features & ARM_SMMU_FEAT_COHERENT_WALK)
>>           wmb();
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ