Received-SPF: pass (google.com: domain of linux-kernel+bounces-164889-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Feedback-ID: i7ce144cd:Fastmail
Date: Wed, 1 May 2024 13:59:02 +1000
From: Peter Hutterer <peter.hutterer@who-t.net>
To: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: linux-input@vger.kernel.org, Jason Andryuk <jandryuk@gmail.com>,
	Hans de Goede <hdegoede@redhat.com>,
	Benjamin Tissoires <benjamin.tissoires@redhat.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] Input: try trimming too long modalias strings
Message-ID: <20240501035902.GA219581@quokka>
References: <ZjAWMQCJdrxZkvkB@google.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZjAWMQCJdrxZkvkB@google.com>

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