Re: Diff for evaluation (WACOM tablet driver)
On Sun, Jun 18, 2023 at 09:36:25AM +, Vladimir Meshcheriakov wrote:
> Good day,
>
> I am currently trying to work on an implementation
> of a driver for the WACOM tablet on openBSD
> I am therefore submitting this diff so that it could potentially be evaluated.
> Please if you have a moment, could you have a look at this diff?
> I have tested it with my Wacom tablet
> and it seems to work correctly,
> the coding style is normally respected,
> but I apologize in advance if my keen eyes have missed out something.
Hi there! i tested the diff with a wacom intuos s
(CS-4100). It attaches sucessfully
uhidev1 at uhub1 port 1 configuration 1 interface 0 "Wacom Co.,Ltd. Intuos S"
rev 2.00/1.11 addr 5
uhidev1: iclass 3/0, 228 report ids
uwacom0 at uhidev1: 9 buttons, Z and W dir, tip, barrel
wsmouse3 at uwacom0 mux 0
uwacom1 at uhidev1: 9 buttons, Z and W dir, tip, barrel
wsmouse4 at uwacom1 mux 0
uwacom2 at uhidev1: 9 buttons, Z and W dir, tip, barrel
wsmouse5 at uwacom2 mux 0
I don't have any of the previous wacom products to see
if they still work. One thing i am trying to do is
to somehow scale the tablet so that it is actually
usable. I looked a bit into running xtsscale
but it outputs
xtsscale -v -d /dev/wsmouse1
XRandR extension version 1.6 present
Unable to find the "WS Pointer Axis Calibration" device property.
There are probably no calibrable devices on this system.
Finally in the patch it seems there is another rogue
usbdevs entry (a GAOMON M10k device).
Thanks for taking the time to write this.
>
> diff --git a/sys/dev/hid/hid.c b/sys/dev/hid/hid.c
> index c758764f17a..20c0c501e91 100644
> --- a/sys/dev/hid/hid.c
> +++ b/sys/dev/hid/hid.c
> @@ -657,3 +657,49 @@ hid_is_collection(const void *desc, int size, uint8_t
> id, int32_t usage)
> hid_end_parse(hd);
> return (0);
> }
> +
> +struct hid_data *
> +hid_get_collection_data(const void *desc, int size, int32_t usage, uint32_t
> collection)
> +{
> + struct hid_data *hd;
> + struct hid_item hi;
> +
> + hd = hid_start_parse(desc, size, hid_all);
> +
> + DPRINTF("%s: usage=0x%x\n", __func__, usage);
> + while (hid_get_item(hd, )) {
> + DPRINTF("%s: kind=%d id=%d usage=0x%x(0x%x)\n", __func__,
> + hi.kind, hi.report_ID, hi.usage, usage);
> + if (hi.kind == hid_collection &&
> + hi.collection == collection && hi.usage == usage){
> + DPRINTF("%s: found\n", __func__);
> + return hd;
> + }
> + }
> + DPRINTF("%s: not found\n", __func__);
> + hid_end_parse(hd);
> + return NULL;
> +}
> +
> +int
> +hid_get_id_of_collection(const void *desc, int size, int32_t usage, uint32_t
> collection)
> +{
> + struct hid_data *hd;
> + struct hid_item hi;
> +
> + hd = hid_start_parse(desc, size, hid_all);
> +
> + DPRINTF("%s: id=%d usage=0x%x\n", __func__, id, usage);
> + while (hid_get_item(hd, )) {
> + DPRINTF("%s: kind=%d id=%d usage=0x%x(0x%x)\n", __func__,
> + hi.kind, hi.report_ID, hi.usage, usage);
> + if (hi.kind == hid_collection &&
> + hi.collection == collection && hi.usage == usage){
> + DPRINTF("%s: found\n", __func__);
> + return hi.report_ID;
> + }
> + }
> + DPRINTF("%s: not found\n", __func__);
> + hid_end_parse(hd);
> + return 0;
> +}
> diff --git a/sys/dev/hid/hid.h b/sys/dev/hid/hid.h
> index 7400e920bc2..78bc4c403c5 100644
> --- a/sys/dev/hid/hid.h
> +++ b/sys/dev/hid/hid.h
> @@ -93,6 +93,8 @@ int hid_locate(const void *, int, int32_t, uint8_t, enum
> hid_kind,
> int32_t hid_get_data(const uint8_t *buf, int, struct hid_location *);
> uint32_t hid_get_udata(const uint8_t *buf, int, struct hid_location *);
> int hid_is_collection(const void *, int, uint8_t, int32_t);
> +struct hid_data *hid_get_collection_data(const void *, int, int32_t,
> uint32_t);
> +int hid_get_id_of_collection(const void *desc, int size, int32_t usage,
> uint32_t collection);
>
> #endif /* _KERNEL */
>
> @@ -353,6 +355,7 @@ int hid_is_collection(const void *, int, uint8_t,
> int32_t);
> #define HUD_TOUCHSCREEN 0x0004
> #define HUD_TOUCHPAD 0x0005
> #define HUD_CONFIG 0x000e
> +#define HUD_STYLUS 0x0020
> #define HUD_FINGER 0x0022
> #define HUD_TIP_PRESSURE 0x0030
> #define HUD_BARREL_PRESSURE 0x0031
> @@ -387,6 +390,12 @@ int hid_is_collection(const void *, int, uint8_t,
> int32_t);
> #define HUD_CONTACT_MAX 0x0055
> #define HUD_SCAN_TIME0x0056
> #define HUD_BUTTON_TYPE 0x0059
> +#define HUD_SECONDARY_BARREL_SWITCH 0x005A
> +#define HUD_WACOM_X 0x0130
> +#define HUD_WACOM_Y 0x0131
> +#define HUD_WACOM_DISTANCE 0x0132
> +#define HUD_WACOM_PAD_BUTTONS00 0x0910
> +#define
Re: httpd: use the host name in SERVER_NAME
On 2023/06/30 15:29:07 +0200, Omar Polo wrote:
> duh. sorry for the dumb question, it was obvious. Here's a better
> diff.
>
> I've made strictier the syntax checks for IPv6 (after the closing ']'
> there's the optional port but then nothing else) and joined together
> the cases for the host:port or hostname alone to simplify the validity
> check.
>
> last thing I'm unsure if whether we should log as-is the original Host
> in case of error, as it can contain anything.
>
> Thanks!
now with a 100% correct return value in the ipv6 case. The memmove
rewinds buf while start points at buf+1. i've called getaddrinfo()
with buf, but we still return `start', so that needs to be fixed.
diff /usr/src
commit - 9832f5035d799ae12e9f848cff5650481b673260
path + /usr/src
blob - 091cc86fec946a4a9d422c04a48f5cbfae015ce0
file + usr.sbin/httpd/server_http.c
--- usr.sbin/httpd/server_http.c
+++ usr.sbin/httpd/server_http.c
@@ -23,6 +23,7 @@
#include
#include
+#include
#include
#include
@@ -831,11 +832,49 @@ char *
return (buf);
}
+static int
+valid_domain(char *name, const char **errstr)
+{
+ size_t i;
+ unsigned char c, last = '\0';
+
+ *errstr = NULL;
+
+ if (*name == '\0') {
+ *errstr = "empty domain name";
+ return 0;
+ }
+ if (!isalpha((unsigned char)name[0]) &&
+ !isdigit((unsigned char)name[0])) {
+ *errstr = "domain name starts with an invalid character";
+ return 0;
+ }
+ for (i = 0; name[i] != '\0'; ++i) {
+ c = tolower((unsigned char)name[i]);
+ name[i] = c;
+ if (last == '.' && c == '.') {
+ *errstr = "domain name contains consecutive separators";
+ return 0;
+ }
+ if (c != '.' && c != '-' && !isalnum(c) &&
+ c != '_') /* technically invalid, but common */ {
+ *errstr = "domain name contains invalid characters";
+ return 0;
+ }
+ last = c;
+ }
+ if (name[i - 1] == '.')
+ name[i - 1] = '\0';
+ return 1;
+}
+
char *
server_http_parsehost(char *host, char *buf, size_t len, int *portval)
{
+ struct addrinfo hints, *ai;
char*start, *end, *port;
const char *errstr = NULL;
+ int err;
if (strlcpy(buf, host, len) >= len) {
log_debug("%s: host name too long", __func__);
@@ -849,18 +888,38 @@ server_http_parsehost(char *host, char *buf, size_t le
/* Address enclosed in [] with port, eg. [2001:db8::1]:80 */
start++;
*end++ = '\0';
- if ((port = strchr(end, ':')) == NULL || *port == '\0')
- port = NULL;
- else
- port++;
+ if (*end == ':') {
+ *end++ = '\0';
+ port = end;
+ } else if (*end != '\0') {
+ log_debug("%s: gibberish after IPv6 address: %s",
+ __func__, host);
+ return (NULL);
+ }
+
memmove(buf, start, strlen(start) + 1);
- } else if ((end = strchr(start, ':')) != NULL) {
- /* Name or address with port, eg. www.example.com:80 */
- *end++ = '\0';
- port = end;
+ start = buf;
+
+ memset(, 0, sizeof(hints));
+ hints.ai_flags = AI_NUMERICHOST;
+ err = getaddrinfo(start, NULL, , );
+ if (err != 0) {
+ log_debug("%s: %s: %s", __func__, gai_strerror(err),
+ host);
+ return (NULL);
+ }
+ freeaddrinfo(ai);
} else {
- /* Name or address with default port, eg. www.example.com */
- port = NULL;
+ /* Name or address with optional port */
+ if ((end = strchr(start, ':')) != NULL) {
+ *end++ = '\0';
+ port = end;
+ }
+
+ if (!valid_domain(start, )) {
+ log_debug("%s: %s: %s", __func__, errstr, host);
+ return (NULL);
+ }
}
if (port != NULL) {
Re: httpd: use the host name in SERVER_NAME
On 2023/06/30 11:14:51 +0200, Florian Obser wrote:
> On 2023-06-30 10:46 +02, Omar Polo wrote:
> > On 2023/06/29 23:43:25 +0200, Omar Polo wrote:
> >> On 2023/06/29 19:55:52 +0200, Florian Obser wrote:
> >> > I'm worried that we pass un-sanitized input through to fcgi.
> >> > Of course we are passing *a lot* of un-sanitized input through to fcgi,
> >> > so does this matter in the grand scheme of things?
> >> > But I'd like if server_http_parsehost() enforces syntactically valid
> >> > hostnames first.
> >> >
> >> > There is valid_domain() in usr.bin/ssh/misc.c or res_hnok in
> >> > resolv.h. Which is probably fine to use since we don't care too much
> >> > about portable.
> >>
> >> right, httpd is happily accepting any nonsense as Host. Here's an
> >> adaptation of millert' valid_domain() from usr.bin/ssh/misc.c.
> >> Hope noone is relying on using an empty/malformed Host :>
> >
> > actually this is wrong. it breaks when an ipv6 address is used as
> > Host. this seems to be an issue in ssh too, although I haven't
>
> IIRC ssh only uses valid_domain() when it already knows that it's not an
> IPv6 address.
% ssh ssh://[::1]
usage: ssh [-46AaCfGgKkMNnqsTtVvXxYy] [-B bind_interface]
[...]
% curl --head http://[::1]
HTTP/1.1 200 OK
[...]
but maybe I'm holding it wrong. IPv6 addresses in the HostName config
directive work fine though. `ssh ::1' also works.
> > [...]
> > should I make it strictier (not done just because it's a bit long and
> > definitely not fun to validate ipv6 addresses), or there's something
> > else in base that I can steal fom? :)
>
> getaddrinfo(3) with AI_NUMERICHOST.
duh. sorry for the dumb question, it was obvious. Here's a better
diff.
I've made strictier the syntax checks for IPv6 (after the closing ']'
there's the optional port but then nothing else) and joined together
the cases for the host:port or hostname alone to simplify the validity
check.
last thing I'm unsure if whether we should log as-is the original Host
in case of error, as it can contain anything.
Thanks!
diff /usr/src
commit - 9832f5035d799ae12e9f848cff5650481b673260
path + /usr/src
blob - 091cc86fec946a4a9d422c04a48f5cbfae015ce0
file + usr.sbin/httpd/server_http.c
--- usr.sbin/httpd/server_http.c
+++ usr.sbin/httpd/server_http.c
@@ -23,6 +23,7 @@
#include
#include
+#include
#include
#include
@@ -831,11 +832,49 @@ char *
return (buf);
}
+static int
+valid_domain(char *name, const char **errstr)
+{
+ size_t i;
+ unsigned char c, last = '\0';
+
+ *errstr = NULL;
+
+ if (*name == '\0') {
+ *errstr = "empty domain name";
+ return 0;
+ }
+ if (!isalpha((unsigned char)name[0]) &&
+ !isdigit((unsigned char)name[0])) {
+ *errstr = "domain name starts with an invalid character";
+ return 0;
+ }
+ for (i = 0; name[i] != '\0'; ++i) {
+ c = tolower((unsigned char)name[i]);
+ name[i] = c;
+ if (last == '.' && c == '.') {
+ *errstr = "domain name contains consecutive separators";
+ return 0;
+ }
+ if (c != '.' && c != '-' && !isalnum(c) &&
+ c != '_') /* technically invalid, but common */ {
+ *errstr = "domain name contains invalid characters";
+ return 0;
+ }
+ last = c;
+ }
+ if (name[i - 1] == '.')
+ name[i - 1] = '\0';
+ return 1;
+}
+
char *
server_http_parsehost(char *host, char *buf, size_t len, int *portval)
{
+ struct addrinfo hints, *ai;
char*start, *end, *port;
const char *errstr = NULL;
+ int err;
if (strlcpy(buf, host, len) >= len) {
log_debug("%s: host name too long", __func__);
@@ -849,18 +888,37 @@ server_http_parsehost(char *host, char *buf, size_t le
/* Address enclosed in [] with port, eg. [2001:db8::1]:80 */
start++;
*end++ = '\0';
- if ((port = strchr(end, ':')) == NULL || *port == '\0')
- port = NULL;
- else
- port++;
+ if (*end == ':') {
+ *end++ = '\0';
+ port = end;
+ } else if (*end != '\0') {
+ log_debug("%s: gibberish after IPv6 address: %s",
+ __func__, host);
+ return (NULL);
+ }
+
memmove(buf, start, strlen(start) + 1);
- } else if ((end = strchr(start, ':')) != NULL) {
- /* Name or address with port, eg. www.example.com:80 */
- *end++ = '\0';
- port = end;
+
+ memset(, 0, sizeof(hints));
+ hints.ai_flags =
Re: httpd: use the host name in SERVER_NAME
On 2023-06-30 10:46 +02, Omar Polo wrote:
> On 2023/06/29 23:43:25 +0200, Omar Polo wrote:
>> On 2023/06/29 19:55:52 +0200, Florian Obser wrote:
>> > I'm worried that we pass un-sanitized input through to fcgi.
>> > Of course we are passing *a lot* of un-sanitized input through to fcgi,
>> > so does this matter in the grand scheme of things?
>> > But I'd like if server_http_parsehost() enforces syntactically valid
>> > hostnames first.
>> >
>> > There is valid_domain() in usr.bin/ssh/misc.c or res_hnok in
>> > resolv.h. Which is probably fine to use since we don't care too much
>> > about portable.
>>
>> right, httpd is happily accepting any nonsense as Host. Here's an
>> adaptation of millert' valid_domain() from usr.bin/ssh/misc.c.
>> Hope noone is relying on using an empty/malformed Host :>
>
> actually this is wrong. it breaks when an ipv6 address is used as
> Host. this seems to be an issue in ssh too, although I haven't
IIRC ssh only uses valid_domain() when it already knows that it's not an
IPv6 address.
> checked if the ssh URI specification inherits the authority from
> rfc3986 as-is.
>
> here's a slightly revised version, but just for the sake of the
> discussion since its ipv6 parsing is too permissive.
>
> should I make it strictier (not done just because it's a bit long and
> definitely not fun to validate ipv6 addresses), or there's something
> else in base that I can steal fom? :)
getaddrinfo(3) with AI_NUMERICHOST.
>
> (while here I've tweaked slightly the nonsense in
> server_http_parsehost wrt ipv6 addresses and port numbers.)
>
>
> Thanks!
>
>
> diff /usr/src
> commit - 9832f5035d799ae12e9f848cff5650481b673260
> path + /usr/src
> blob - 091cc86fec946a4a9d422c04a48f5cbfae015ce0
> file + usr.sbin/httpd/server_http.c
> --- usr.sbin/httpd/server_http.c
> +++ usr.sbin/httpd/server_http.c
> @@ -831,6 +831,60 @@ char *
> return (buf);
> }
>
> +static int
> +valid_domain(char *name, const char **errstr)
> +{
> + size_t i;
> + unsigned char c, last = '\0';
> +
> + *errstr = NULL;
> +
> + if (*name == '\0') {
> + *errstr = "empty domain name";
> + return 0;
> + }
> +
> + if (name[0] == '[') {
> + for (i = 1; name[i] != '\0'; ++i) {
> + if (name[i] == ']')
> + break;
> + if (!isxdigit((unsigned char)name[i]) &&
> + name[i] != ':') {
> + *errstr = "invalid IPv6 address";
> + return 0;
> + }
> + }
> + if (name[i] != ']' || name[i + 1] != '\0') {
> + *errstr = "invalid IPv6 address";
> + return 0;
> + }
> + return 1;
> + }
> +
> + if (!isalpha((unsigned char)name[0]) &&
> + !isdigit((unsigned char)name[0])) {
> + *errstr = "domain name starts with an invalid character";
> + return 0;
> + }
> + for (i = 0; name[i] != '\0'; ++i) {
> + c = tolower((unsigned char)name[i]);
> + name[i] = c;
> + if (last == '.' && c == '.') {
> + *errstr = "domain name contains consecutive separators";
> + return 0;
> + }
> + if (c != '.' && c != '-' && !isalnum(c) &&
> + c != '_') /* technically invalid, but common */ {
> + *errstr = "domain name contains invalid characters";
> + return 0;
> + }
> + last = c;
> + }
> + if (name[i - 1] == '.')
> + name[i - 1] = '\0';
> + return 1;
> +}
> +
> char *
> server_http_parsehost(char *host, char *buf, size_t len, int *portval)
> {
> @@ -847,13 +901,11 @@ server_http_parsehost(char *host, char *buf, size_t le
>
> if (*start == '[' && (end = strchr(start, ']')) != NULL) {
> /* Address enclosed in [] with port, eg. [2001:db8::1]:80 */
> - start++;
are you forgetting to skip over the opening '['?
> - *end++ = '\0';
> - if ((port = strchr(end, ':')) == NULL || *port == '\0')
> - port = NULL;
> - else
> - port++;
> - memmove(buf, start, strlen(start) + 1);
> + end++;
> + if (*end == ':') {
> + *end++ = '\0';
> + port = end;
> + }
> } else if ((end = strchr(start, ':')) != NULL) {
> /* Name or address with port, eg. www.example.com:80 */
> *end++ = '\0';
> @@ -863,6 +915,11 @@ server_http_parsehost(char *host, char *buf, size_t le
> port = NULL;
> }
>
> + if (!valid_domain(start, )) {
> + log_debug("%s: %s: %s", __func__, errstr, host);
> + return (NULL);
> + }
> +
> if (port != NULL) {
> /* Save the
Re: httpd: use the host name in SERVER_NAME
On 2023/06/29 23:43:25 +0200, Omar Polo wrote:
> On 2023/06/29 19:55:52 +0200, Florian Obser wrote:
> > I'm worried that we pass un-sanitized input through to fcgi.
> > Of course we are passing *a lot* of un-sanitized input through to fcgi,
> > so does this matter in the grand scheme of things?
> > But I'd like if server_http_parsehost() enforces syntactically valid
> > hostnames first.
> >
> > There is valid_domain() in usr.bin/ssh/misc.c or res_hnok in
> > resolv.h. Which is probably fine to use since we don't care too much
> > about portable.
>
> right, httpd is happily accepting any nonsense as Host. Here's an
> adaptation of millert' valid_domain() from usr.bin/ssh/misc.c.
> Hope noone is relying on using an empty/malformed Host :>
actually this is wrong. it breaks when an ipv6 address is used as
Host. this seems to be an issue in ssh too, although I haven't
checked if the ssh URI specification inherits the authority from
rfc3986 as-is.
here's a slightly revised version, but just for the sake of the
discussion since its ipv6 parsing is too permissive.
should I make it strictier (not done just because it's a bit long and
definitely not fun to validate ipv6 addresses), or there's something
else in base that I can steal fom? :)
(while here I've tweaked slightly the nonsense in
server_http_parsehost wrt ipv6 addresses and port numbers.)
Thanks!
diff /usr/src
commit - 9832f5035d799ae12e9f848cff5650481b673260
path + /usr/src
blob - 091cc86fec946a4a9d422c04a48f5cbfae015ce0
file + usr.sbin/httpd/server_http.c
--- usr.sbin/httpd/server_http.c
+++ usr.sbin/httpd/server_http.c
@@ -831,6 +831,60 @@ char *
return (buf);
}
+static int
+valid_domain(char *name, const char **errstr)
+{
+ size_t i;
+ unsigned char c, last = '\0';
+
+ *errstr = NULL;
+
+ if (*name == '\0') {
+ *errstr = "empty domain name";
+ return 0;
+ }
+
+ if (name[0] == '[') {
+ for (i = 1; name[i] != '\0'; ++i) {
+ if (name[i] == ']')
+ break;
+ if (!isxdigit((unsigned char)name[i]) &&
+ name[i] != ':') {
+ *errstr = "invalid IPv6 address";
+ return 0;
+ }
+ }
+ if (name[i] != ']' || name[i + 1] != '\0') {
+ *errstr = "invalid IPv6 address";
+ return 0;
+ }
+ return 1;
+ }
+
+ if (!isalpha((unsigned char)name[0]) &&
+ !isdigit((unsigned char)name[0])) {
+ *errstr = "domain name starts with an invalid character";
+ return 0;
+ }
+ for (i = 0; name[i] != '\0'; ++i) {
+ c = tolower((unsigned char)name[i]);
+ name[i] = c;
+ if (last == '.' && c == '.') {
+ *errstr = "domain name contains consecutive separators";
+ return 0;
+ }
+ if (c != '.' && c != '-' && !isalnum(c) &&
+ c != '_') /* technically invalid, but common */ {
+ *errstr = "domain name contains invalid characters";
+ return 0;
+ }
+ last = c;
+ }
+ if (name[i - 1] == '.')
+ name[i - 1] = '\0';
+ return 1;
+}
+
char *
server_http_parsehost(char *host, char *buf, size_t len, int *portval)
{
@@ -847,13 +901,11 @@ server_http_parsehost(char *host, char *buf, size_t le
if (*start == '[' && (end = strchr(start, ']')) != NULL) {
/* Address enclosed in [] with port, eg. [2001:db8::1]:80 */
- start++;
- *end++ = '\0';
- if ((port = strchr(end, ':')) == NULL || *port == '\0')
- port = NULL;
- else
- port++;
- memmove(buf, start, strlen(start) + 1);
+ end++;
+ if (*end == ':') {
+ *end++ = '\0';
+ port = end;
+ }
} else if ((end = strchr(start, ':')) != NULL) {
/* Name or address with port, eg. www.example.com:80 */
*end++ = '\0';
@@ -863,6 +915,11 @@ server_http_parsehost(char *host, char *buf, size_t le
port = NULL;
}
+ if (!valid_domain(start, )) {
+ log_debug("%s: %s: %s", __func__, errstr, host);
+ return (NULL);
+ }
+
if (port != NULL) {
/* Save the requested port */
*portval = strtonum(port, 0, 0x, );
