Re: slaacd, can we use AF_INET6 to restrict route messages?
On Thu, Jul 05, 2018 at 10:51:24PM +0100, Stuart Henderson wrote:
> I noticed relatively high cpu use from slaacd on a BGP router
> that was undergoing some route churn earlier (no interfaces were
> actually configured to use slaac).
>
> routesock is currently unfiltered so will see a lot of messages,
> frontend_routesock is filtered but will still see all RTM_DELETE
> which will be noticeable during churn.
>
> Seems that we may be able to restrict these to inet6 (kernel comment
> is, "If route socket is bound to an address family only send messages
> that match the address family. Address family agnostic messages are
> always sent.") Does this make sense or am I missing something about
> the messages needed by slaacd?
>
In my opinion this is the right thing to do.
RTM_IFINFO is always sent, RTM_NEWADDR and RTM_DELADDR will be filtered
based on the address family as is RTM_DELETE. Those should be fine.
My question mark is about RTM_PROPOSAL. Does slaacd need to see dhclient
RTM_PROPOSAL messages? Looking at the code it does not seem like that is
the case.
Also it seems routesock is never read so it should be
shutdown(routesock, SHUT_RD);
OK claudio@ (which is not much worth when it comes to AF_INET6 :))
>
> Index: slaacd.c
> ===
> RCS file: /cvs/src/sbin/slaacd/slaacd.c,v
> retrieving revision 1.23
> diff -u -p -u -7 -r1.23 slaacd.c
> --- slaacd.c 18 Jun 2018 16:13:45 - 1.23
> +++ slaacd.c 5 Jul 2018 21:44:38 -
> @@ -240,15 +240,15 @@ main(int argc, char *argv[])
> pipe_main2frontend[1], debug, verbose);
>
> slaacd_process = PROC_MAIN;
>
> log_procinit(log_procnames[slaacd_process]);
>
> if ((routesock = socket(PF_ROUTE, SOCK_RAW | SOCK_CLOEXEC |
> - SOCK_NONBLOCK, 0)) < 0)
> + SOCK_NONBLOCK, AF_INET6)) < 0)
> fatal("route socket");
>
> event_init();
>
> /* Setup signal handler. */
> signal_set(&ev_sigint, SIGINT, main_sig_handler, NULL);
> signal_set(&ev_sigterm, SIGTERM, main_sig_handler, NULL);
> @@ -300,16 +300,16 @@ main(int argc, char *argv[])
> /* only router advertisements */
> ICMP6_FILTER_SETBLOCKALL(&filt);
> ICMP6_FILTER_SETPASS(ND_ROUTER_ADVERT, &filt);
> if (setsockopt(icmp6sock, IPPROTO_ICMPV6, ICMP6_FILTER, &filt,
> sizeof(filt)) == -1)
> fatal("ICMP6_FILTER");
>
> - if ((frontend_routesock = socket(PF_ROUTE, SOCK_RAW | SOCK_CLOEXEC, 0))
> - < 0)
> + if ((frontend_routesock = socket(PF_ROUTE, SOCK_RAW | SOCK_CLOEXEC,
> + AF_INET6)) < 0)
> fatal("route socket");
>
> rtfilter = ROUTE_FILTER(RTM_IFINFO) | ROUTE_FILTER(RTM_NEWADDR) |
> ROUTE_FILTER(RTM_DELADDR) | ROUTE_FILTER(RTM_PROPOSAL) |
> ROUTE_FILTER(RTM_DELETE);
> if (setsockopt(frontend_routesock, PF_ROUTE, ROUTE_MSGFILTER,
> &rtfilter, sizeof(rtfilter)) < 0)
>
--
:wq Claudio
Re: pthread_barrier_init: check count argument
On Thu, Jul 5, 2018 at 1:20 AM Paul Irofti wrote: > POSIX mandates that we return EINVAL if count equals zero on barrier > initialization. Which makes sense to me. > > This also fixes posixtestsuite conformance 3-1 test. > > OK? > ok guenther@
slaacd, can we use AF_INET6 to restrict route messages?
I noticed relatively high cpu use from slaacd on a BGP router
that was undergoing some route churn earlier (no interfaces were
actually configured to use slaac).
routesock is currently unfiltered so will see a lot of messages,
frontend_routesock is filtered but will still see all RTM_DELETE
which will be noticeable during churn.
Seems that we may be able to restrict these to inet6 (kernel comment
is, "If route socket is bound to an address family only send messages
that match the address family. Address family agnostic messages are
always sent.") Does this make sense or am I missing something about
the messages needed by slaacd?
Index: slaacd.c
===
RCS file: /cvs/src/sbin/slaacd/slaacd.c,v
retrieving revision 1.23
diff -u -p -u -7 -r1.23 slaacd.c
--- slaacd.c18 Jun 2018 16:13:45 - 1.23
+++ slaacd.c5 Jul 2018 21:44:38 -
@@ -240,15 +240,15 @@ main(int argc, char *argv[])
pipe_main2frontend[1], debug, verbose);
slaacd_process = PROC_MAIN;
log_procinit(log_procnames[slaacd_process]);
if ((routesock = socket(PF_ROUTE, SOCK_RAW | SOCK_CLOEXEC |
- SOCK_NONBLOCK, 0)) < 0)
+ SOCK_NONBLOCK, AF_INET6)) < 0)
fatal("route socket");
event_init();
/* Setup signal handler. */
signal_set(&ev_sigint, SIGINT, main_sig_handler, NULL);
signal_set(&ev_sigterm, SIGTERM, main_sig_handler, NULL);
@@ -300,16 +300,16 @@ main(int argc, char *argv[])
/* only router advertisements */
ICMP6_FILTER_SETBLOCKALL(&filt);
ICMP6_FILTER_SETPASS(ND_ROUTER_ADVERT, &filt);
if (setsockopt(icmp6sock, IPPROTO_ICMPV6, ICMP6_FILTER, &filt,
sizeof(filt)) == -1)
fatal("ICMP6_FILTER");
- if ((frontend_routesock = socket(PF_ROUTE, SOCK_RAW | SOCK_CLOEXEC, 0))
- < 0)
+ if ((frontend_routesock = socket(PF_ROUTE, SOCK_RAW | SOCK_CLOEXEC,
+ AF_INET6)) < 0)
fatal("route socket");
rtfilter = ROUTE_FILTER(RTM_IFINFO) | ROUTE_FILTER(RTM_NEWADDR) |
ROUTE_FILTER(RTM_DELADDR) | ROUTE_FILTER(RTM_PROPOSAL) |
ROUTE_FILTER(RTM_DELETE);
if (setsockopt(frontend_routesock, PF_ROUTE, ROUTE_MSGFILTER,
&rtfilter, sizeof(rtfilter)) < 0)
Re: clock(): why is CLOCKS_PER_SEC 100 and not 1,000,000?
On Thu, Jul 05, 2018 at 09:28:34AM -0400, Scott Cheloha wrote: > > On Jul 5, 2018, at 6:51 AM, Paul Irofti wrote: > > > > [...] > > From [1]: > > > The value of CLOCKS_PER_SEC shall be 1 million on XSI-conformant systems. > > However, it may be variable on other systems, and it should not be assumed > > that CLOCKS_PER_SEC is a compile-time constant. > > [...] > > Having it set to a million also introduces issues with overflow for interfaces > like clock(3), although even POSIX now acknowledges this issue and recommends > other interfaces. ... issues with overflow if clock_t is 32-bit. clock_t has been 64-bit since 2013, but CLOCKS_PER_SEC was 100 before then.
Re: clock(): why is CLOCKS_PER_SEC 100 and not 1,000,000?
> On Jul 5, 2018, at 6:51 AM, Paul Irofti wrote: > > Hi, > > POSIX requires that CLOCKS_PER_SEC be 1,000,000[0,1]. > > Any reason we have it set to 100? > > Paul > > [0] -- http://pubs.opengroup.org/onlinepubs/009695399/functions/clock.html > [1] -- http://pubs.opengroup.org/onlinepubs/009695399/basedefs/time.h.html >From [1]: > The value of CLOCKS_PER_SEC shall be 1 million on XSI-conformant systems. > However, it may be variable on other systems, and it should not be assumed > that CLOCKS_PER_SEC is a compile-time constant. We don't claim to be strictly XSI-conforming. I mean, we have XSI options, but we're not strictly conforming. Having it set to a million also introduces issues with overflow for interfaces like clock(3), although even POSIX now acknowledges this issue and recommends other interfaces. It's 100 because hz is 100 on every platform.
Re: pthread_barrier_init: check count argument
> Date: Thu, 5 Jul 2018 13:19:20 +0300
> From: Paul Irofti
>
> Hi,
>
> POSIX mandates that we return EINVAL if count equals zero on barrier
> initialization. Which makes sense to me.
>
> This also fixes posixtestsuite conformance 3-1 test.
>
> OK?
>
>
> Index: rthread_barrier.c
> ===
> RCS file: /cvs/src/lib/librthread/rthread_barrier.c,v
> retrieving revision 1.3
> diff -u -p -u -p -r1.3 rthread_barrier.c
> --- rthread_barrier.c 15 Apr 2016 17:54:17 - 1.3
> +++ rthread_barrier.c 5 Jul 2018 10:16:44 -
> @@ -31,6 +31,9 @@ pthread_barrier_init(pthread_barrier_t *
> if (barrier == NULL)
> return (EINVAL);
>
> + if (count == 0)
> + return(EINVAL);
You forgot a space here. Otherwise ok kettenis@
> +
> if (attr != NULL) {
> if (*attr == NULL)
> return (EINVAL);
>
>
bgpd introduce filterstate
Next step on the bigger RIB refactor. Introduce a filterstate instead of
passing multiple things around. As a benefit this reduces the amount of
malloc() / free() calls because the rde_aspath is now on the stack.
After that more changes will become possible like moving nexthop into the
state, etc.
--
:wq Claudio
Index: rde.c
===
RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
retrieving revision 1.384
diff -u -p -r1.384 rde.c
--- rde.c 5 Jul 2018 10:25:26 - 1.384
+++ rde.c 5 Jul 2018 11:34:44 -
@@ -1363,7 +1363,7 @@ void
rde_update_update(struct rde_peer *peer, struct rde_aspath *asp,
struct bgpd_addr *prefix, u_int8_t prefixlen)
{
- struct rde_aspath *fasp;
+ struct filterstate state;
struct prefix *p;
enum filter_actions action;
u_int16_ti;
@@ -1380,16 +1380,15 @@ rde_update_update(struct rde_peer *peer,
for (i = RIB_LOC_START; i < rib_size; i++) {
if (*ribs[i].name == '\0')
break;
+ rde_filterstate_prep(&state, asp);
/* input filter */
- action = rde_filter(ribs[i].in_rules, peer, &fasp, p);
-
- if (fasp == NULL)
- fasp = asp;
+ action = rde_filter(ribs[i].in_rules, peer, p, &state);
if (action == ACTION_ALLOW) {
rde_update_log("update", i, peer,
- &fasp->nexthop->exit_nexthop, prefix, prefixlen);
- path_update(&ribs[i].rib, peer, fasp, prefix,
+ &state.aspath.nexthop->exit_nexthop, prefix,
+ prefixlen);
+ path_update(&ribs[i].rib, peer, &state.aspath, prefix,
prefixlen, 0);
} else if (prefix_remove(&ribs[i].rib, peer, prefix, prefixlen,
0)) {
@@ -1397,9 +1396,8 @@ rde_update_update(struct rde_peer *peer,
NULL, prefix, prefixlen);
}
- /* free modified aspath */
- if (fasp != asp)
- path_put(fasp);
+ /* clear state */
+ rde_filterstate_clean(&state);
}
}
@@ -2298,23 +2296,19 @@ void
rde_dump_filterout(struct rde_peer *peer, struct prefix *p,
struct ctl_show_rib_request *req)
{
- struct bgpd_addr addr;
- struct rde_aspath *fasp;
+ struct filterstate state;
enum filter_actions a;
if (up_test_update(peer, p) != 1)
return;
- pt_getaddr(p->re->prefix, &addr);
- a = rde_filter(out_rules, peer, &fasp, p);
- if (fasp == NULL)
- fasp = prefix_aspath(p);
+ rde_filterstate_prep(&state, prefix_aspath(p));
+ a = rde_filter(out_rules, peer, p, &state);
if (a == ACTION_ALLOW)
- rde_dump_rib_as(p, fasp, req->pid, req->flags);
+ rde_dump_rib_as(p, &state.aspath, req->pid, req->flags);
- if (fasp != prefix_aspath(p))
- path_put(fasp);
+ rde_filterstate_clean(&state);
}
void
@@ -3043,11 +3037,12 @@ rde_reload_done(void)
void
rde_softreconfig_in(struct rib_entry *re, void *ptr)
{
+ struct filterstate state;
struct rib_desc *rib = ptr;
struct prefix *p, *np;
struct pt_entry *pt;
struct rde_peer *peer;
- struct rde_aspath *asp, *fasp;
+ struct rde_aspath *asp;
enum filter_actions action;
struct bgpd_addr addr;
@@ -3062,30 +3057,29 @@ rde_softreconfig_in(struct rib_entry *re
asp = prefix_aspath(p);
peer = asp->peer;
- action = rde_filter(rib->in_rules, peer, &fasp, p);
- fasp = fasp != NULL ? fasp : asp;
+ rde_filterstate_prep(&state, asp);
+ action = rde_filter(rib->in_rules, peer, p, &state);
if (action == ACTION_ALLOW) {
/* update Local-RIB */
- path_update(&rib->rib, peer, fasp, &addr,
+ path_update(&rib->rib, peer, &state.aspath, &addr,
pt->prefixlen, 0);
} else if (action == ACTION_DENY) {
/* remove from Local-RIB */
prefix_remove(&rib->rib, peer, &addr, pt->prefixlen, 0);
}
- if (fasp != asp)
- path_put(fasp);
+ rde_filterstate_clean(&state);
}
}
void
rde_softreconfig_out(struct rib_entry *re, void *ptr)
{
+ struct filterstate ostate, nstate;
struct prefix *p = re->active;
struct pt_entry *pt;
struct rde_peer *peer = ptr;
Re: [PATCH] fakertc: A Real time clock simulator
> From: Aaron Lancaster > Date: Wed, 4 Jul 2018 11:40:38 -0600 > > I wrote this patch/script to deal with a very irritating case of > circular dependency failure I recently encountered on an APU2 system. > > On systems with non-existent or dead battery real time clocks, with DNS > over TLS being used, if your clock gets reset to a much earlier date > due to power off, DNS over TLS will cease to function due to certs not > being valid yet, thus DNS will fail entirely, meaning that pool.ntp.org > will fail to be looked up, and ntpd(8) can not fix the date. Thus > you arrive at a situation where DNS is broken because time is broken > because DNS is broken that is not fixable without manual intervention. > > Similar situations can arise with DNSSEC, ntpd.conf(4) constraint > settings, as well as stuff like 802.1X authentication (although I have not > run into that specific case). > > My solution is to sync the current system time to a file every 30 > minutes, as well as on shutdown, and restore from it on boot if the > boot system time fails with some basic sanity checks. It is not a > perfect solution but there can't be a perfect solution as far as I > can see for this specific class of brokenness, and demanding users > bolt on an RTC to an embedded system so networking isn't fundamentally > broken seems like pretty extreme overkill. In any case this solution > is better than taking the system back to January 1st 1970. > > It seems pretty universally useful, as RTC batteries die all the time, > and it rendering your network connectivity totally broken except for > manual intervention seems pretty awful for something like a remote > system, so I have not provided any knobs for turning it on and off. > > It would make sense to emit the current date into the > /var/db/fakertc.time file from the installer, as well as do a clock > sanity check from /etc/daily but I wasn't certain how to modify these > files in the correct manner so I left them untouched. > > Looking for feedback, thanks for your time The solution you've implemented already exists. If the RTC doesn't report a valid time, OpenBSD will take the time from the filesystem. The issue here is that some of our RTC drivers don't properly indicate that the time they report isn't valid. Some of our ports check the time and decide that any time before is invalid. And some hardware can detect a power-loss and drivers can detect this. It is all a bit inconsistent though. But fixing the cmos RTC code and/or teaching amd64/i386 that times before a certain cutoff time are invalid is a better solution than adding this fake driver. Cheers, Mark
Re: priofilter and rtm message types
On Wed, Jul 04, 2018 at 11:51:46PM +0200, Sebastian Benoit wrote:
> Remi noticed that ospfd does no longer see new interfaces.
>
> The priority filter should only work on routing messages that
> have a rtm_priority.
>
> So these are out:
>
> RTM_DELADDR
> RTM_NEWADDR (struct ifa_msghdr)
> RTM_IFINFO (struct if_msghdr)
> RTM_IFANNOUNCE (struct if_announcemsghdr)
> RTM_BFD (struct bfd_msghdr)
>
> and all others (struct rt_msghdr) are ok and handled in the default case
> further down.
>
> Move the rop_priority check there.
>
> ok?
RTM_BFD is not handled by the switch() and neither is RTM_INVALIDATE.
Now RTM_INVALIDATE is only internally used but RTM_BFD should be added
to the RTM_RESOLVE case.
With that OK claudio@
> diff --git sys/net/rtsock.c sys/net/rtsock.c
> index afab5d72505..4d3442d0924 100644
> --- sys/net/rtsock.c
> +++ sys/net/rtsock.c
> @@ -465,9 +465,6 @@ next:
> if (rtm->rtm_type != RTM_DESYNC && rop->rop_msgfilter != 0 &&
> !(rop->rop_msgfilter & (1 << rtm->rtm_type)))
> goto next;
> - if (rop->rop_priority != 0 &&
> - rop->rop_priority < rtm->rtm_priority)
> - goto next;
> switch (rtm->rtm_type) {
> case RTM_IFANNOUNCE:
> case RTM_DESYNC:
> @@ -483,6 +480,9 @@ next:
> goto next;
> break;
> default:
> + if (rop->rop_priority != 0 &&
> + rop->rop_priority < rtm->rtm_priority)
> + goto next;
> /* check against rtable id */
> if (rop->rop_rtableid != RTABLE_ANY &&
> rop->rop_rtableid != rtm->rtm_tableid)
>
--
:wq Claudio
clock(): why is CLOCKS_PER_SEC 100 and not 1,000,000?
Hi, POSIX requires that CLOCKS_PER_SEC be 1,000,000[0,1]. Any reason we have it set to 100? Paul [0] -- http://pubs.opengroup.org/onlinepubs/009695399/functions/clock.html [1] -- http://pubs.opengroup.org/onlinepubs/009695399/basedefs/time.h.html
pthread_barrier_init: check count argument
Hi,
POSIX mandates that we return EINVAL if count equals zero on barrier
initialization. Which makes sense to me.
This also fixes posixtestsuite conformance 3-1 test.
OK?
Index: rthread_barrier.c
===
RCS file: /cvs/src/lib/librthread/rthread_barrier.c,v
retrieving revision 1.3
diff -u -p -u -p -r1.3 rthread_barrier.c
--- rthread_barrier.c 15 Apr 2016 17:54:17 - 1.3
+++ rthread_barrier.c 5 Jul 2018 10:16:44 -
@@ -31,6 +31,9 @@ pthread_barrier_init(pthread_barrier_t *
if (barrier == NULL)
return (EINVAL);
+ if (count == 0)
+ return(EINVAL);
+
if (attr != NULL) {
if (*attr == NULL)
return (EINVAL);
Network syscalls description: why we can unlock them
Diff below unlocks 12 network syscalls. It relies on the recent work
that has been to make 'struct file' refcounting mpsafe and on the
corresponding socket lock before messing with the content of any
socket.
Why can we unlock the following syscall? Here's an explanation.
- sys_recvmsg() and sys_recvfrom() are wrappers around recvit(). They
hold a socket reference then call soreceive() which will gran the
corresponding socket lock. For STREAM & DGRAM sockets this is the
NET_LOCK() for the rest it is the KERNEL_LOCK().
- sys_accept() and sys_accept4() are wrappers around doaccept(). They
hold a socket reference, allocate a 'struct file' then block for a
corresponding connect(2). Once awaken the fully populate `fp' and
place it in the global data structure. The race with dup2(2), while
sleeping, has been previously fixed:
https://marc.info/?l=openbsd-cvs&m=152749772009924&w=2
https://marc.info/?l=openbsd-cvs&m=152996258723362&w=2
- sys_getpeername() and sys_getsockname() hold a socket reference, grab
the socket lock then go down into PRU_PEERADDR and PRU_SOCKADDR res-
pectively. Reading PCB fields are serialized by the corresponding
lock.
- sys_connect() holds a socket reference, grab the socket lock and
establish the connection under this lock.
- sys_bind(), sys_listen() and sys_shutdown() do the same but respect-
ively call sobind(), solisten() and soshutdown() on the socket.
solisten() reads global int-size variables that are only modified
with the KERNEL_LOCK(). So I'm also grabbing it here to indicate
that his needs more love.
- sys_setsockopt() and sys_getsockopt() hold a socket reference, grab
the socket lock then call sosetopt() and sogetopt() respectively.
Reading/writing PCB fields is fine with the corresponding lock.
There's however one global which is created in sosplice(), even if
the NET_LOCK() is enough to prevent races, creating a thread still
require the KERNEL_LOCK(). So I'm grabbing it here.
Please do not forget to do "cd /sys/kern && make syscalls" after
applying this diff.
Index: kern/syscalls.master
===
RCS file: /cvs/src/sys/kern/syscalls.master,v
retrieving revision 1.184
diff -u -p -r1.184 syscalls.master
--- kern/syscalls.master27 Jun 2018 16:38:23 - 1.184
+++ kern/syscalls.master5 Jul 2018 09:35:15 -
@@ -88,18 +88,18 @@
#else
26 UNIMPL ptrace
#endif
-27 STD { ssize_t sys_recvmsg(int s, struct msghdr *msg, \
+27 STD NOLOCK { ssize_t sys_recvmsg(int s, struct msghdr *msg, \
int flags); }
28 STD NOLOCK { ssize_t sys_sendmsg(int s, \
const struct msghdr *msg, int flags); }
-29 STD { ssize_t sys_recvfrom(int s, void *buf, size_t len, \
+29 STD NOLOCK { ssize_t sys_recvfrom(int s, void *buf, size_t len, \
int flags, struct sockaddr *from, \
socklen_t *fromlenaddr); }
-30 STD { int sys_accept(int s, struct sockaddr *name, \
+30 STD NOLOCK { int sys_accept(int s, struct sockaddr *name, \
socklen_t *anamelen); }
-31 STD { int sys_getpeername(int fdes, struct sockaddr *asa, \
+31 STD NOLOCK { int sys_getpeername(int fdes, struct sockaddr *asa, \
socklen_t *alen); }
-32 STD { int sys_getsockname(int fdes, struct sockaddr *asa, \
+32 STD NOLOCK { int sys_getsockname(int fdes, struct sockaddr *asa, \
socklen_t *alen); }
33 STD { int sys_access(const char *path, int amode); }
34 STD { int sys_chflags(const char *path, u_int flags); }
@@ -205,7 +205,7 @@
91 STD { int sys_nanosleep(const struct timespec *rqtp, \
struct timespec *rmtp); }
92 STD { int sys_fcntl(int fd, int cmd, ... void *arg); }
-93 STD { int sys_accept4(int s, struct sockaddr *name, \
+93 STD NOLOCK { int sys_accept4(int s, struct sockaddr *name, \
socklen_t *anamelen, int flags); }
94 STD { int sys___thrsleep(const volatile void *ident, \
clockid_t clock_id, const struct timespec *tp, \
@@ -213,18 +213,18 @@
95 STD { int sys_fsync(int fd); }
96 STD { int sys_setpriority(int which, id_t who, int prio); }
97 STD NOLOCK { int sys_socket(int domain, int type, int protocol); }
-98 STD { int sys_connect(int s, const struct sockaddr *name, \
+98 STD NOLOCK { int sys_connect(int s, const struct sockaddr *name, \
socklen_t namelen); }
99 STD { int sys_getdents(int fd, void *buf, size_t buflen); }
100STD { int sys_getprior
Re: uaudio: fix detection of a bunch of logitech webcams
On Thu, Jul 05, 2018 at 11:04:34AM +0200, Martin Pieuchot wrote:
> On 05/07/18(Thu) 09:28, Landry Breuil wrote:
> > On Fri, Jun 29, 2018 at 01:50:24PM +0200, Landry Breuil wrote:
> > > On Fri, Jun 29, 2018 at 09:05:56AM +0200, Remco wrote:
> > > > > > If you don't find any better solution, I'd suggest using a device ID
> > > > > > check rather than adding a quirk. Because such quirk cannot be
> > > > > > generic.
> > > > > > In that case you have an off-by-one, but another device might have a
> > > > > > different problem.
> > > > >
> > > > > That was in the case we'd encounter other devices where the descriptor
> > > > > is bogus and has the same issue, but i agree this is unlikely.
> > > > >
> > > >
> > > > More likely for Logitech USB webcams I'm afraid.
> > > > My dad's Logitech C500 showed the same issue several years back.
> > > > (I'll try to find some time to borrow the device and test your patch)
> > > > There's also a report that suggests a C200 may be affected as well.
> > > > (https://marc.info/?l=openbsd-misc&m=127952275021290&w=2)
> > >
> > > Aaah, very interesting data point. Thanks for the pointer.. maybe a
> > > 'generic fix' would be to ignore size checks for all logitech webcams,
> > > or to print a warning but not error out in this case. Is there some kind
> > > of 'usbdevs -v/lsusb -v' database somewhere ?
> >
> > So after feedback from lots of logitech webcam users (thanks!), i got a list
> > for working vs non-working devices, and i think a new quirk would do as they
> > all expose the same issue (bLength for FEATURE_UNIT is 9 while it is 8 on
> > working devices) - and they would all be 'fixed' by the attached diff.
>
> If you know that some descriptors have an off-by-one, I'd suggest you add
> 1 to the corresponding buffer length instead of completely removing the
> check. Removing it completely now open the door to other overflows and
> adding 1 auto-document the hardware bug ;)
Well that was the intent of the original diff. New version...
Index: uaudio.c
===
RCS file: /cvs/src/sys/dev/usb/uaudio.c,v
retrieving revision 1.128
diff -u -r1.128 uaudio.c
--- uaudio.c30 Dec 2017 23:08:29 - 1.128
+++ uaudio.c5 Jul 2018 09:12:42 -
@@ -172,6 +172,7 @@
#define UAUDIO_FLAG_VENDOR_CLASS 0x0010/* claims vendor class but
works */
#define UAUDIO_FLAG_DEPENDENT 0x0020 /* play and record params must equal */
#define UAUDIO_FLAG_EMU0202 0x0040
+#define UAUDIO_FLAG_BAD_ADC_LEN 0x0080 /* bad audio control descriptor
size */
struct uaudio_devs {
struct usb_devno uv_dev;
@@ -222,6 +223,16 @@
UAUDIO_FLAG_BAD_AUDIO },
{ { USB_VENDOR_LOGITECH, USB_PRODUCT_LOGITECH_QUICKCAMZOOM },
UAUDIO_FLAG_BAD_AUDIO },
+ { { USB_VENDOR_LOGITECH, USB_PRODUCT_LOGITECH_WEBCAMC200 },
+ UAUDIO_FLAG_BAD_ADC_LEN },
+ { { USB_VENDOR_LOGITECH, USB_PRODUCT_LOGITECH_WEBCAMC210 },
+ UAUDIO_FLAG_BAD_ADC_LEN },
+ { { USB_VENDOR_LOGITECH, USB_PRODUCT_LOGITECH_WEBCAMC270 },
+ UAUDIO_FLAG_BAD_ADC_LEN },
+ { { USB_VENDOR_LOGITECH, USB_PRODUCT_LOGITECH_WEBCAMC310 },
+ UAUDIO_FLAG_BAD_ADC_LEN },
+ { { USB_VENDOR_LOGITECH, USB_PRODUCT_LOGITECH_WEBCAMC500 },
+ UAUDIO_FLAG_BAD_ADC_LEN },
{ { USB_VENDOR_TELEX, USB_PRODUCT_TELEX_MIC1 },
UAUDIO_FLAG_NO_FRAC }
};
@@ -1814,6 +1825,10 @@
sc->sc_audio_rev = UGETW(acdp->bcdADC);
DPRINTFN(2,("%s: found AC header, vers=%03x, len=%d\n",
__func__, sc->sc_audio_rev, aclen));
+
+ /* Some webcams descriptors advertise an off-by-one wTotalLength */
+ if (sc->sc_quirks & UAUDIO_FLAG_BAD_ADC_LEN)
+ aclen++;
sc->sc_nullalt = -1;
Re: uaudio: fix detection of a bunch of logitech webcams
On 05/07/18(Thu) 09:28, Landry Breuil wrote:
> On Fri, Jun 29, 2018 at 01:50:24PM +0200, Landry Breuil wrote:
> > On Fri, Jun 29, 2018 at 09:05:56AM +0200, Remco wrote:
> > > > > If you don't find any better solution, I'd suggest using a device ID
> > > > > check rather than adding a quirk. Because such quirk cannot be
> > > > > generic.
> > > > > In that case you have an off-by-one, but another device might have a
> > > > > different problem.
> > > >
> > > > That was in the case we'd encounter other devices where the descriptor
> > > > is bogus and has the same issue, but i agree this is unlikely.
> > > >
> > >
> > > More likely for Logitech USB webcams I'm afraid.
> > > My dad's Logitech C500 showed the same issue several years back.
> > > (I'll try to find some time to borrow the device and test your patch)
> > > There's also a report that suggests a C200 may be affected as well.
> > > (https://marc.info/?l=openbsd-misc&m=127952275021290&w=2)
> >
> > Aaah, very interesting data point. Thanks for the pointer.. maybe a
> > 'generic fix' would be to ignore size checks for all logitech webcams,
> > or to print a warning but not error out in this case. Is there some kind
> > of 'usbdevs -v/lsusb -v' database somewhere ?
>
> So after feedback from lots of logitech webcam users (thanks!), i got a list
> for working vs non-working devices, and i think a new quirk would do as they
> all expose the same issue (bLength for FEATURE_UNIT is 9 while it is 8 on
> working devices) - and they would all be 'fixed' by the attached diff.
If you know that some descriptors have an off-by-one, I'd suggest you add
1 to the corresponding buffer length instead of completely removing the
check. Removing it completely now open the door to other overflows and
adding 1 auto-document the hardware bug ;)
> If an overflow is detected on the descriptor size (ie the (ibuf +
> dp->bLength > ibufend) check but we know we're on a broken device (with
> UAUDIO_FLAG_BAD_ADC_LEN quirk) we silently avoid returning USBD_INVAL,
> and that allows uaudio(4) detection to succeed. That's a different take than
> adding 1 to aclen in the first version of the diff, i dunno which is better.
>
> Now, i'm pretty sure i managed to properly test the device some days ago
> and record sound with it, but now i only manage to produce 68-bytes
> empty wav files, using 'aucat -f rsnd/1 -o /tmp/test.wav' after having
> ensured kern.audio.record was 1. Something is fishy
>
> Oks, testing & feedback welcome, so that i can move forward and get more
> testing/debugging done.
>
> Landry
>
> Index: uaudio.c
> ===
> RCS file: /cvs/src/sys/dev/usb/uaudio.c,v
> retrieving revision 1.128
> diff -u -r1.128 uaudio.c
> --- uaudio.c 30 Dec 2017 23:08:29 - 1.128
> +++ uaudio.c 3 Jul 2018 06:44:28 -
> @@ -172,6 +172,7 @@
> #define UAUDIO_FLAG_VENDOR_CLASS 0x0010 /* claims vendor class but
> works */
> #define UAUDIO_FLAG_DEPENDENT 0x0020 /* play and record params must
> equal */
> #define UAUDIO_FLAG_EMU0202 0x0040
> +#define UAUDIO_FLAG_BAD_ADC_LEN 0x0080 /* bad audio control descriptor
> size */
>
> struct uaudio_devs {
> struct usb_devno uv_dev;
> @@ -222,6 +223,16 @@
> UAUDIO_FLAG_BAD_AUDIO },
> { { USB_VENDOR_LOGITECH, USB_PRODUCT_LOGITECH_QUICKCAMZOOM },
> UAUDIO_FLAG_BAD_AUDIO },
> + { { USB_VENDOR_LOGITECH, USB_PRODUCT_LOGITECH_WEBCAMC200 },
> + UAUDIO_FLAG_BAD_ADC_LEN },
> + { { USB_VENDOR_LOGITECH, USB_PRODUCT_LOGITECH_WEBCAMC210 },
> + UAUDIO_FLAG_BAD_ADC_LEN },
> + { { USB_VENDOR_LOGITECH, USB_PRODUCT_LOGITECH_WEBCAMC270 },
> + UAUDIO_FLAG_BAD_ADC_LEN },
> + { { USB_VENDOR_LOGITECH, USB_PRODUCT_LOGITECH_WEBCAMC310 },
> + UAUDIO_FLAG_BAD_ADC_LEN },
> + { { USB_VENDOR_LOGITECH, USB_PRODUCT_LOGITECH_WEBCAMC500 },
> + UAUDIO_FLAG_BAD_ADC_LEN },
> { { USB_VENDOR_TELEX, USB_PRODUCT_TELEX_MIC1 },
> UAUDIO_FLAG_NO_FRAC }
> };
> @@ -1832,7 +1843,8 @@
> if (ibuf >= ibufend)
> break;
> dp = (const usb_descriptor_t *)ibuf;
> - if (ibuf + dp->bLength > ibufend) {
> + if (ibuf + dp->bLength > ibufend &&
> + !(sc->sc_quirks & UAUDIO_FLAG_BAD_ADC_LEN)) {
> free(iot, M_TEMP, 0);
> return (USBD_INVAL);
> }
>
uaudio: fix detection of a bunch of logitech webcams
On Fri, Jun 29, 2018 at 01:50:24PM +0200, Landry Breuil wrote:
> On Fri, Jun 29, 2018 at 09:05:56AM +0200, Remco wrote:
> > > > If you don't find any better solution, I'd suggest using a device ID
> > > > check rather than adding a quirk. Because such quirk cannot be generic.
> > > > In that case you have an off-by-one, but another device might have a
> > > > different problem.
> > >
> > > That was in the case we'd encounter other devices where the descriptor
> > > is bogus and has the same issue, but i agree this is unlikely.
> > >
> >
> > More likely for Logitech USB webcams I'm afraid.
> > My dad's Logitech C500 showed the same issue several years back.
> > (I'll try to find some time to borrow the device and test your patch)
> > There's also a report that suggests a C200 may be affected as well.
> > (https://marc.info/?l=openbsd-misc&m=127952275021290&w=2)
>
> Aaah, very interesting data point. Thanks for the pointer.. maybe a
> 'generic fix' would be to ignore size checks for all logitech webcams,
> or to print a warning but not error out in this case. Is there some kind
> of 'usbdevs -v/lsusb -v' database somewhere ?
So after feedback from lots of logitech webcam users (thanks!), i got a list
for working vs non-working devices, and i think a new quirk would do as they
all expose the same issue (bLength for FEATURE_UNIT is 9 while it is 8 on
working devices) - and they would all be 'fixed' by the attached diff.
If an overflow is detected on the descriptor size (ie the (ibuf +
dp->bLength > ibufend) check but we know we're on a broken device (with
UAUDIO_FLAG_BAD_ADC_LEN quirk) we silently avoid returning USBD_INVAL,
and that allows uaudio(4) detection to succeed. That's a different take than
adding 1 to aclen in the first version of the diff, i dunno which is better.
Now, i'm pretty sure i managed to properly test the device some days ago
and record sound with it, but now i only manage to produce 68-bytes
empty wav files, using 'aucat -f rsnd/1 -o /tmp/test.wav' after having
ensured kern.audio.record was 1. Something is fishy
Oks, testing & feedback welcome, so that i can move forward and get more
testing/debugging done.
Landry
Index: uaudio.c
===
RCS file: /cvs/src/sys/dev/usb/uaudio.c,v
retrieving revision 1.128
diff -u -r1.128 uaudio.c
--- uaudio.c30 Dec 2017 23:08:29 - 1.128
+++ uaudio.c3 Jul 2018 06:44:28 -
@@ -172,6 +172,7 @@
#define UAUDIO_FLAG_VENDOR_CLASS 0x0010/* claims vendor class but
works */
#define UAUDIO_FLAG_DEPENDENT 0x0020 /* play and record params must equal */
#define UAUDIO_FLAG_EMU0202 0x0040
+#define UAUDIO_FLAG_BAD_ADC_LEN 0x0080 /* bad audio control descriptor
size */
struct uaudio_devs {
struct usb_devno uv_dev;
@@ -222,6 +223,16 @@
UAUDIO_FLAG_BAD_AUDIO },
{ { USB_VENDOR_LOGITECH, USB_PRODUCT_LOGITECH_QUICKCAMZOOM },
UAUDIO_FLAG_BAD_AUDIO },
+ { { USB_VENDOR_LOGITECH, USB_PRODUCT_LOGITECH_WEBCAMC200 },
+ UAUDIO_FLAG_BAD_ADC_LEN },
+ { { USB_VENDOR_LOGITECH, USB_PRODUCT_LOGITECH_WEBCAMC210 },
+ UAUDIO_FLAG_BAD_ADC_LEN },
+ { { USB_VENDOR_LOGITECH, USB_PRODUCT_LOGITECH_WEBCAMC270 },
+ UAUDIO_FLAG_BAD_ADC_LEN },
+ { { USB_VENDOR_LOGITECH, USB_PRODUCT_LOGITECH_WEBCAMC310 },
+ UAUDIO_FLAG_BAD_ADC_LEN },
+ { { USB_VENDOR_LOGITECH, USB_PRODUCT_LOGITECH_WEBCAMC500 },
+ UAUDIO_FLAG_BAD_ADC_LEN },
{ { USB_VENDOR_TELEX, USB_PRODUCT_TELEX_MIC1 },
UAUDIO_FLAG_NO_FRAC }
};
@@ -1832,7 +1843,8 @@
if (ibuf >= ibufend)
break;
dp = (const usb_descriptor_t *)ibuf;
- if (ibuf + dp->bLength > ibufend) {
+ if (ibuf + dp->bLength > ibufend &&
+ !(sc->sc_quirks & UAUDIO_FLAG_BAD_ADC_LEN)) {
free(iot, M_TEMP, 0);
return (USBD_INVAL);
}
