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

[Alison Schofield <alison.schofield@intel.com>]

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
> 
>