* [bug report] wilc1000: move wilc driver out of staging
@ 2025-08-29 7:50 Dan Carpenter
2025-08-29 18:53 ` Ajay.Kathat
0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2025-08-29 7:50 UTC (permalink / raw)
To: Ajay Singh; +Cc: linux-wireless
Hello Ajay Singh,
[ Obviously this bug was in staging as well... ]
Commit 5625f965d764 ("wilc1000: move wilc driver out of staging")
from Jun 25, 2020 (linux-next), leads to the following Smatch static
checker warning:
drivers/net/wireless/microchip/wilc1000/wlan_cfg.c:184 wilc_wlan_parse_response_frame()
error: '__memcpy()' 'cfg->s[i]->str' copy overflow (512 vs 65537)
drivers/net/wireless/microchip/wilc1000/wlan_cfg.c
138 static void wilc_wlan_parse_response_frame(struct wilc *wl, u8 *info, int size)
139 {
140 u16 wid;
141 u32 len = 0, i = 0;
142 struct wilc_cfg *cfg = &wl->cfg;
143
144 while (size > 0) {
145 i = 0;
146 wid = get_unaligned_le16(info);
147
148 switch (FIELD_GET(WILC_WID_TYPE, wid)) {
149 case WID_CHAR:
150 while (cfg->b[i].id != WID_NIL && cfg->b[i].id != wid)
151 i++;
152
This is the rx path and info comes from skb->data so it feels like we
need more bounds checking.
if (info < 5)
return;
153 if (cfg->b[i].id == wid)
154 cfg->b[i].val = info[4];
155
156 len = 3;
157 break;
158
159 case WID_SHORT:
if (info < 6)
return;
160 while (cfg->hw[i].id != WID_NIL && cfg->hw[i].id != wid)
161 i++;
162
163 if (cfg->hw[i].id == wid)
164 cfg->hw[i].val = get_unaligned_le16(&info[4]);
165
166 len = 4;
167 break;
168
169 case WID_INT:
if (info < 8)
return;
170 while (cfg->w[i].id != WID_NIL && cfg->w[i].id != wid)
171 i++;
172
173 if (cfg->w[i].id == wid)
174 cfg->w[i].val = get_unaligned_le32(&info[4]);
175
176 len = 6;
177 break;
178
179 case WID_STR:
How many bytes are there in cfg->s[i].str? Smatch thinks 512, but I
don't know where Smatch is getting that...
len = 2 + get_unaligned_le16(&info[2]);
if (len + 2 > SOME_LIMIT)
return;
180 while (cfg->s[i].id != WID_NIL && cfg->s[i].id != wid)
181 i++;
182
183 if (cfg->s[i].id == wid)
--> 184 memcpy(cfg->s[i].str, &info[2],
185 get_unaligned_le16(&info[2]) + 2);
186
187 len = 2 + get_unaligned_le16(&info[2]);
188 break;
189
190 default:
191 break;
192 }
193 size -= (2 + len);
194 info += (2 + len);
195 }
196 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [bug report] wilc1000: move wilc driver out of staging
2025-08-29 7:50 [bug report] wilc1000: move wilc driver out of staging Dan Carpenter
@ 2025-08-29 18:53 ` Ajay.Kathat
0 siblings, 0 replies; 2+ messages in thread
From: Ajay.Kathat @ 2025-08-29 18:53 UTC (permalink / raw)
To: dan.carpenter; +Cc: linux-wireless
Hi Dan,
Thanks for pointing out this warning along with the analysis.
On 8/29/25 00:50, Dan Carpenter wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Hello Ajay Singh,
>
> [ Obviously this bug was in staging as well... ]
>
> Commit 5625f965d764 ("wilc1000: move wilc driver out of staging")
> from Jun 25, 2020 (linux-next), leads to the following Smatch static
> checker warning:
>
> drivers/net/wireless/microchip/wilc1000/wlan_cfg.c:184 wilc_wlan_parse_response_frame()
> error: '__memcpy()' 'cfg->s[i]->str' copy overflow (512 vs 65537)
>
> drivers/net/wireless/microchip/wilc1000/wlan_cfg.c
> 138 static void wilc_wlan_parse_response_frame(struct wilc *wl, u8 *info, int size)
> 139 {
> 140 u16 wid;
> 141 u32 len = 0, i = 0;
> 142 struct wilc_cfg *cfg = &wl->cfg;
> 143
> 144 while (size > 0) {
> 145 i = 0;
> 146 wid = get_unaligned_le16(info);
> 147
> 148 switch (FIELD_GET(WILC_WID_TYPE, wid)) {
> 149 case WID_CHAR:
> 150 while (cfg->b[i].id != WID_NIL && cfg->b[i].id != wid)
> 151 i++;
> 152
>
> This is the rx path and info comes from skb->data so it feels like we
> need more bounds checking.
>
> if (info < 5)
> return;
>
Agree, the read can go beyond the buffer limit so it's safe to add boundary
checks before accessing the 'info' buffer.
> 153 if (cfg->b[i].id == wid)
> 154 cfg->b[i].val = info[4];
> 155
> 156 len = 3;
> 157 break;
> 158
> 159 case WID_SHORT:
>
> if (info < 6)
> return;
>
> 160 while (cfg->hw[i].id != WID_NIL && cfg->hw[i].id != wid)
> 161 i++;
> 162
> 163 if (cfg->hw[i].id == wid)
> 164 cfg->hw[i].val = get_unaligned_le16(&info[4]);
> 165
> 166 len = 4;
> 167 break;
> 168
> 169 case WID_INT:
>
> if (info < 8)
> return;
>
> 170 while (cfg->w[i].id != WID_NIL && cfg->w[i].id != wid)
> 171 i++;
> 172
> 173 if (cfg->w[i].id == wid)
> 174 cfg->w[i].val = get_unaligned_le32(&info[4]);
> 175
> 176 len = 6;
> 177 break;
> 178
> 179 case WID_STR:
>
> How many bytes are there in cfg->s[i].str? Smatch thinks 512, but I
> don't know where Smatch is getting that...
>
> len = 2 + get_unaligned_le16(&info[2]);
> if (len + 2 > SOME_LIMIT)
> return;
I think Smatch got 512 value from the below macro.
#define WILC_MAX_ASSOC_RESP_FRAME_SIZE 512
Patch[1] has been submitted to address this warning. I hope this would resolve
the warning.
1.
https://lore.kernel.org/linux-wireless/20250829182241.45150-1-ajay.kathat@microchip.com/
Regards,
Ajay
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2025-08-29 18:53 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-29 7:50 [bug report] wilc1000: move wilc driver out of staging Dan Carpenter
2025-08-29 18:53 ` Ajay.Kathat
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox