[PATCH] ch: Do not read past the end of vendor_labels[]

[PATCH] ch: Do not read past the end of vendor_labels[]

[Bart Van Assche]

This patch fixes the following gcc 10 compiler error:

In function 'memcpy',
    inlined from 'ch_ioctl' at drivers/scsi/ch.c:666:4:
./include/linux/string.h:377:4: error: call to '__read_overflow2' declared with attribute error: detected read beyond size of object passed as 2nd parameter
  377 |    __read_overflow2();
      |    ^~~~~~~~~~~~~~~~~~

Cc: Hannes Reinecke <hare@suse.de>
Cc: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/ch.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c
index b81b397366db..b675a01380eb 100644
--- a/drivers/scsi/ch.c
+++ b/drivers/scsi/ch.c
@@ -651,19 +651,23 @@ static long ch_ioctl(struct file *file,
 		memset(&vparams,0,sizeof(vparams));
 		if (ch->counts[CHET_V1]) {
 			vparams.cvp_n1  = ch->counts[CHET_V1];
-			memcpy(vparams.cvp_label1,vendor_labels[0],16);
+			strncpy(vparams.cvp_label1, vendor_labels[0],
+				ARRAY_SIZE(vparams.cvp_label1));
 		}
 		if (ch->counts[CHET_V2]) {
 			vparams.cvp_n2  = ch->counts[CHET_V2];
-			memcpy(vparams.cvp_label2,vendor_labels[1],16);
+			strncpy(vparams.cvp_label2, vendor_labels[1],
+				ARRAY_SIZE(vparams.cvp_label2));
 		}
 		if (ch->counts[CHET_V3]) {
 			vparams.cvp_n3  = ch->counts[CHET_V3];
-			memcpy(vparams.cvp_label3,vendor_labels[2],16);
+			strncpy(vparams.cvp_label3, vendor_labels[2],
+				ARRAY_SIZE(vparams.cvp_label3));
 		}
 		if (ch->counts[CHET_V4]) {
 			vparams.cvp_n4  = ch->counts[CHET_V4];
-			memcpy(vparams.cvp_label4,vendor_labels[3],16);
+			strncpy(vparams.cvp_label4, vendor_labels[3],
+				ARRAY_SIZE(vparams.cvp_label4));
 		}
 		if (copy_to_user(argp, &vparams, sizeof(vparams)))
 			return -EFAULT;

Re: [PATCH] ch: Do not read past the end of vendor_labels[]

[Arnd Bergmann]

On Mon, Jun 29, 2020 at 6:11 PM Bart Van Assche <bvanassche@acm.org> wrote:
>
> This patch fixes the following gcc 10 compiler error:
>
> In function 'memcpy',
>     inlined from 'ch_ioctl' at drivers/scsi/ch.c:666:4:
> ./include/linux/string.h:377:4: error: call to '__read_overflow2' declared with attribute error: detected read beyond size of object passed as 2nd parameter
>   377 |    __read_overflow2();
>       |    ^~~~~~~~~~~~~~~~~~
>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/scsi/ch.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c
> index b81b397366db..b675a01380eb 100644
> --- a/drivers/scsi/ch.c
> +++ b/drivers/scsi/ch.c
> @@ -651,19 +651,23 @@ static long ch_ioctl(struct file *file,
>                 memset(&vparams,0,sizeof(vparams));
>                 if (ch->counts[CHET_V1]) {
>                         vparams.cvp_n1  = ch->counts[CHET_V1];
> -                       memcpy(vparams.cvp_label1,vendor_labels[0],16);
> +                       strncpy(vparams.cvp_label1, vendor_labels[0],
> +                               ARRAY_SIZE(vparams.cvp_label1));
>                 }

Against which tree is this? I see in mainline the correct

      strncpy(vparams.cvp_label1,vendor_labels[0],16);

rather than the broken memcpy. If this was changed recently to the
broken version, maybe send a revert, or add a "Fixes" tag?

        Arnd

Re: [PATCH] ch: Do not read past the end of vendor_labels[]

[Bart Van Assche]

On 2020-06-29 11:33, Arnd Bergmann wrote:
> On Mon, Jun 29, 2020 at 6:11 PM Bart Van Assche <bvanassche@acm.org> wrote:
>> diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c
>> index b81b397366db..b675a01380eb 100644
>> --- a/drivers/scsi/ch.c
>> +++ b/drivers/scsi/ch.c
>> @@ -651,19 +651,23 @@ static long ch_ioctl(struct file *file,
>>                 memset(&vparams,0,sizeof(vparams));
>>                 if (ch->counts[CHET_V1]) {
>>                         vparams.cvp_n1  = ch->counts[CHET_V1];
>> -                       memcpy(vparams.cvp_label1,vendor_labels[0],16);
>> +                       strncpy(vparams.cvp_label1, vendor_labels[0],
>> +                               ARRAY_SIZE(vparams.cvp_label1));
>>                 }
> 
> Against which tree is this? I see in mainline the correct
> 
>       strncpy(vparams.cvp_label1,vendor_labels[0],16);
> 
> rather than the broken memcpy. If this was changed recently to the
> broken version, maybe send a revert, or add a "Fixes" tag?

Hi Arnd,

Thanks for having taken a look. This patch applies to Martin's for-next
branch. The most recent ch patch I found in Linus' master branch is "ch:
remove ch_mutex()" from February 2020. I haven't found any more recent
ch patches in the linux-next/master branch either. Have I perhaps been
looking at the wrong repository or the wrong branch?

Thanks,

Bart.

Re: [PATCH] ch: Do not read past the end of vendor_labels[]

[Arnd Bergmann]

On Mon, Jun 29, 2020 at 9:26 PM Bart Van Assche <bvanassche@acm.org> wrote:
> On 2020-06-29 11:33, Arnd Bergmann wrote:
> > On Mon, Jun 29, 2020 at 6:11 PM Bart Van Assche <bvanassche@acm.org> wrote:
> >> diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c
> >> index b81b397366db..b675a01380eb 100644
> >> --- a/drivers/scsi/ch.c
> >> +++ b/drivers/scsi/ch.c
> >> @@ -651,19 +651,23 @@ static long ch_ioctl(struct file *file,
> >>                 memset(&vparams,0,sizeof(vparams));
> >>                 if (ch->counts[CHET_V1]) {
> >>                         vparams.cvp_n1  = ch->counts[CHET_V1];
> >> -                       memcpy(vparams.cvp_label1,vendor_labels[0],16);
> >> +                       strncpy(vparams.cvp_label1, vendor_labels[0],
> >> +                               ARRAY_SIZE(vparams.cvp_label1));
> >>                 }
> >
> > Against which tree is this? I see in mainline the correct
> >
> >       strncpy(vparams.cvp_label1,vendor_labels[0],16);
> >
> > rather than the broken memcpy. If this was changed recently to the
> > broken version, maybe send a revert, or add a "Fixes" tag?
>
> Hi Arnd,
>
> Thanks for having taken a look. This patch applies to Martin's for-next
> branch. The most recent ch patch I found in Linus' master branch is "ch:
> remove ch_mutex()" from February 2020. I haven't found any more recent
> ch patches in the linux-next/master branch either. Have I perhaps been
> looking at the wrong repository or the wrong branch?

That is the right branch, and I don't see any later changes to the file
after Feb 2020 in there or in mainline either, but I also clearly see it
using strncpy(). See also:

https://elixir.bootlin.com/linux/v5.8-rc3/source/drivers/scsi/ch.c#L647

I think there were some patches under discussion about replacing
a lot of strncpy() calls with the more intuitive memcpy(), strnlcpy()
or strscpy() alternatives, but in this case strncpy() in in fact the
correct one (as you also concluded) and I don't see any patches to
this file getting applied to that effect.

       Arnd