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

Jacob Keller
2026-04-15T05:37:43Z

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