Re: [Intel-wired-lan] [PATCH v2] dpf: fix UAF and double free in idpf_plug_vport_aux_dev() error path

[Jacob Keller <jacob.e.keller@intel.com>]

On 4/14/2026 6:47 PM, Guangshuo Li wrote:
> Hi Jacob,
> 
> Thanks for reviewing.
> 
> On Wed, 15 Apr 2026 at 05:03, Jacob Keller <jacob.e.keller@intel.com> wrote:
>>
>>
>> This doesn't look right. The commit message analysis seems to match this
>> fix from Greg KH:
>>
>> https://lore.kernel.org/intel-wired-lan/2026041432-tapestry-condition-22ff@gregkh/
>>
>> But the changes do not make any sense to me. It looks like a poorly done
>> AI-generated "fix" which is not correct. Greg's version does look like
>> it properly resolves this.
>>
>>> v2:
>>>   - note that the issue was identified by my static analysis tool
>>>   - and confirmed by manual review
>>>
>>
>> What even is this change log?? I see that version was sent and everyone
>> else was sane enough to just silently reject or ignore the v1...
>>
>>>  drivers/net/ethernet/intel/idpf/idpf_idc.c | 6 +++++-
>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/idpf/idpf_idc.c b/drivers/net/ethernet/intel/idpf/idpf_idc.c
>>> index 6dad0593f7f2..2a18907643fc 100644
>>> --- a/drivers/net/ethernet/intel/idpf/idpf_idc.c
>>> +++ b/drivers/net/ethernet/intel/idpf/idpf_idc.c
>>> @@ -59,6 +59,7 @@ static int idpf_plug_vport_aux_dev(struct iidc_rdma_core_dev_info *cdev_info,
>>>       char name[IDPF_IDC_MAX_ADEV_NAME_LEN];
>>>       struct auxiliary_device *adev;
>>>       int ret;
>>> +     int adev_id;
>>>
>>
>> You create a local variable here...
>>
>>>       iadev = kzalloc(sizeof(*iadev), GFP_KERNEL);
>>>       if (!iadev)
>>> @@ -74,11 +75,14 @@ static int idpf_plug_vport_aux_dev(struct iidc_rdma_core_dev_info *cdev_info,
>>>               goto err_ida_alloc;
>>>       }
>>>       adev->id = ret;
>>> +     adev->id = adev_id;
>>
>> adev_is is never initialized, so you assign a random garbage
>> uninitialized value. This is obviously wrong and will lead to worse
>> errors than the failed cleanup.
>>
>> I'm rejecting this patch in favor of the clearly appropriate fix from Greg.
>>
>>>       adev->dev.release = idpf_vport_adev_release;
>>>       adev->dev.parent = &cdev_info->pdev->dev;
>>>       sprintf(name, "%04x.rdma.vdev", cdev_info->pdev->vendor);
>>>       adev->name = name;
>>>
>>> +     /* iadev is owned by the auxiliary device */
>>> +     iadev = NULL;>          ret = auxiliary_device_init(adev);
>>>       if (ret)
>>>               goto err_aux_dev_init;
>>> @@ -92,7 +96,7 @@ static int idpf_plug_vport_aux_dev(struct iidc_rdma_core_dev_info *cdev_info,
>>>  err_aux_dev_add:
>>>       auxiliary_device_uninit(adev);
>>>  err_aux_dev_init:
>>> -     ida_free(&idpf_idc_ida, adev->id);
>>> +     ida_free(&idpf_idc_ida, adev_id);
>>>  err_ida_alloc:
>>>       vdev_info->adev = NULL;
>>>       kfree(iadev);
>>
> 
> You are right that the v2 patch as sent is incomplete. That was my
> mistake when preparing/sending v2: it accidentally dropped the adev_id
> = ret; assignment, which made that version incorrect.
> 
> For reference, the original v1 patch is here:
> 
> https://lkml.org/lkml/2026/3/21/421
> 
> In v1, adev_id was assigned from ret before use, so I believe that
> particular uninitialized-variable issue was introduced in the v2
> posting.
> 
> Sorry for the confusion caused by the broken v2 posting.

No problem. I had missed the other version, which explains my confusion.
Still, to my eyes, the fix looks to be an equivalent fix as one
submitted by GregKH:

https://lore.kernel.org/intel-wired-lan/2026041116-retail-bagginess-250f@gregkh/

Do you agree this is effectively a different fix for the same problem?
Or is there really two different double-free issues here that both need
patching? I haven't been able to fully convince my self either way, but
I am leaning on this being one problem, and I think Gregs solution feels
simpler to understand.

Thanks,
Jake

> 
> Thanks,
> Guangshuo