diff mbox series

[v2] Input: try trimming too long modalias strings

Message ID ZjAWMQCJdrxZkvkB@google.com
State Accepted
Commit c1307f8a152ac69f7efb759edfb8d71b4aa228f4
Headers show
Series [v2] Input: try trimming too long modalias strings | expand

Commit Message

Dmitry Torokhov April 29, 2024, 9:50 p.m. UTC
If an input device declares too many capability bits then modalias
string for such device may become too long and not fit into uevent
buffer, resulting in failure of sending said uevent. This, in turn,
may prevent userspace from recognizing existence of such devices.

This is typically not a concern for real hardware devices as they have
limited number of keys, but happen with synthetic devices such as
ones created by xen-kbdfront driver, which creates devices as being
capable of delivering all possible keys, since it doesn't know what
keys the backend may produce.

To deal with such devices input core will attempt to trim key data,
in the hope that the rest of modalias string will fit in the given
buffer. When trimming key data it will indicate that it is not
complete by placing "+," sign, resulting in conversions like this:

old: k71,72,73,74,78,7A,7B,7C,7D,8E,9E,A4,AD,E0,E1,E4,F8,174,
new: k71,72,73,74,78,7A,7B,7C,+,

This should allow existing udev rules continue to work with existing
devices, and will also allow writing more complex rules that would
recognize trimmed modalias and check input device characteristics by
other means (for example by parsing KEY= data in uevent or parsing
input device sysfs attributes).

Note that the driver core may try adding more uevent environment
variables once input core is done adding its own, so when forming
modalias we can not use the entire available buffer, so we reduce
it by somewhat an arbitrary amount (96 bytes).

Reported-by: Jason Andryuk <jandryuk@gmail.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---

v2: do not use entire available buffer when formatting modalias, leave
    some space for driver core to add more data.

 drivers/input/input.c | 108 +++++++++++++++++++++++++++++++++++-------
 1 file changed, 91 insertions(+), 17 deletions(-)

Comments

Jason Andryuk April 30, 2024, 10:25 p.m. UTC | #1
On Mon, Apr 29, 2024 at 9:04 PM Jason Andryuk <jandryuk@gmail.com> wrote:
>
> On Mon, Apr 29, 2024 at 5:50 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > If an input device declares too many capability bits then modalias
> > string for such device may become too long and not fit into uevent
> > buffer, resulting in failure of sending said uevent. This, in turn,
> > may prevent userspace from recognizing existence of such devices.
> >
> > This is typically not a concern for real hardware devices as they have
> > limited number of keys, but happen with synthetic devices such as
> > ones created by xen-kbdfront driver, which creates devices as being
> > capable of delivering all possible keys, since it doesn't know what
> > keys the backend may produce.
> >
> > To deal with such devices input core will attempt to trim key data,
> > in the hope that the rest of modalias string will fit in the given
> > buffer. When trimming key data it will indicate that it is not
> > complete by placing "+," sign, resulting in conversions like this:
> >
> > old: k71,72,73,74,78,7A,7B,7C,7D,8E,9E,A4,AD,E0,E1,E4,F8,174,
> > new: k71,72,73,74,78,7A,7B,7C,+,
> >
> > This should allow existing udev rules continue to work with existing
> > devices, and will also allow writing more complex rules that would
> > recognize trimmed modalias and check input device characteristics by
> > other means (for example by parsing KEY= data in uevent or parsing
> > input device sysfs attributes).
> >
> > Note that the driver core may try adding more uevent environment
> > variables once input core is done adding its own, so when forming
> > modalias we can not use the entire available buffer, so we reduce
> > it by somewhat an arbitrary amount (96 bytes).
> >
> > Reported-by: Jason Andryuk <jandryuk@gmail.com>
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>
> Tested-by: Jason Andryuk <jandryuk@gmail.com>
>
> I don't have the gdm setup available to test, but loginctl looks good
> showing the Xen Virtual Keyboard assigned to a seat:
> # loginctl seat-status seat0
> seat0
>          Devices:
>                   ├─/sys/devices/LNXSYSTM:00/LNXPWRBN:00/input/input0
>                   │ input:input0 "Power Button"
>                   ├─/sys/devices/LNXSYSTM:00/LNXSLPBN:00/input/input1
>                   │ input:input1 "Sleep Button"
>                   ├─/sys/devices/platform/i8042/serio0/input/input2
>                   │ input:input2 "AT Translated Set 2 keyboard"
>                   ├─/sys/devices/platform/i8042/serio1/input/input4
>                   │ input:input4 "ImExPS/2 Generic Explorer Mouse"
>                   ├─/sys/devices/virtual/input/input5
>                   │ input:input5 "Xen Virtual Keyboard"
>                   │ └─/sys/devices/virtual/input/input5/event4
>                   │   input:event4
>                   └─/sys/devices/virtual/input/input6
>                     input:input6 "Xen Virtual Pointer"

What do you think about Cc: stable@vger.kernel.org?  I'd like to get
the Xen Keyboard working as widely as possible, so I'd like it
backported if possible.

Thanks,
Jason
Peter Hutterer May 1, 2024, 3:59 a.m. UTC | #2
On Mon, Apr 29, 2024 at 02:50:41PM -0700, Dmitry Torokhov wrote:
> If an input device declares too many capability bits then modalias
> string for such device may become too long and not fit into uevent
> buffer, resulting in failure of sending said uevent. This, in turn,
> may prevent userspace from recognizing existence of such devices.
> 
> This is typically not a concern for real hardware devices as they have
> limited number of keys, but happen with synthetic devices such as
> ones created by xen-kbdfront driver, which creates devices as being
> capable of delivering all possible keys, since it doesn't know what
> keys the backend may produce.
> 
> To deal with such devices input core will attempt to trim key data,
> in the hope that the rest of modalias string will fit in the given
> buffer. When trimming key data it will indicate that it is not
> complete by placing "+," sign, resulting in conversions like this:
> 
> old: k71,72,73,74,78,7A,7B,7C,7D,8E,9E,A4,AD,E0,E1,E4,F8,174,
> new: k71,72,73,74,78,7A,7B,7C,+,
> 
> This should allow existing udev rules continue to work with existing
> devices, and will also allow writing more complex rules that would
> recognize trimmed modalias and check input device characteristics by
> other means (for example by parsing KEY= data in uevent or parsing
> input device sysfs attributes).
> 
> Note that the driver core may try adding more uevent environment
> variables once input core is done adding its own, so when forming
> modalias we can not use the entire available buffer, so we reduce
> it by somewhat an arbitrary amount (96 bytes).
> 
> Reported-by: Jason Andryuk <jandryuk@gmail.com>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> 
> v2: do not use entire available buffer when formatting modalias, leave
>     some space for driver core to add more data.

ftr, there's nothing in the projects I maintain that require those bits
of the modalias, so I'm good :) I'm not aware of any parsers that would
struggle with the + sign here. git grep of systemd doesn't show anything
either, so I think we're good.

Took me an embarrassingly long time to wrap my head around the code but 
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>

Cheers,
  Peter

> 
>  drivers/input/input.c | 108 +++++++++++++++++++++++++++++++++++-------
>  1 file changed, 91 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/input/input.c b/drivers/input/input.c
> index b04bcdeee557..045f4b62088a 100644
> --- a/drivers/input/input.c
> +++ b/drivers/input/input.c
> @@ -1338,19 +1338,19 @@ static int input_print_modalias_bits(char *buf, int size,
>  				     char name, const unsigned long *bm,
>  				     unsigned int min_bit, unsigned int max_bit)
>  {
> -	int len = 0, i;
> +	int bit = min_bit;
> +	int len = 0;
>  
>  	len += snprintf(buf, max(size, 0), "%c", name);
> -	for (i = min_bit; i < max_bit; i++)
> -		if (bm[BIT_WORD(i)] & BIT_MASK(i))
> -			len += snprintf(buf + len, max(size - len, 0), "%X,", i);
> +	for_each_set_bit_from(bit, bm, max_bit)
> +		len += snprintf(buf + len, max(size - len, 0), "%X,", bit);
>  	return len;
>  }
>  
> -static int input_print_modalias(char *buf, int size, const struct input_dev *id,
> -				int add_cr)
> +static int input_print_modalias_parts(char *buf, int size, int full_len,
> +				      const struct input_dev *id)
>  {
> -	int len;
> +	int len, klen, remainder, space;
>  
>  	len = snprintf(buf, max(size, 0),
>  		       "input:b%04Xv%04Xp%04Xe%04X-",
> @@ -1359,8 +1359,48 @@ static int input_print_modalias(char *buf, int size, const struct input_dev *id,
>  
>  	len += input_print_modalias_bits(buf + len, size - len,
>  				'e', id->evbit, 0, EV_MAX);
> -	len += input_print_modalias_bits(buf + len, size - len,
> +
> +	/*
> +	 * Calculate the remaining space in the buffer making sure we
> +	 * have place for the terminating 0.
> +	 */
> +	space = max(size - (len + 1), 0);
> +
> +	klen = input_print_modalias_bits(buf + len, size - len,
>  				'k', id->keybit, KEY_MIN_INTERESTING, KEY_MAX);
> +	len += klen;
> +
> +	/*
> +	 * If we have more data than we can fit in the buffer, check
> +	 * if we can trim key data to fit in the rest. We will indicate
> +	 * that key data is incomplete by adding "+" sign at the end, like
> +	 * this: * "k1,2,3,45,+,".
> +	 *
> +	 * Note that we shortest key info (if present) is "k+," so we
> +	 * can only try to trim if key data is longer than that.
> +	 */
> +	if (full_len && size < full_len + 1 && klen > 3) {
> +		remainder = full_len - len;
> +		/*
> +		 * We can only trim if we have space for the remainder
> +		 * and also for at least "k+," which is 3 more characters.
> +		 */
> +		if (remainder <= space - 3) {
> +			/*
> +			 * We are guaranteed to have 'k' in the buffer, so
> +			 * we need at least 3 additional bytes for storing
> +			 * "+," in addition to the remainder.
> +			 */
> +			for (int i = size - 1 - remainder - 3; i >= 0; i--) {
> +				if (buf[i] == 'k' || buf[i] == ',') {
> +					strcpy(buf + i + 1, "+,");
> +					len = i + 3; /* Not counting '\0' */
> +					break;
> +				}
> +			}
> +		}
> +	}
> +
>  	len += input_print_modalias_bits(buf + len, size - len,
>  				'r', id->relbit, 0, REL_MAX);
>  	len += input_print_modalias_bits(buf + len, size - len,
> @@ -1376,22 +1416,37 @@ static int input_print_modalias(char *buf, int size, const struct input_dev *id,
>  	len += input_print_modalias_bits(buf + len, size - len,
>  				'w', id->swbit, 0, SW_MAX);
>  
> -	if (add_cr)
> -		len += snprintf(buf + len, max(size - len, 0), "\n");
> -
>  	return len;
>  }
>  
> +static int input_print_modalias(char *buf, int size, const struct input_dev *id)
> +{
> +	int full_len;
> +
> +	/*
> +	 * Printing is done in 2 passes: first one figures out total length
> +	 * needed for the modalias string, second one will try to trim key
> +	 * data in case when buffer is too small for the entire modalias.
> +	 * If the buffer is too small regardless, it will fill as much as it
> +	 * can (without trimming key data) into the buffer and leave it to
> +	 * the caller to figure out what to do with the result.
> +	 */
> +	full_len = input_print_modalias_parts(NULL, 0, 0, id);
> +	return input_print_modalias_parts(buf, size, full_len, id);
> +}
> +
>  static ssize_t input_dev_show_modalias(struct device *dev,
>  				       struct device_attribute *attr,
>  				       char *buf)
>  {
>  	struct input_dev *id = to_input_dev(dev);
> -	ssize_t len;
> +	size_t len;
>  
> -	len = input_print_modalias(buf, PAGE_SIZE, id, 1);
> +	len = input_print_modalias(buf, PAGE_SIZE, id);
> +	if (len < PAGE_SIZE - 2)
> +		len += snprintf(buf + len, PAGE_SIZE - len, "\n");
>  
> -	return min_t(int, len, PAGE_SIZE);
> +	return min(len, PAGE_SIZE);
>  }
>  static DEVICE_ATTR(modalias, S_IRUGO, input_dev_show_modalias, NULL);
>  
> @@ -1601,6 +1656,23 @@ static int input_add_uevent_bm_var(struct kobj_uevent_env *env,
>  	return 0;
>  }
>  
> +/*
> + * This is a pretty gross hack. When building uevent data the driver core
> + * may try adding more environment variables to kobj_uevent_env without
> + * telling us, so we have no idea how much of the buffer we can use to
> + * avoid overflows/-ENOMEM elsewhere. To work around this let's artificially
> + * reduce amount of memory we will use for the modalias environment variable.
> + *
> + * The potential additions are:
> + *
> + * SEQNUM=18446744073709551615 - (%llu - 28 bytes)
> + * HOME=/ (6 bytes)
> + * PATH=/sbin:/bin:/usr/sbin:/usr/bin (34 bytes)
> + *
> + * 68 bytes total. Allow extra buffer - 96 bytes
> + */
> +#define UEVENT_ENV_EXTRA_LEN	96
> +
>  static int input_add_uevent_modalias_var(struct kobj_uevent_env *env,
>  					 const struct input_dev *dev)
>  {
> @@ -1610,9 +1682,11 @@ static int input_add_uevent_modalias_var(struct kobj_uevent_env *env,
>  		return -ENOMEM;
>  
>  	len = input_print_modalias(&env->buf[env->buflen - 1],
> -				   sizeof(env->buf) - env->buflen,
> -				   dev, 0);
> -	if (len >= (sizeof(env->buf) - env->buflen))
> +				   (int)sizeof(env->buf) - env->buflen -
> +					UEVENT_ENV_EXTRA_LEN,
> +				   dev);
> +	if (len >= ((int)sizeof(env->buf) - env->buflen -
> +					UEVENT_ENV_EXTRA_LEN))
>  		return -ENOMEM;
>  
>  	env->buflen += len;
> -- 
> 2.44.0.769.g3c40516874-goog
> 
> 
> -- 
> Dmitry
Dmitry Torokhov May 1, 2024, 7:11 p.m. UTC | #3
On Tue, Apr 30, 2024 at 06:25:13PM -0400, Jason Andryuk wrote:
> On Mon, Apr 29, 2024 at 9:04 PM Jason Andryuk <jandryuk@gmail.com> wrote:
> >
> > On Mon, Apr 29, 2024 at 5:50 PM Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > >
> > > If an input device declares too many capability bits then modalias
> > > string for such device may become too long and not fit into uevent
> > > buffer, resulting in failure of sending said uevent. This, in turn,
> > > may prevent userspace from recognizing existence of such devices.
> > >
> > > This is typically not a concern for real hardware devices as they have
> > > limited number of keys, but happen with synthetic devices such as
> > > ones created by xen-kbdfront driver, which creates devices as being
> > > capable of delivering all possible keys, since it doesn't know what
> > > keys the backend may produce.
> > >
> > > To deal with such devices input core will attempt to trim key data,
> > > in the hope that the rest of modalias string will fit in the given
> > > buffer. When trimming key data it will indicate that it is not
> > > complete by placing "+," sign, resulting in conversions like this:
> > >
> > > old: k71,72,73,74,78,7A,7B,7C,7D,8E,9E,A4,AD,E0,E1,E4,F8,174,
> > > new: k71,72,73,74,78,7A,7B,7C,+,
> > >
> > > This should allow existing udev rules continue to work with existing
> > > devices, and will also allow writing more complex rules that would
> > > recognize trimmed modalias and check input device characteristics by
> > > other means (for example by parsing KEY= data in uevent or parsing
> > > input device sysfs attributes).
> > >
> > > Note that the driver core may try adding more uevent environment
> > > variables once input core is done adding its own, so when forming
> > > modalias we can not use the entire available buffer, so we reduce
> > > it by somewhat an arbitrary amount (96 bytes).
> > >
> > > Reported-by: Jason Andryuk <jandryuk@gmail.com>
> > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> >
> > Tested-by: Jason Andryuk <jandryuk@gmail.com>
> >
> > I don't have the gdm setup available to test, but loginctl looks good
> > showing the Xen Virtual Keyboard assigned to a seat:
> > # loginctl seat-status seat0
> > seat0
> >          Devices:
> >                   ├─/sys/devices/LNXSYSTM:00/LNXPWRBN:00/input/input0
> >                   │ input:input0 "Power Button"
> >                   ├─/sys/devices/LNXSYSTM:00/LNXSLPBN:00/input/input1
> >                   │ input:input1 "Sleep Button"
> >                   ├─/sys/devices/platform/i8042/serio0/input/input2
> >                   │ input:input2 "AT Translated Set 2 keyboard"
> >                   ├─/sys/devices/platform/i8042/serio1/input/input4
> >                   │ input:input4 "ImExPS/2 Generic Explorer Mouse"
> >                   ├─/sys/devices/virtual/input/input5
> >                   │ input:input5 "Xen Virtual Keyboard"
> >                   │ └─/sys/devices/virtual/input/input5/event4
> >                   │   input:event4
> >                   └─/sys/devices/virtual/input/input6
> >                     input:input6 "Xen Virtual Pointer"
> 
> What do you think about Cc: stable@vger.kernel.org?  I'd like to get
> the Xen Keyboard working as widely as possible, so I'd like it
> backported if possible.

I am open to it, but I'd like Benjamin/Hans to take a look at this
as well (I see Peter already gave his Reviewed-by).

Thanks.
diff mbox series

Patch

diff --git a/drivers/input/input.c b/drivers/input/input.c
index b04bcdeee557..045f4b62088a 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -1338,19 +1338,19 @@  static int input_print_modalias_bits(char *buf, int size,
 				     char name, const unsigned long *bm,
 				     unsigned int min_bit, unsigned int max_bit)
 {
-	int len = 0, i;
+	int bit = min_bit;
+	int len = 0;
 
 	len += snprintf(buf, max(size, 0), "%c", name);
-	for (i = min_bit; i < max_bit; i++)
-		if (bm[BIT_WORD(i)] & BIT_MASK(i))
-			len += snprintf(buf + len, max(size - len, 0), "%X,", i);
+	for_each_set_bit_from(bit, bm, max_bit)
+		len += snprintf(buf + len, max(size - len, 0), "%X,", bit);
 	return len;
 }
 
-static int input_print_modalias(char *buf, int size, const struct input_dev *id,
-				int add_cr)
+static int input_print_modalias_parts(char *buf, int size, int full_len,
+				      const struct input_dev *id)
 {
-	int len;
+	int len, klen, remainder, space;
 
 	len = snprintf(buf, max(size, 0),
 		       "input:b%04Xv%04Xp%04Xe%04X-",
@@ -1359,8 +1359,48 @@  static int input_print_modalias(char *buf, int size, const struct input_dev *id,
 
 	len += input_print_modalias_bits(buf + len, size - len,
 				'e', id->evbit, 0, EV_MAX);
-	len += input_print_modalias_bits(buf + len, size - len,
+
+	/*
+	 * Calculate the remaining space in the buffer making sure we
+	 * have place for the terminating 0.
+	 */
+	space = max(size - (len + 1), 0);
+
+	klen = input_print_modalias_bits(buf + len, size - len,
 				'k', id->keybit, KEY_MIN_INTERESTING, KEY_MAX);
+	len += klen;
+
+	/*
+	 * If we have more data than we can fit in the buffer, check
+	 * if we can trim key data to fit in the rest. We will indicate
+	 * that key data is incomplete by adding "+" sign at the end, like
+	 * this: * "k1,2,3,45,+,".
+	 *
+	 * Note that we shortest key info (if present) is "k+," so we
+	 * can only try to trim if key data is longer than that.
+	 */
+	if (full_len && size < full_len + 1 && klen > 3) {
+		remainder = full_len - len;
+		/*
+		 * We can only trim if we have space for the remainder
+		 * and also for at least "k+," which is 3 more characters.
+		 */
+		if (remainder <= space - 3) {
+			/*
+			 * We are guaranteed to have 'k' in the buffer, so
+			 * we need at least 3 additional bytes for storing
+			 * "+," in addition to the remainder.
+			 */
+			for (int i = size - 1 - remainder - 3; i >= 0; i--) {
+				if (buf[i] == 'k' || buf[i] == ',') {
+					strcpy(buf + i + 1, "+,");
+					len = i + 3; /* Not counting '\0' */
+					break;
+				}
+			}
+		}
+	}
+
 	len += input_print_modalias_bits(buf + len, size - len,
 				'r', id->relbit, 0, REL_MAX);
 	len += input_print_modalias_bits(buf + len, size - len,
@@ -1376,22 +1416,37 @@  static int input_print_modalias(char *buf, int size, const struct input_dev *id,
 	len += input_print_modalias_bits(buf + len, size - len,
 				'w', id->swbit, 0, SW_MAX);
 
-	if (add_cr)
-		len += snprintf(buf + len, max(size - len, 0), "\n");
-
 	return len;
 }
 
+static int input_print_modalias(char *buf, int size, const struct input_dev *id)
+{
+	int full_len;
+
+	/*
+	 * Printing is done in 2 passes: first one figures out total length
+	 * needed for the modalias string, second one will try to trim key
+	 * data in case when buffer is too small for the entire modalias.
+	 * If the buffer is too small regardless, it will fill as much as it
+	 * can (without trimming key data) into the buffer and leave it to
+	 * the caller to figure out what to do with the result.
+	 */
+	full_len = input_print_modalias_parts(NULL, 0, 0, id);
+	return input_print_modalias_parts(buf, size, full_len, id);
+}
+
 static ssize_t input_dev_show_modalias(struct device *dev,
 				       struct device_attribute *attr,
 				       char *buf)
 {
 	struct input_dev *id = to_input_dev(dev);
-	ssize_t len;
+	size_t len;
 
-	len = input_print_modalias(buf, PAGE_SIZE, id, 1);
+	len = input_print_modalias(buf, PAGE_SIZE, id);
+	if (len < PAGE_SIZE - 2)
+		len += snprintf(buf + len, PAGE_SIZE - len, "\n");
 
-	return min_t(int, len, PAGE_SIZE);
+	return min(len, PAGE_SIZE);
 }
 static DEVICE_ATTR(modalias, S_IRUGO, input_dev_show_modalias, NULL);
 
@@ -1601,6 +1656,23 @@  static int input_add_uevent_bm_var(struct kobj_uevent_env *env,
 	return 0;
 }
 
+/*
+ * This is a pretty gross hack. When building uevent data the driver core
+ * may try adding more environment variables to kobj_uevent_env without
+ * telling us, so we have no idea how much of the buffer we can use to
+ * avoid overflows/-ENOMEM elsewhere. To work around this let's artificially
+ * reduce amount of memory we will use for the modalias environment variable.
+ *
+ * The potential additions are:
+ *
+ * SEQNUM=18446744073709551615 - (%llu - 28 bytes)
+ * HOME=/ (6 bytes)
+ * PATH=/sbin:/bin:/usr/sbin:/usr/bin (34 bytes)
+ *
+ * 68 bytes total. Allow extra buffer - 96 bytes
+ */
+#define UEVENT_ENV_EXTRA_LEN	96
+
 static int input_add_uevent_modalias_var(struct kobj_uevent_env *env,
 					 const struct input_dev *dev)
 {
@@ -1610,9 +1682,11 @@  static int input_add_uevent_modalias_var(struct kobj_uevent_env *env,
 		return -ENOMEM;
 
 	len = input_print_modalias(&env->buf[env->buflen - 1],
-				   sizeof(env->buf) - env->buflen,
-				   dev, 0);
-	if (len >= (sizeof(env->buf) - env->buflen))
+				   (int)sizeof(env->buf) - env->buflen -
+					UEVENT_ENV_EXTRA_LEN,
+				   dev);
+	if (len >= ((int)sizeof(env->buf) - env->buflen -
+					UEVENT_ENV_EXTRA_LEN))
 		return -ENOMEM;
 
 	env->buflen += len;