Re: Re: [PATCH 03/35] fbdev: sisfb: Use safer strscpy() instead of strcpy()

Re: Re: [PATCH 03/35] fbdev: sisfb: Use safer strscpy() instead of strcpy()

[Ai Chao]

Hello David and Helge
...
> > > -            strcpy(ivideo->myid, "SiS 730");
> > > +            strscpy(ivideo->myid, "SiS 730");  
> > 
> > The compiler knows at build time the length of myid, and the "SIS 730" string.
> > Using strscpy() has no benefit here either. Contrary, the code generated
> > because of using strscpy() is probably even larger.
> > Don't replace such code with strscpy().

> Both should get converted to a memcpy().

> If you increase the literal to be too long I'm pretty sure you'll
> get a compiler warning/error from strcpy().
> OTOH strscpy() is more likely to truncate the string (I'd need to
> check).

> So leaving it as strcpy() is fine - and possibly even better.
> The header files might get changed to error strcpy() unless the compiler
> knows the source string has a constant length and the destination is
> big enough - but that hasn't been done yet.

struct sis_video_info {
    char    myid[40];
}
I have rewritten the code: 
strcpy(ivideo->myid, "SiS 730-0123456789abcdefghijklmnopqrstuvwxyz0123456789");
Used gcc version 11.4.0 (Ubuntu 11.4.0-1ubuntu1~22.04.3)
There was no compiler warning or error. 
The strcpy copies the entire string into myid(causing a buffer overflow),
whereas strscpy only copies 40 characters into myid according to its size.

Thanks,
Ai Chao

Re: Re: [PATCH 03/35] fbdev: sisfb: Use safer strscpy() instead of strcpy()

[Ai Chao]

Hello Helge
...
>>> So leaving it as strcpy() is fine - and possibly even better.
>>> The header files might get changed to error strcpy() unless the compiler
>>> knows the source string has a constant length and the destination is
>>> big enough - but that hasn't been done yet.
>> 
>> struct sis_video_info {
>>      char    myid[40];
>> }
>> I have rewritten the code:
>> strcpy(ivideo->myid, "SiS 730-0123456789abcdefghijklmnopqrstuvwxyz0123456789");
>> Used gcc version 11.4.0 (Ubuntu 11.4.0-1ubuntu1~22.04.3)
>> There was no compiler warning or error.
>
>Sure it would
>But the compiler issued a warning that the string is too big..
>So, such places will be detected at compile time.

gcc-version: gcc version 11.4.0 (Ubuntu 11.4.0-1ubuntu1~22.04.3)
aichao@thinksys:~/dev/linux-next$ make W=2 drivers/video/fbdev/sis/sis_main.o
  CC [M]  drivers/video/fbdev/sis/sis_main.o

There was no error and no warning.

Thanks,
Ai Chao