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 <[email protected]> > --- > 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 > >
