[PATCH 1/2] nvmet: re-fix tracing strncpy() warning

[PATCH 1/2] nvmet: re-fix tracing strncpy() warning

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

An earlier patch had tried to address a warning about a string copy with
missing zero termination:

drivers/nvme/target/trace.h:52:3: warning: ‘strncpy’ specified bound 32 equals destination size [-Wstringop-truncation]

The new version causes a different warning with some compiler versions, notably
gcc-9 and gcc-10, and also misses the zero padding that was apparently done
intentionally in the original code:

drivers/nvme/target/trace.h:56:2: error: 'strncpy' specified bound depends on the length of the source argument [-Werror=stringop-overflow=]

Change it to use strscpy_pad() with the original length, which will give
a properly padded and zero-terminated string as well as avoiding the warning.

Fixes: d86481e924a7 ("nvmet: use min of device_path and disk len")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/nvme/target/trace.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/nvme/target/trace.h b/drivers/nvme/target/trace.h
index 6109b3806b12..155334ddc13f 100644
--- a/drivers/nvme/target/trace.h
+++ b/drivers/nvme/target/trace.h
@@ -53,8 +53,7 @@ static inline void __assign_req_name(char *name, struct nvmet_req *req)
 		return;
 	}
 
-	strncpy(name, req->ns->device_path,
-		min_t(size_t, DISK_NAME_LEN, strlen(req->ns->device_path)));
+	strscpy_pad(name, req->ns->device_path, DISK_NAME_LEN);
 }
 #endif
 
-- 
2.39.2

[PATCH 2/2] nvme: trace: avoid memcpy overflow warning

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

A previous patch introduced a struct_group() in nvme_common_command to help
stringop fortification figure out the length of the fields, but one function
is not currently using them:

In file included from drivers/nvme/target/core.c:7:
In file included from include/linux/string.h:254:
include/linux/fortify-string.h:592:4: error: call to '__read_overflow2_field' declared with 'warning' attribute: detected read beyond size of field (2nd parameter); maybe use struct_group()? [-Werror,-Wattribute-warning]
                        __read_overflow2_field(q_size_field, size);
                        ^

Change this one to use the correct field name to avoid the warning.

Fixes: 5c629dc9609dc ("nvme: use struct group for generic command dwords")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/nvme/target/trace.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/nvme/target/trace.h b/drivers/nvme/target/trace.h
index 155334ddc13f..dbb911fd502d 100644
--- a/drivers/nvme/target/trace.h
+++ b/drivers/nvme/target/trace.h
@@ -84,8 +84,7 @@ TRACE_EVENT(nvmet_req_init,
 		__entry->flags = cmd->common.flags;
 		__entry->nsid = le32_to_cpu(cmd->common.nsid);
 		__entry->metadata = le64_to_cpu(cmd->common.metadata);
-		memcpy(__entry->cdw10, &cmd->common.cdw10,
-			sizeof(__entry->cdw10));
+		memcpy(__entry->cdw10, &cmd->common.cdws, sizeof(__entry->cdw10));
 	),
 	TP_printk("nvmet%s: %sqid=%d, cmdid=%u, nsid=%u, flags=%#x, "
 		  "meta=%#llx, cmd=(%s, %s)",
-- 
2.39.2

Re: [PATCH 2/2] nvme: trace: avoid memcpy overflow warning

[Christoph Hellwig]

On Wed, Jan 03, 2024 at 04:56:56PM +0100, Arnd Bergmann wrote:
> -		memcpy(__entry->cdw10, &cmd->common.cdw10,
> -			sizeof(__entry->cdw10));
> +		memcpy(__entry->cdw10, &cmd->common.cdws, sizeof(__entry->cdw10));

Please avoid the overly long line.

Re: [PATCH 2/2] nvme: trace: avoid memcpy overflow warning

[Keith Busch]

On Wed, Jan 03, 2024 at 04:56:56PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> A previous patch introduced a struct_group() in nvme_common_command to help
> stringop fortification figure out the length of the fields, but one function
> is not currently using them:
> 
> In file included from drivers/nvme/target/core.c:7:
> In file included from include/linux/string.h:254:
> include/linux/fortify-string.h:592:4: error: call to '__read_overflow2_field' declared with 'warning' attribute: detected read beyond size of field (2nd parameter); maybe use struct_group()? [-Werror,-Wattribute-warning]
>                         __read_overflow2_field(q_size_field, size);
>                         ^
> 
> Change this one to use the correct field name to avoid the warning.
> 
> Fixes: 5c629dc9609dc ("nvme: use struct group for generic command dwords")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Thanks, applied to nvme-6.8 with Christoph's line-wrap suggestion.

Re: [PATCH 1/2] nvmet: re-fix tracing strncpy() warning

[Keith Busch]

On Wed, Jan 03, 2024 at 04:56:55PM +0100, Arnd Bergmann wrote:
> @@ -53,8 +53,7 @@ static inline void __assign_req_name(char *name, struct nvmet_req *req)
>  		return;
>  	}
>  
> -	strncpy(name, req->ns->device_path,
> -		min_t(size_t, DISK_NAME_LEN, strlen(req->ns->device_path)));
> +	strscpy_pad(name, req->ns->device_path, DISK_NAME_LEN);
>  }

I like this one, however Daniel has a different fix for this already
staged in nvme-6.8:

 https://git.infradead.org/nvme.git/commitdiff/8f6c0eec5fad13785fd53a5b3b5f8b97b722a2a3

Re: [PATCH 1/2] nvmet: re-fix tracing strncpy() warning

[Arnd Bergmann]

On Fri, Jan 5, 2024, at 21:24, Keith Busch wrote:
> On Wed, Jan 03, 2024 at 04:56:55PM +0100, Arnd Bergmann wrote:
>> @@ -53,8 +53,7 @@ static inline void __assign_req_name(char *name, struct nvmet_req *req)
>>  		return;
>>  	}
>>  
>> -	strncpy(name, req->ns->device_path,
>> -		min_t(size_t, DISK_NAME_LEN, strlen(req->ns->device_path)));
>> +	strscpy_pad(name, req->ns->device_path, DISK_NAME_LEN);
>>  }
>
> I like this one, however Daniel has a different fix for this already
> staged in nvme-6.8:
>
>  
> https://git.infradead.org/nvme.git/commitdiff/8f6c0eec5fad13785fd53a5b3b5f8b97b722a2a3

+       snprintf(name,
+                min_t(size_t, DISK_NAME_LEN, strlen(req->ns->device_path) + 1),
+                "%s", req->ns->device_path);

Don't we still need the zero-padding here to avoid leaking
kernel data to userspace?

    Arnd

Re: [PATCH 1/2] nvmet: re-fix tracing strncpy() warning

[Keith Busch]

On Fri, Jan 05, 2024 at 09:36:38PM +0100, Arnd Bergmann wrote:
> On Fri, Jan 5, 2024, at 21:24, Keith Busch wrote:
> > On Wed, Jan 03, 2024 at 04:56:55PM +0100, Arnd Bergmann wrote:
> >> @@ -53,8 +53,7 @@ static inline void __assign_req_name(char *name, struct nvmet_req *req)
> >>  		return;
> >>  	}
> >>  
> >> -	strncpy(name, req->ns->device_path,
> >> -		min_t(size_t, DISK_NAME_LEN, strlen(req->ns->device_path)));
> >> +	strscpy_pad(name, req->ns->device_path, DISK_NAME_LEN);
> >>  }
> >
> > I like this one, however Daniel has a different fix for this already
> > staged in nvme-6.8:
> >
> >  
> > https://git.infradead.org/nvme.git/commitdiff/8f6c0eec5fad13785fd53a5b3b5f8b97b722a2a3
> 
> +       snprintf(name,
> +                min_t(size_t, DISK_NAME_LEN, strlen(req->ns->device_path) + 1),
> +                "%s", req->ns->device_path);
> 
> Don't we still need the zero-padding here to avoid leaking
> kernel data to userspace?

I'm not sure. This potentially leaves trace buffer memory uninitialized
after the string, but isn't the trace buffer user accessible when it was
initially allocated?

For correctness, though, yes, I think you're right so I may just back
out this one and replace with yours since we haven't sent a recent 6.8
pull request yet.

Re: [PATCH 1/2] nvmet: re-fix tracing strncpy() warning

[Arnd Bergmann]

On Fri, Jan 5, 2024, at 21:57, Keith Busch wrote:
> On Fri, Jan 05, 2024 at 09:36:38PM +0100, Arnd Bergmann wrote:
>> On Fri, Jan 5, 2024, at 21:24, Keith Busch wrote:
>> > On Wed, Jan 03, 2024 at 04:56:55PM +0100, Arnd Bergmann wrote:
>> >> @@ -53,8 +53,7 @@ static inline void __assign_req_name(char *name, struct nvmet_req *req)
>> >>  		return;
>> >>  	}
>> >>  
>> >> -	strncpy(name, req->ns->device_path,
>> >> -		min_t(size_t, DISK_NAME_LEN, strlen(req->ns->device_path)));
>> >> +	strscpy_pad(name, req->ns->device_path, DISK_NAME_LEN);
>> >>  }
>> >
>> > I like this one, however Daniel has a different fix for this already
>> > staged in nvme-6.8:
>> >
>> >  
>> > https://git.infradead.org/nvme.git/commitdiff/8f6c0eec5fad13785fd53a5b3b5f8b97b722a2a3
>> 
>> +       snprintf(name,
>> +                min_t(size_t, DISK_NAME_LEN, strlen(req->ns->device_path) + 1),
>> +                "%s", req->ns->device_path);
>> 
>> Don't we still need the zero-padding here to avoid leaking
>> kernel data to userspace?
>
> I'm not sure. This potentially leaves trace buffer memory uninitialized
> after the string, but isn't the trace buffer user accessible when it was
> initially allocated?

I'm always confused by how the tracing macros work exactly, so
I don't know either. Looking through the other tracing headers
with string copies, I see that about half of them use the padding
variants (strncpy or strscpy_pad), while the other half use
non-padding strscpy.

     Arnd