Re: [Linux Kernel Bug] KASAN: null-ptr-deref Read in generic_hwtstamp_ioctl_lower

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

On 29/10/2025 16:47, Kory Maincent wrote:
> On Wed, 29 Oct 2025 18:19:34 +0200
> Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
> 
>> On Wed, Oct 29, 2025 at 11:06:51AM +0100, Kory Maincent wrote:
>>> Hello Jiaming,
>>>
>>> +Vlad
>>>
>>> On Wed, 29 Oct 2025 16:45:37 +0800
>>> Jiaming Zhang <r772577952@gmail.com> wrote:
>>>    
>>>> Dear Linux kernel developers and maintainers,
>>>>
>>>> We are writing to report a null pointer dereference bug discovered in
>>>> the net subsystem. This bug is reproducible on the latest version
>>>> (v6.18-rc3, commit dcb6fa37fd7bc9c3d2b066329b0d27dedf8becaa).
>>>>
>>>> The root cause is in tsconfig_prepare_data(), where a local
>>>> kernel_hwtstamp_config struct (cfg) is initialized using {}, setting
>>>> all its members to zero. Consequently, cfg.ifr becomes NULL.
>>>>
>>>> cfg is then passed as: tsconfig_prepare_data() ->
>>>> dev_get_hwtstamp_phylib() -> vlan_hwtstamp_get() (via
>>>> dev->netdev_ops->ndo_hwtstamp_get) -> generic_hwtstamp_get_lower() ->
>>>> generic_hwtstamp_ioctl_lower().
>>>>
>>>> The function generic_hwtstamp_ioctl_lower() assumes cfg->ifr is a
>>>> valid pointer and attempts to access cfg->ifr->ifr_ifru. This access
>>>> dereferences the NULL pointer, triggering the bug.
>>>
>>> Thanks for spotting this issue!
>>>
>>> In the ideal world we would have all Ethernet driver supporting the
>>> hwtstamp_get/set NDOs but that not currently the case.	
>>> Vladimir Oltean was working on this but it is not done yet.
>>> $ git grep SIOCGHWTSTAMP drivers/net/ethernet | wc -l
>>> 16
>>
>> Vadim also took the initiative and submitted (is still submitting?) some
>> more conversions, whereas I lost all steam.
> 
> Ok no worry I was simply pointing this out, people will convert it when they
> want to use the new netlink API.
>   
>>>> As a potential fix, we can declare a local struct ifreq variable in
>>>> tsconfig_prepare_data(), zero-initializing it, and then assigning its
>>>> address to cfg.ifr before calling dev_get_hwtstamp_phylib(). This
>>>> ensures that functions down the call chain receive a valid pointer.
>>>
>>> If we do that we will have legacy IOCTL path inside the Netlink path and
>>> that's not something we want.
>>> In fact it is possible because the drivers calling
>>> generic_hwtstamp_get/set_lower functions are already converted to hwtstamp
>>> NDOs therefore the NDO check in tsconfig_prepare_data is not working on
>>> these case.
>>
>> I remember we had this discussion before.
>>
>> | This is why I mentioned by ndo_hwtstamp_set() conversion, because
>> | suddenly it is a prerequisite for any further progress to be done.
>> | You can't convert SIOCSHWTSTAMP to netlink if there are some driver
>> | implementations which still use ndo_eth_ioctl(). They need to be
>> | UAPI-agnostic.
>>
>> https://lore.kernel.org/netdev/20231122140850.li2mvf6tpo3f2fhh@skbuf/
>>
>> I'm not sure what was your agreement with the netdev maintainer
>> accepting the tsconfig netlink work with unconverted device drivers left
>> in the tree.
> 
> I did like 21th versions and there was not many people active in the reviews.
> No one stand against this work.
> 
>>> IMO the solution is to add a check on the ifr value in the
>>> generic_hwtstamp_set/get_lower functions like that:
>>>
>>> int generic_hwtstamp_set_lower(struct net_device *dev,
>>> 			       struct kernel_hwtstamp_config *kernel_cfg,
>>> 			       struct netlink_ext_ack *extack)
>>> {
>>> ...
>>>
>>> 	/* Netlink path with unconverted lower driver */
>>> 	if (!kernel_cfg->ifr)
>>> 		return -EOPNOTSUPP;
>>>
>>> 	/* Legacy path: unconverted lower driver */
>>> 	return generic_hwtstamp_ioctl_lower(dev, SIOCSHWTSTAMP, kernel_cfg);
>>> }
>>
>> This plugs one hole (two including _get). How many more are there? If
>> this is an oversight, the entire tree needs to be reviewed for
>> ndo_hwtstamp_get() / ndo_hwtstamp_test() pointer tests which were used
>> as an indication that this net device is netlink ready. Stacked
>> virtual interfaces are netlink-ready only when the entire chain down to
>> the physical interface is netlink-ready.
> 
> I don't see this as a hole. The legacy ioctl path still works.
> If people want to use the new Netlink path on their board, yes they need to
> convert all the parts of the chain to hwtstamp NDOs. If they don't they will
> get now a EOPNOTSUPP error instead of a null pointer dereference koops.

I agree with Kory - we don't have many spots in the code calling HW
timestamping configuration. The ones to check is actually phy code and
can drivers. But anyways, we have this interface exposed to UAPI, and we
have ethtool with supports it already. And there is a bug, which can be
fixed with the proposed code.

I'm working right now to finish conversion by the end of this term, both
can and phy will be switched to new API as well as mlx5/ti_netcp
ethernet drivers.