drivers/staging/rtl8192e/rtllib_wx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Replace strcpy() with strscpy() in rtl819x_translate_scan()
function to ensure buffer safety.
Signed-off-by: Abhishek Tamboli <abhishektamboli9@gmail.com>
---
drivers/staging/rtl8192e/rtllib_wx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/rtl8192e/rtllib_wx.c b/drivers/staging/rtl8192e/rtllib_wx.c
index fbd4ec824084..970b7fcb3f7e 100644
--- a/drivers/staging/rtl8192e/rtllib_wx.c
+++ b/drivers/staging/rtl8192e/rtllib_wx.c
@@ -61,7 +61,7 @@ static inline char *rtl819x_translate_scan(struct rtllib_device *ieee,
iwe.cmd = SIOCGIWNAME;
for (i = 0; i < ARRAY_SIZE(rtllib_modes); i++) {
if (network->mode & BIT(i)) {
- strcpy(pname, rtllib_modes[i]);
+ strscpy(pname, rtllib_modes[i], sizeof(pname));
pname += strlen(rtllib_modes[i]);
}
}
--
2.34.1Hi Abhishek,
kernel test robot noticed the following build errors:
[auto build test ERROR on staging/staging-testing]
url: https://github.com/intel-lab-lkp/linux/commits/Abhishek-Tamboli/staging-rtl8192e-Replace-strcpy-with-strscpy-in-rtl819x_translate_scan/20240821-024358
base: staging/staging-testing
patch link: https://lore.kernel.org/r/20240820184216.45390-1-abhishektamboli9%40gmail.com
patch subject: [PATCH] staging: rtl8192e: Replace strcpy with strscpy in rtl819x_translate_scan
config: um-allmodconfig (https://download.01.org/0day-ci/archive/20240821/202408211709.E9iGxQa8-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 26670e7fa4f032a019d23d56c6a02926e854e8af)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240821/202408211709.E9iGxQa8-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408211709.E9iGxQa8-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from include/linux/bvec.h:10:
In file included from include/linux/highmem.h:8:
In file included from include/linux/cacheflush.h:5:
In file included from arch/um/include/asm/cacheflush.h:4:
In file included from arch/um/include/asm/tlbflush.h:9:
In file included from include/linux/mm.h:2228:
include/linux/vmstat.h:514:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
514 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
In file included from drivers/staging/rtl8192e/rtllib_wx.c:18:
In file included from include/linux/etherdevice.h:20:
In file included from include/linux/if_ether.h:19:
In file included from include/linux/skbuff.h:17:
In file included from include/linux/bvec.h:10:
In file included from include/linux/highmem.h:12:
In file included from include/linux/hardirq.h:11:
In file included from arch/um/include/asm/hardirq.h:5:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:14:
In file included from arch/um/include/asm/io.h:24:
include/asm-generic/io.h:548:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
548 | val = __raw_readb(PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:561:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
561 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
| ^
In file included from drivers/staging/rtl8192e/rtllib_wx.c:18:
In file included from include/linux/etherdevice.h:20:
In file included from include/linux/if_ether.h:19:
In file included from include/linux/skbuff.h:17:
In file included from include/linux/bvec.h:10:
In file included from include/linux/highmem.h:12:
In file included from include/linux/hardirq.h:11:
In file included from arch/um/include/asm/hardirq.h:5:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:14:
In file included from arch/um/include/asm/io.h:24:
include/asm-generic/io.h:574:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
574 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
| ^
In file included from drivers/staging/rtl8192e/rtllib_wx.c:18:
In file included from include/linux/etherdevice.h:20:
In file included from include/linux/if_ether.h:19:
In file included from include/linux/skbuff.h:17:
In file included from include/linux/bvec.h:10:
In file included from include/linux/highmem.h:12:
In file included from include/linux/hardirq.h:11:
In file included from arch/um/include/asm/hardirq.h:5:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:14:
In file included from arch/um/include/asm/io.h:24:
include/asm-generic/io.h:585:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
585 | __raw_writeb(value, PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:595:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
595 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:605:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
605 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:693:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
693 | readsb(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:701:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
701 | readsw(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:709:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
709 | readsl(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:718:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
718 | writesb(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:727:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
727 | writesw(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:736:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
736 | writesl(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
In file included from drivers/staging/rtl8192e/rtllib_wx.c:15:
In file included from include/linux/wireless.h:13:
In file included from include/uapi/linux/wireless.h:74:
In file included from include/linux/socket.h:8:
In file included from include/linux/uio.h:10:
In file included from include/linux/mm_types.h:8:
In file included from include/linux/kref.h:16:
In file included from include/linux/spinlock.h:63:
In file included from include/linux/lockdep.h:14:
In file included from include/linux/smp.h:13:
In file included from include/linux/cpumask.h:12:
In file included from include/linux/bitmap.h:13:
In file included from include/linux/string.h:374:
>> include/linux/fortify-string.h:293:3: error: call to '__write_overflow' declared with 'error' attribute: detected write beyond size of object (1st parameter)
293 | __write_overflow();
| ^
13 warnings and 1 error generated.
vim +293 include/linux/fortify-string.h
a28a6e860c6cf23 Francis Laniel 2021-02-25 274
03699f271de1f4d Kees Cook 2022-09-02 275 /* Defined after fortified strnlen() to reuse it. */
e6584c3964f2ff7 Kees Cook 2023-09-20 276 extern ssize_t __real_strscpy(char *, const char *, size_t) __RENAME(sized_strscpy);
e6584c3964f2ff7 Kees Cook 2023-09-20 277 __FORTIFY_INLINE ssize_t sized_strscpy(char * const POS p, const char * const POS q, size_t size)
a28a6e860c6cf23 Francis Laniel 2021-02-25 278 {
a28a6e860c6cf23 Francis Laniel 2021-02-25 279 /* Use string size rather than possible enclosing struct size. */
21a2c74b0a2a784 Kees Cook 2023-04-07 280 const size_t p_size = __member_size(p);
21a2c74b0a2a784 Kees Cook 2023-04-07 281 const size_t q_size = __member_size(q);
21a2c74b0a2a784 Kees Cook 2023-04-07 282 size_t len;
a28a6e860c6cf23 Francis Laniel 2021-02-25 283
a28a6e860c6cf23 Francis Laniel 2021-02-25 284 /* If we cannot get size of p and q default to call strscpy. */
311fb40aa0569ab Kees Cook 2022-09-02 285 if (p_size == SIZE_MAX && q_size == SIZE_MAX)
a28a6e860c6cf23 Francis Laniel 2021-02-25 286 return __real_strscpy(p, q, size);
a28a6e860c6cf23 Francis Laniel 2021-02-25 287
a28a6e860c6cf23 Francis Laniel 2021-02-25 288 /*
a28a6e860c6cf23 Francis Laniel 2021-02-25 289 * If size can be known at compile time and is greater than
a28a6e860c6cf23 Francis Laniel 2021-02-25 290 * p_size, generate a compile time write overflow error.
a28a6e860c6cf23 Francis Laniel 2021-02-25 291 */
fa35198f39571bb Kees Cook 2022-09-19 292 if (__compiletime_lessthan(p_size, size))
a28a6e860c6cf23 Francis Laniel 2021-02-25 @293 __write_overflow();
a28a6e860c6cf23 Francis Laniel 2021-02-25 294
62e1cbfc5d79538 Kees Cook 2022-10-02 295 /* Short-circuit for compile-time known-safe lengths. */
62e1cbfc5d79538 Kees Cook 2022-10-02 296 if (__compiletime_lessthan(p_size, SIZE_MAX)) {
62e1cbfc5d79538 Kees Cook 2022-10-02 297 len = __compiletime_strlen(q);
62e1cbfc5d79538 Kees Cook 2022-10-02 298
62e1cbfc5d79538 Kees Cook 2022-10-02 299 if (len < SIZE_MAX && __compiletime_lessthan(len, size)) {
62e1cbfc5d79538 Kees Cook 2022-10-02 300 __underlying_memcpy(p, q, len + 1);
62e1cbfc5d79538 Kees Cook 2022-10-02 301 return len;
62e1cbfc5d79538 Kees Cook 2022-10-02 302 }
62e1cbfc5d79538 Kees Cook 2022-10-02 303 }
62e1cbfc5d79538 Kees Cook 2022-10-02 304
a28a6e860c6cf23 Francis Laniel 2021-02-25 305 /*
a28a6e860c6cf23 Francis Laniel 2021-02-25 306 * This call protects from read overflow, because len will default to q
a28a6e860c6cf23 Francis Laniel 2021-02-25 307 * length if it smaller than size.
a28a6e860c6cf23 Francis Laniel 2021-02-25 308 */
a28a6e860c6cf23 Francis Laniel 2021-02-25 309 len = strnlen(q, size);
a28a6e860c6cf23 Francis Laniel 2021-02-25 310 /*
a28a6e860c6cf23 Francis Laniel 2021-02-25 311 * If len equals size, we will copy only size bytes which leads to
a28a6e860c6cf23 Francis Laniel 2021-02-25 312 * -E2BIG being returned.
a28a6e860c6cf23 Francis Laniel 2021-02-25 313 * Otherwise we will copy len + 1 because of the final '\O'.
a28a6e860c6cf23 Francis Laniel 2021-02-25 314 */
a28a6e860c6cf23 Francis Laniel 2021-02-25 315 len = len == size ? size : len + 1;
a28a6e860c6cf23 Francis Laniel 2021-02-25 316
a28a6e860c6cf23 Francis Laniel 2021-02-25 317 /*
a28a6e860c6cf23 Francis Laniel 2021-02-25 318 * Generate a runtime write overflow error if len is greater than
a28a6e860c6cf23 Francis Laniel 2021-02-25 319 * p_size.
a28a6e860c6cf23 Francis Laniel 2021-02-25 320 */
3d965b33e40d973 Kees Cook 2023-04-07 321 if (p_size < len)
3d965b33e40d973 Kees Cook 2023-04-07 322 fortify_panic(FORTIFY_FUNC_strscpy, FORTIFY_WRITE, p_size, len, -E2BIG);
a28a6e860c6cf23 Francis Laniel 2021-02-25 323
a28a6e860c6cf23 Francis Laniel 2021-02-25 324 /*
a28a6e860c6cf23 Francis Laniel 2021-02-25 325 * We can now safely call vanilla strscpy because we are protected from:
a28a6e860c6cf23 Francis Laniel 2021-02-25 326 * 1. Read overflow thanks to call to strnlen().
a28a6e860c6cf23 Francis Laniel 2021-02-25 327 * 2. Write overflow thanks to above ifs.
a28a6e860c6cf23 Francis Laniel 2021-02-25 328 */
a28a6e860c6cf23 Francis Laniel 2021-02-25 329 return __real_strscpy(p, q, len);
a28a6e860c6cf23 Francis Laniel 2021-02-25 330 }
a28a6e860c6cf23 Francis Laniel 2021-02-25 331
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Le 20/08/2024 à 20:42, Abhishek Tamboli a écrit :
> Replace strcpy() with strscpy() in rtl819x_translate_scan()
> function to ensure buffer safety.
>
> Signed-off-by: Abhishek Tamboli <abhishektamboli9@gmail.com>
> ---
> drivers/staging/rtl8192e/rtllib_wx.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/rtl8192e/rtllib_wx.c b/drivers/staging/rtl8192e/rtllib_wx.c
> index fbd4ec824084..970b7fcb3f7e 100644
> --- a/drivers/staging/rtl8192e/rtllib_wx.c
> +++ b/drivers/staging/rtl8192e/rtllib_wx.c
> @@ -61,7 +61,7 @@ static inline char *rtl819x_translate_scan(struct rtllib_device *ieee,
> iwe.cmd = SIOCGIWNAME;
> for (i = 0; i < ARRAY_SIZE(rtllib_modes); i++) {
> if (network->mode & BIT(i)) {
> - strcpy(pname, rtllib_modes[i]);
> + strscpy(pname, rtllib_modes[i], sizeof(pname));
This not correct.
sizeof(pname) is 4 here, but the buffer that is really used is "char
proto_name[6];"
6 chars are needed for storing "N-24G" (see rtllib_modes), so 5 chars +
ending \0.
When you will send a v2, here are a few others things you could give a
look at:
- is 'pname' really needed or is 'proto_name' enough?
- what about the "*pname = '\0';" after the loop?
- if a "mode" matches, do we need to iterate the whole rtllib_modes
array? (have a look at wireless_mode)
CJ
> pname += strlen(rtllib_modes[i]);
> }
> }
> --
> 2.34.1
>
>
>
On Tue, Aug 20, 2024 at 09:38:22PM +0200, Christophe JAILLET wrote: > - if a "mode" matches, do we need to iterate the whole rtllib_modes > array? (have a look at wireless_mode) > Can only one mode be set at a time? regards, dan carpenter
Le 21/08/2024 à 20:23, Dan Carpenter a écrit : > On Tue, Aug 20, 2024 at 09:38:22PM +0200, Christophe JAILLET wrote: >> - if a "mode" matches, do we need to iterate the whole rtllib_modes >> array? (have a look at wireless_mode) >> > > Can only one mode be set at a time? > > regards, > dan carpenter > > > Hmm, apparently several can be set (see [1]) Base on a few lines below, it looks that WIRELESS_MODE_N_24G is exclusive from the other ones. So the 6 char array seems to be sized either for "N-24G", either for a concatenation of a few other modes that won't exceed the size of the buffer. CJ [1]: https://elixir.bootlin.com/linux/v6.11-rc4/source/drivers/staging/rtl8192e/rtllib_rx.c#L2200
On Sat, Aug 24, 2024 at 08:35:03AM +0200, Christophe JAILLET wrote: > Le 21/08/2024 à 20:23, Dan Carpenter a écrit : > > On Tue, Aug 20, 2024 at 09:38:22PM +0200, Christophe JAILLET wrote: > > > - if a "mode" matches, do we need to iterate the whole rtllib_modes > > > array? (have a look at wireless_mode) > > > > > > > Can only one mode be set at a time? > > > > regards, > > dan carpenter > > > > > > > > Hmm, apparently several can be set (see [1]) > > Base on a few lines below, it looks that WIRELESS_MODE_N_24G is exclusive > from the other ones. > > So the 6 char array seems to be sized either for "N-24G", either for a > concatenation of a few other modes that won't exceed the size of the buffer. > Yeah. I started to review this patch and found the same thing but I never hit send on my review. 6 chars is enough. If you look at the commit which changed the buffer size to 6, anything larger would cause a GCC checker warning about truncating strings. Still, it's kind of ugly that we need to do this much research to verify that the code isn't a memory corruption bug. It doesn't feel future proof either. It would be nicer if this were obviously safe from just reviewing the function. regards, dan carpenter
On Wed, Aug 21, 2024 at 12:12:16AM +0530, Abhishek Tamboli wrote:
> Replace strcpy() with strscpy() in rtl819x_translate_scan()
> function to ensure buffer safety.
>
> Signed-off-by: Abhishek Tamboli <abhishektamboli9@gmail.com>
> ---
> drivers/staging/rtl8192e/rtllib_wx.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/rtl8192e/rtllib_wx.c b/drivers/staging/rtl8192e/rtllib_wx.c
> index fbd4ec824084..970b7fcb3f7e 100644
> --- a/drivers/staging/rtl8192e/rtllib_wx.c
> +++ b/drivers/staging/rtl8192e/rtllib_wx.c
> @@ -61,7 +61,7 @@ static inline char *rtl819x_translate_scan(struct rtllib_device *ieee,
> iwe.cmd = SIOCGIWNAME;
> for (i = 0; i < ARRAY_SIZE(rtllib_modes); i++) {
> if (network->mode & BIT(i)) {
> - strcpy(pname, rtllib_modes[i]);
> + strscpy(pname, rtllib_modes[i], sizeof(pname));
^^^^^
pname is a pointer, not an array, so this doesn't work.
> pname += strlen(rtllib_modes[i]);
^^^^^^^^
pname is incremented here.
What this loop is doing is that it's going through all the network modes and
adding to the string. You should look at the rtllib_modes[] array and ensure
that if we printed every string it would fit into pname. (Currently that is not
the case. Probably not all network modes are possible. But I have looked at
this code and I'm saying that we should just ensure that we could handle it if
they were all possible).
Feel free to re-format the code how ever you want to make that happen.
regards,
dan carpenter
On Tue, Aug 20, 2024 at 10:29:47PM +0300, Dan Carpenter wrote:
> On Wed, Aug 21, 2024 at 12:12:16AM +0530, Abhishek Tamboli wrote:
> > Replace strcpy() with strscpy() in rtl819x_translate_scan()
> > function to ensure buffer safety.
> >
> > Signed-off-by: Abhishek Tamboli <abhishektamboli9@gmail.com>
> > ---
> > drivers/staging/rtl8192e/rtllib_wx.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/rtl8192e/rtllib_wx.c b/drivers/staging/rtl8192e/rtllib_wx.c
> > index fbd4ec824084..970b7fcb3f7e 100644
> > --- a/drivers/staging/rtl8192e/rtllib_wx.c
> > +++ b/drivers/staging/rtl8192e/rtllib_wx.c
> > @@ -61,7 +61,7 @@ static inline char *rtl819x_translate_scan(struct rtllib_device *ieee,
> > iwe.cmd = SIOCGIWNAME;
> > for (i = 0; i < ARRAY_SIZE(rtllib_modes); i++) {
> > if (network->mode & BIT(i)) {
> > - strcpy(pname, rtllib_modes[i]);
> > + strscpy(pname, rtllib_modes[i], sizeof(pname));
> ^^^^^
> pname is a pointer, not an array, so this doesn't work.
Thanks for pointing out the issue with strscpy.
> > pname += strlen(rtllib_modes[i]);
> ^^^^^^^^
> pname is incremented here.
>
> What this loop is doing is that it's going through all the network modes and
> adding to the string. You should look at the rtllib_modes[] array and ensure
> that if we printed every string it would fit into pname. (Currently that is not
> the case. Probably not all network modes are possible. But I have looked at
> this code and I'm saying that we should just ensure that we could handle it if
> they were all possible).
I understand that the size of proto_name is insufficient if all network modes
from rtllib_modes[] are copied, so I need to increase its size.
Given this, would it be better to use strcat?
This would eliminate the need for the pname pointer and align with
the current code's method of concatenating the rtllib_modes.
Regards,
Abhishek
On Wed, Aug 21, 2024 at 11:18:17PM +0530, Abhishek Tamboli wrote: > > Given this, would it be better to use strcat? > This would eliminate the need for the pname pointer and align with > the current code's method of concatenating the rtllib_modes. We can't ack the patch until we see it. Try it out and see if it looks good to you. regards, dan carpenter
© 2016 - 2026 Red Hat, Inc.