Re: [PATCH] iwlegacy: Sanity check for sta_id

[Stanislaw Gruszka <stf_xl@wp.pl>]

Hi,

On Sat, Sep 06, 2025 at 05:42:32PM +0800, Chen Yufeng wrote:
> This patch is similar to 2da424b0773c("iwlwifi: Sanity check for sta_id").
> `2da424b0773c` introduced a sanity check to prevent potential memory 
> corruption in function `iwl_sta_ucode_activate`.
> 
> In the iwlegacy driver, the function `il_sta_ucode_activate` shares 
> a similar logic with the `iwl_sta_ucode_activate` function in iwlwifi. 
> Initial observations suggest that the function may not adequately 
> validate the range of the `sta_id` parameter. If `sta_id` exceeds 
> the expected range, it could result in memory corruption or crash.
> 
> Although there is no confirmation of a similar vulnerability in the 
> iwlegacy driver, it is recommended to adopt a preventive approach 
> by adding range checks for `sta_id` in the `il_sta_ucode_activate` 
> function. For example:


> ```
> if (sta_id >= IL_STATION_COUNT) {
>     IL_ERR(il, "invalid sta_id %u", sta_id);
>     return -EINVAL;
> }
> ```
> Adding such boundary checks can effectively mitigate potential 
> memory corruption issues.

Ask your LLM to write a simple changelog instead of marketing fluff.
Something like: 'Add sanity check for il->stations[] array index.'.
It would be sufficient.

> Signed-off-by: Chen Yufeng <chenyufeng@iie.ac.cn>
> ---
>  drivers/net/wireless/intel/iwlegacy/common.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/wireless/intel/iwlegacy/common.c b/drivers/net/wireless/intel/iwlegacy/common.c
> index b7bd3ec4cc50..a3bcf9d9ffa2 100644
> --- a/drivers/net/wireless/intel/iwlegacy/common.c
> +++ b/drivers/net/wireless/intel/iwlegacy/common.c
> @@ -1735,10 +1735,13 @@ il_cancel_scan_deferred_work(struct il_priv *il)
>  EXPORT_SYMBOL(il_cancel_scan_deferred_work);
>  
>  /* il->sta_lock must be held */
> -static void
> +static int
>  il_sta_ucode_activate(struct il_priv *il, u8 sta_id)
>  {
> -
> +	if (sta_id >= IL_STATION_COUNT) {
> +		IL_ERR(il, "invalid sta_id %u", sta_id);
Please compile check your changes.

> +		return -EINVAL;
> +	}
>  	if (!(il->stations[sta_id].used & IL_STA_DRIVER_ACTIVE))
>  		IL_ERR("ACTIVATE a non DRIVER active station id %u addr %pM\n",
>  		       sta_id, il->stations[sta_id].sta.sta.addr);
> @@ -1752,6 +1755,7 @@ il_sta_ucode_activate(struct il_priv *il, u8 sta_id)
>  		D_ASSOC("Added STA id %u addr %pM to uCode\n", sta_id,
>  			il->stations[sta_id].sta.sta.addr);
>  	}
> +	return 0;
>  }
>  
>  static int
> @@ -1774,8 +1778,7 @@ il_process_add_sta_resp(struct il_priv *il, struct il_addsta_cmd *addsta,

This check should be done here, in il_process_add_sta_resp() since we
dereference il->stations[sta_id] in other places in this function.

Regards
Stanislaw
>  	switch (pkt->u.add_sta.status) {
>  	case ADD_STA_SUCCESS_MSK:
>  		D_INFO("C_ADD_STA PASSED\n");
> -		il_sta_ucode_activate(il, sta_id);
> -		ret = 0;
> +		ret = il_sta_ucode_activate(il, sta_id);
>  		break;
>  	case ADD_STA_NO_ROOM_IN_TBL:
>  		IL_ERR("Adding station %d failed, no room in table.\n", sta_id);
> -- 
> 2.34.1
>