xenbus_directory() gets a reply buffer that xenbus_comms.c
null-terminates at body[len]. split_strings() counts strings using
that terminator, but then copies only len bytes into its combined
pointer/string allocation.

If a malformed or unexpected directory reply does not carry a final NUL
within the advertised length, the relocated last string is left
unterminated. Callers then treat the entries as C strings and can read
past the allocation.

Allocate and copy the transport-added terminator as part of the
relocated string block. This preserves current parsing behavior while
keeping every returned entry NUL-terminated.

Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>
---
 drivers/xen/xenbus/xenbus_xs.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/xen/xenbus/xenbus_xs.c b/drivers/xen/xenbus/xenbus_xs.c
index c202e7c55..05b758fff 100644
--- a/drivers/xen/xenbus/xenbus_xs.c
+++ b/drivers/xen/xenbus/xenbus_xs.c
@@ -420,13 +420,13 @@ static char **split_strings(char *strings, unsigned int len, unsigned int *num)
 	/* Count the strings. */
 	*num = count_strings(strings, len);
 
-	/* Transfer to one big alloc for easy freeing. */
-	ret = kmalloc(*num * sizeof(char *) + len, GFP_NOIO | __GFP_HIGH);
+	/* Transfer to one big alloc for easy freeing. Keep the extra NUL. */
+	ret = kmalloc(*num * sizeof(char *) + len + 1, GFP_NOIO | __GFP_HIGH);
 	if (!ret) {
 		kfree(strings);
 		return ERR_PTR(-ENOMEM);
 	}
-	memcpy(&ret[*num], strings, len);
+	memcpy(&ret[*num], strings, len + 1);
 	kfree(strings);
 
 	strings = (char *)&ret[*num];
-- 
2.54.0

On 24.06.26 14:42, Yousef Alhouseen wrote:
> xenbus_directory() gets a reply buffer that xenbus_comms.c
> null-terminates at body[len]. split_strings() counts strings using
> that terminator, but then copies only len bytes into its combined
> pointer/string allocation.
> 
> If a malformed or unexpected directory reply does not carry a final NUL
> within the advertised length, the relocated last string is left
> unterminated. Callers then treat the entries as C strings and can read
> past the allocation.

This would be a major bug in a trusted component (Xenstore).

> Allocate and copy the transport-added terminator as part of the
> relocated string block. This preserves current parsing behavior while
> keeping every returned entry NUL-terminated.

I'd rather see a check that the last byte of the reply is indeed a NUL byte.
In case it is not NUL, then the reply is most certainly nonsense anyway and
a fat pr_err_once() is wanted plus "return ERR_PTR(-EIO);"


Juergen

split_strings() walks each directory entry with strlen(). Although the
transport adds a terminator after the reply buffer, a malformed reply
without a final NUL inside its advertised length would let that walk
cross the protocol payload boundary.

Reject such replies before counting the strings. Report the protocol
violation once and return -EIO to the caller.

Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>
---
Changes in v2:
- Reject malformed replies instead of copying the transport-added
  terminator, as suggested by Juergen Gross.

 drivers/xen/xenbus/xenbus_xs.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/xen/xenbus/xenbus_xs.c b/drivers/xen/xenbus/xenbus_xs.c
index c202e7c55..d1cca4acb 100644
--- a/drivers/xen/xenbus/xenbus_xs.c
+++ b/drivers/xen/xenbus/xenbus_xs.c
@@ -417,6 +417,12 @@ static char **split_strings(char *strings, unsigned int len, unsigned int *num)
 {
 	char *p, **ret;
 
+	if (len && strings[len - 1]) {
+		pr_err_once("malformed XS_DIRECTORY reply\n");
+		kfree(strings);
+		return ERR_PTR(-EIO);
+	}
+
 	/* Count the strings. */
 	*num = count_strings(strings, len);
 
-- 
2.54.0

On 27.06.26 00:37, Yousef Alhouseen wrote:
> split_strings() walks each directory entry with strlen(). Although the
> transport adds a terminator after the reply buffer, a malformed reply
> without a final NUL inside its advertised length would let that walk
> cross the protocol payload boundary.
> 
> Reject such replies before counting the strings. Report the protocol
> violation once and return -EIO to the caller.
> 
> Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen