[PATCH] nvdimm: ndtest: reject wrapped config-data offsets

[PATCH] nvdimm: ndtest: reject wrapped config-data offsets

[Samuel Moelius]

The ndtest provider validates get/set config-data requests by adding the
ioctl-provided offset and length and comparing the result against
LABEL_SIZE. That addition can wrap, so an offset such as U32_MAX with a
one-byte length passes validation and then copies from or to
label_area + U32_MAX.

Validate the command buffer shape, then validate the offset first and
validate the length against the remaining label area so wrapped ranges
are rejected before the copy. Report the rejection through the command
status field so the DIMM ioctl ABI returns a nonzero command status
instead of faulting.

Assisted-by: Codex:gpt-5.5-cyber-preview
Signed-off-by: Samuel Moelius <sam.moelius@trailofbits.com>
---
 tools/testing/nvdimm/test/ndtest.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/tools/testing/nvdimm/test/ndtest.c b/tools/testing/nvdimm/test/ndtest.c
index 8e3b6be53839..1df93f5e4cb6 100644
--- a/tools/testing/nvdimm/test/ndtest.c
+++ b/tools/testing/nvdimm/test/ndtest.c
@@ -207,9 +207,15 @@ static int ndtest_config_get(struct ndtest_dimm *p, unsigned int buf_len,
 {
 	unsigned int len;
 
-	if ((hdr->in_offset + hdr->in_length) > LABEL_SIZE)
+	if (buf_len < sizeof(*hdr) || hdr->in_length > buf_len - sizeof(*hdr))
 		return -EINVAL;
 
+	if (hdr->in_offset > LABEL_SIZE ||
+	    hdr->in_length > LABEL_SIZE - hdr->in_offset) {
+		hdr->status = -EINVAL;
+		return 0;
+	}
+
 	hdr->status = 0;
 	len = min(hdr->in_length, LABEL_SIZE - hdr->in_offset);
 	memcpy(hdr->out_buf, p->label_area + hdr->in_offset, len);
@@ -221,10 +227,20 @@ static int ndtest_config_set(struct ndtest_dimm *p, unsigned int buf_len,
 			     struct nd_cmd_set_config_hdr *hdr)
 {
 	unsigned int len;
+	u32 *status;
 
-	if ((hdr->in_offset + hdr->in_length) > LABEL_SIZE)
+	if (buf_len < sizeof(*hdr) + sizeof(*status) ||
+	    hdr->in_length > buf_len - sizeof(*hdr) - sizeof(*status))
 		return -EINVAL;
 
+	status = (void *)hdr + sizeof(*hdr) + hdr->in_length;
+	if (hdr->in_offset > LABEL_SIZE ||
+	    hdr->in_length > LABEL_SIZE - hdr->in_offset) {
+		*status = -EINVAL;
+		return 0;
+	}
+
+	*status = 0;
 	len = min(hdr->in_length, LABEL_SIZE - hdr->in_offset);
 	memcpy(p->label_area + hdr->in_offset, hdr->in_buf, len);
 
-- 
2.43.0

Re: [PATCH] nvdimm: ndtest: reject wrapped config-data offsets

[Alison Schofield]

On Fri, Jun 05, 2026 at 12:53:36AM +0000, Samuel Moelius wrote:
> The ndtest provider validates get/set config-data requests by adding the
> ioctl-provided offset and length and comparing the result against
> LABEL_SIZE. That addition can wrap, so an offset such as U32_MAX with a
> one-byte length passes validation and then copies from or to
> label_area + U32_MAX.
> 
> Validate the command buffer shape, then validate the offset first and
> validate the length against the remaining label area so wrapped ranges
> are rejected before the copy. Report the rejection through the command
> status field so the DIMM ioctl ABI returns a nonzero command status
> instead of faulting.

Hi Samuel,

Sorry Samuel I just found this one hiding in the backlog today.
Some comments inline -

Also wondering about testing of this?

-- Alison

> 
> Assisted-by: Codex:gpt-5.5-cyber-preview
> Signed-off-by: Samuel Moelius <sam.moelius@trailofbits.com>
> ---
>  tools/testing/nvdimm/test/ndtest.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/nvdimm/test/ndtest.c b/tools/testing/nvdimm/test/ndtest.c
> index 8e3b6be53839..1df93f5e4cb6 100644
> --- a/tools/testing/nvdimm/test/ndtest.c
> +++ b/tools/testing/nvdimm/test/ndtest.c
> @@ -207,9 +207,15 @@ static int ndtest_config_get(struct ndtest_dimm *p, unsigned int buf_len,
>  {
>  	unsigned int len;
>  
> -	if ((hdr->in_offset + hdr->in_length) > LABEL_SIZE)
> +	if (buf_len < sizeof(*hdr) || hdr->in_length > buf_len - sizeof(*hdr))
>  		return -EINVAL;
>  
> +	if (hdr->in_offset > LABEL_SIZE ||
> +	    hdr->in_length > LABEL_SIZE - hdr->in_offset) {
> +		hdr->status = -EINVAL;
> +		return 0;


This reports the failure thru the cmd status but waht about the
in-kenrel callers? nvdimm_[get|set]_config_data() check cmd_rc
not the status field. Would an out of range request appear to
succeed on those paths?  Can we preserve the error for those
callers?


> +	}
> +
>  	hdr->status = 0;
>  	len = min(hdr->in_length, LABEL_SIZE - hdr->in_offset);

At this point I think min is redundant. hdr->in_length is guaranteed
to fit in the remaining label area.


>  	memcpy(hdr->out_buf, p->label_area + hdr->in_offset, len);
> @@ -221,10 +227,20 @@ static int ndtest_config_set(struct ndtest_dimm *p, unsigned int buf_len,
>  			     struct nd_cmd_set_config_hdr *hdr)
>  {
>  	unsigned int len;
> +	u32 *status;
>  
> -	if ((hdr->in_offset + hdr->in_length) > LABEL_SIZE)
> +	if (buf_len < sizeof(*hdr) + sizeof(*status) ||
> +	    hdr->in_length > buf_len - sizeof(*hdr) - sizeof(*status))
>  		return -EINVAL;
>  
> +	status = (void *)hdr + sizeof(*hdr) + hdr->in_length;

Will that status filed be naturally aligned?  Should this use
put_aligned()?



> +	if (hdr->in_offset > LABEL_SIZE ||
> +	    hdr->in_length > LABEL_SIZE - hdr->in_offset) {
> +		*status = -EINVAL;
> +		return 0;
> +	}
> +
> +	*status = 0;
>  	len = min(hdr->in_length, LABEL_SIZE - hdr->in_offset);

Like above, min is needless.


>  	memcpy(p->label_area + hdr->in_offset, hdr->in_buf, len);
>  
> -- 
> 2.43.0
> 
>