- Wine-devel - list.winehq.org

Re: [PATCH] ntsync: Check wait count based on byte size.
by Elizabeth Figura 20 Feb '25

20 Feb '25

On Wednesday, 19 February 2025 08:25:27 CST Greg Kroah-Hartman wrote: > On Tue, Feb 18, 2025 at 06:19:20PM -0600, Elizabeth Figura wrote: > > GCC versions below 13 incorrectly detect the copy size as being static and too > > small to fit in the "fds" array. Work around this by explicitly calculating the > > size and returning EINVAL based on that, instead of based on the object count. > > > > Reported-by: kernel test robot <lkp(a)intel.com> > > Closes: https://lore.kernel.org/oe-kbuild-all/202502072019.LYoCR9bF-lkp(a)intel.com/ > > Suggested-by: Arnd Bergmann <arnd(a)arndb.de> > > > > -- > > > > Suggested-by as per Arnd's request, but the only thing I changed was preserving > > array_size() [as noted by Geert in the linked thread]. I tested and found no > > regressions. > > You forgot to sign-off on this commit :( > > Can you resend it please with that fixed? Done, thanks. I used to have format.signOff=true but lost that somewhere...

1 0

[PATCH v2] ntsync: Check wait count based on byte size.
by Elizabeth Figura 20 Feb '25

20 Feb '25

GCC versions below 13 incorrectly detect the copy size as being static and too small to fit in the "fds" array. Work around this by explicitly calculating the size and returning EINVAL based on that, instead of based on the object count. Reported-by: kernel test robot <lkp(a)intel.com> Closes: https://lore.kernel.org/oe-kbuild-all/202502072019.LYoCR9bF-lkp(a)intel.com/ Suggested-by: Arnd Bergmann <arnd(a)arndb.de> Signed-off-by: Elizabeth Figura <zfigura(a)codeweavers.com> -- Suggested-by as per Arnd's request, but the only thing I changed was preserving array_size() [as noted by Geert in the linked thread]. I tested and found no regressions. v2: Add missing sign-off --- drivers/misc/ntsync.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/misc/ntsync.c b/drivers/misc/ntsync.c index 586b86243e1d..24cb2f71186a 100644 --- a/drivers/misc/ntsync.c +++ b/drivers/misc/ntsync.c @@ -873,6 +873,7 @@ static int setup_wait(struct ntsync_device *dev, { int fds[NTSYNC_MAX_WAIT_COUNT + 1]; const __u32 count = args->count; + size_t size = array_size(count, sizeof(fds[0])); struct ntsync_q *q; __u32 total_count; __u32 i, j; @@ -880,15 +881,14 @@ static int setup_wait(struct ntsync_device *dev, if (args->pad || (args->flags & ~NTSYNC_WAIT_REALTIME)) return -EINVAL; - if (args->count > NTSYNC_MAX_WAIT_COUNT) + if (size >= sizeof(fds)) return -EINVAL; total_count = count; if (args->alert) total_count++; - if (copy_from_user(fds, u64_to_user_ptr(args->objs), - array_size(count, sizeof(*fds)))) + if (copy_from_user(fds, u64_to_user_ptr(args->objs), size)) return -EFAULT; if (args->alert) fds[count] = args->alert; -- 2.47.2

1 1

[PATCH] ntsync: Check wait count based on byte size.
by Elizabeth Figura 19 Feb '25

19 Feb '25

GCC versions below 13 incorrectly detect the copy size as being static and too small to fit in the "fds" array. Work around this by explicitly calculating the size and returning EINVAL based on that, instead of based on the object count. Reported-by: kernel test robot <lkp(a)intel.com> Closes: https://lore.kernel.org/oe-kbuild-all/202502072019.LYoCR9bF-lkp(a)intel.com/ Suggested-by: Arnd Bergmann <arnd(a)arndb.de> -- Suggested-by as per Arnd's request, but the only thing I changed was preserving array_size() [as noted by Geert in the linked thread]. I tested and found no regressions. --- drivers/misc/ntsync.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/misc/ntsync.c b/drivers/misc/ntsync.c index 586b86243e1d..24cb2f71186a 100644 --- a/drivers/misc/ntsync.c +++ b/drivers/misc/ntsync.c @@ -873,6 +873,7 @@ static int setup_wait(struct ntsync_device *dev, { int fds[NTSYNC_MAX_WAIT_COUNT + 1]; const __u32 count = args->count; + size_t size = array_size(count, sizeof(fds[0])); struct ntsync_q *q; __u32 total_count; __u32 i, j; @@ -880,15 +881,14 @@ static int setup_wait(struct ntsync_device *dev, if (args->pad || (args->flags & ~NTSYNC_WAIT_REALTIME)) return -EINVAL; - if (args->count > NTSYNC_MAX_WAIT_COUNT) + if (size >= sizeof(fds)) return -EINVAL; total_count = count; if (args->alert) total_count++; - if (copy_from_user(fds, u64_to_user_ptr(args->objs), - array_size(count, sizeof(*fds)))) + if (copy_from_user(fds, u64_to_user_ptr(args->objs), size)) return -EFAULT; if (args->alert) fds[count] = args->alert; -- 2.47.2

1 0

Re: [PATCH] ntsync: Set the permissions to be 0666
by Elizabeth Figura 18 Feb '25

18 Feb '25

On Friday, 14 February 2025 06:28:00 CST Mike Lothian wrote: > This allows ntsync to be usuable by non-root processes out of the box > > Signed-off-by: Mike Lothian <mike(a)fireburn.co.uk> > --- > drivers/misc/ntsync.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/misc/ntsync.c b/drivers/misc/ntsync.c > index 055395cde42b..586b86243e1d 100644 > --- a/drivers/misc/ntsync.c > +++ b/drivers/misc/ntsync.c > @@ -1208,6 +1208,7 @@ static struct miscdevice ntsync_misc = { > .minor = MISC_DYNAMIC_MINOR, > .name = NTSYNC_NAME, > .fops = &ntsync_fops, > + .mode = 0666, // Setting file permissions to 0666 > }; > > module_misc_device(ntsync_misc); > Reviewed-by: Elizabeth Figura <zfigura(a)codeweavers.com> -- The comment seems rather redundant, but otherwise this is correct and prudent.

1 0

Re: [PATCH] ntsync: Set the permissions to be 0666
by Elizabeth Figura 15 Feb '25

15 Feb '25

On Friday, 14 February 2025 19:03:33 CST Darrick J. Wong wrote: > On Fri, Feb 14, 2025 at 04:15:25PM -0600, Elizabeth Figura wrote: > > On Friday, 14 February 2025 12:45:39 CST Darrick J. Wong wrote: > > > On Fri, Feb 14, 2025 at 12:13:03PM -0600, Elizabeth Figura wrote: > > > > On Friday, 14 February 2025 07:06:20 CST Greg Kroah-Hartman wrote: > > > > > On Fri, Feb 14, 2025 at 12:28:00PM +0000, Mike Lothian wrote: > > > > > > This allows ntsync to be usuable by non-root processes out of the box > > > > > > > > > > Are you sure you need/want that? If so, why? How did existing testing > > > > > not ever catch this? > > > > > > > > Hi, sorry, this is of course my fault. > > > > > > > > We do need /dev/ntsync to be openable from user space for it to be > > > > useful. I'm not sure what the most "correct" permissions are to have > > > > in this case (when we don't specifically need read or write), but I > > > > don't think I see a reason not to just set to 666 or 444. > > > > > > > > I originally assumed that the right way to do this was not to set the > > > > mode on the kernel file but rather through udev; I believe I was using > > > > the code for /dev/loop-control or /dev/fuse as an example, which both > > > > do that. So I (and others who tested) had just manually set up udev > > > > rules for this, with the eventual intent of adding a default rule to > > > > systemd like the others. I only recently realized that doing something > > > > like this patch is possible and precedented. > > > > > > > > I don't know what the best way to address this is, but this is > > > > certainly the simplest. > > > > > > Paranoid defaults in the kernel, and then a udev rule to relax the mode > > > at runtime. You could also have logind scripts to add add per-user > > > allow acls to the device file at user session set up time... or however > > > it is that /dev/sr0 has me on the allow list. I'm not sure how that > > > happens exactly, but it works smoothly. > > > > > > I get far less complaining about relaxing posture than tightening it > > > (==breaking things) after the fact. > > > > FWIW, it may be worth stressing that this is not a hardware device in > > any sense, it's a software driver that only lives in a char device > > (and dedicated module) for the sake of isolating the code. I can't > > imagine any reason to control access per-user, although my experience > > may not be enough to grant such imagination. > > Oh, I'm aware that ntsync is a driver for a software "device" that > implements various Windows APIs and isn't real hardware. :) > > But, you might want prevent non-root systemd services (e.g. avahi) from > being able to access /dev/ntsync if, say, someone breaches that, while > at the same time allowing access to (say) logged-in users who can run > Wine. I see the idea, though I don't know if it's applicable in this case—the individual ntsync file descriptions are also supposed to be isolated from one another, so even a rogue avahi would still have another barrier towards compromising a Wine process. Of course you may be aware of that and be advocating for the more barriers the better. I can't say what systemd will actually prefer in this case, but they do seem to have requested that the kernel change it... [1] [1] https://github.com/systemd/systemd/pull/36384

1 0

Re: [PATCH] ntsync: Set the permissions to be 0666
by Elizabeth Figura 14 Feb '25

14 Feb '25

On Friday, 14 February 2025 12:45:39 CST Darrick J. Wong wrote: > On Fri, Feb 14, 2025 at 12:13:03PM -0600, Elizabeth Figura wrote: > > On Friday, 14 February 2025 07:06:20 CST Greg Kroah-Hartman wrote: > > > On Fri, Feb 14, 2025 at 12:28:00PM +0000, Mike Lothian wrote: > > > > This allows ntsync to be usuable by non-root processes out of the box > > > > > > Are you sure you need/want that? If so, why? How did existing testing > > > not ever catch this? > > > > Hi, sorry, this is of course my fault. > > > > We do need /dev/ntsync to be openable from user space for it to be > > useful. I'm not sure what the most "correct" permissions are to have > > in this case (when we don't specifically need read or write), but I > > don't think I see a reason not to just set to 666 or 444. > > > > I originally assumed that the right way to do this was not to set the > > mode on the kernel file but rather through udev; I believe I was using > > the code for /dev/loop-control or /dev/fuse as an example, which both > > do that. So I (and others who tested) had just manually set up udev > > rules for this, with the eventual intent of adding a default rule to > > systemd like the others. I only recently realized that doing something > > like this patch is possible and precedented. > > > > I don't know what the best way to address this is, but this is > > certainly the simplest. > > Paranoid defaults in the kernel, and then a udev rule to relax the mode > at runtime. You could also have logind scripts to add add per-user > allow acls to the device file at user session set up time... or however > it is that /dev/sr0 has me on the allow list. I'm not sure how that > happens exactly, but it works smoothly. > > I get far less complaining about relaxing posture than tightening it > (==breaking things) after the fact. FWIW, it may be worth stressing that this is not a hardware device in any sense, it's a software driver that only lives in a char device (and dedicated module) for the sake of isolating the code. I can't imagine any reason to control access per-user, although my experience may not be enough to grant such imagination. The only actual risk is a bug in the code itself—which is always possible—but at that point you'd presumably just want to disable it at build time or something similar.

1 0

Re: [PATCH] ntsync: Set the permissions to be 0666
by Elizabeth Figura 14 Feb '25

14 Feb '25

On Friday, 14 February 2025 07:06:20 CST Greg Kroah-Hartman wrote: > On Fri, Feb 14, 2025 at 12:28:00PM +0000, Mike Lothian wrote: > > This allows ntsync to be usuable by non-root processes out of the box > > Are you sure you need/want that? If so, why? How did existing testing > not ever catch this? Hi, sorry, this is of course my fault. We do need /dev/ntsync to be openable from user space for it to be useful. I'm not sure what the most "correct" permissions are to have in this case (when we don't specifically need read or write), but I don't think I see a reason not to just set to 666 or 444. I originally assumed that the right way to do this was not to set the mode on the kernel file but rather through udev; I believe I was using the code for /dev/loop-control or /dev/fuse as an example, which both do that. So I (and others who tested) had just manually set up udev rules for this, with the eventual intent of adding a default rule to systemd like the others. I only recently realized that doing something like this patch is possible and precedented. I don't know what the best way to address this is, but this is certainly the simplest.

1 0

Wine staging 10.1 release
by Alistair Leslie-Hughes 09 Feb '25

09 Feb '25

Binary packages for various distributions will be available from: https://www.winehq.org/download Summary since last release * Rebased to current wine 10.1 (361 patches are applied to wine vanilla) Upstreamed (Either directly from staging or fixed with a similar patch). * None Removed (No longer required). * None Added: * [53644] vbscript: class single line multivar. * [56464] vbscript: Implement Join * [52769] mf: Add HTTP/HTTPS IMFByteStream implementation. * [26302] richedit20: Implement IRichEditOle::ImportDataObject * [53620] err:ole:com_get_class_object class IEnumObjects not registered. * [56891] Visual novel Mebae (Tanuki Soft) crashes on startup * [51139] d2d1: Implement drawing of ID2D1GeometryGroup * [56280] oleaut32: VarAnd use VT_I4 when converting VT_BSTR Updated: * vkd3d-latest Where can you help * Run Steam/Battle.net/GOG/UPlay/Epic * Test your favorite game. * Test your favorite applications. * Improve staging patches and get them accepted upstream. * Suggest patches to be included in staging. As always, if you find a bug, please report it via https://bugs.winehq.org Best Regards Alistair.

1 0

FOSDEM
by Huw Davies 30 Jan '25

30 Jan '25

Hi folks, Many CodeWeavers folks will be in Brussels this weekend attending FOSDEM. If anybody else is around, or near Brussels, please feel free to either drop me an email or simply turn up to the BrewDog on Friday or Saturday evenings from 17:00. Many of us will be in Brussels Thursday and Sunday evenings too, but our exact location is less certain; if you let me know you’re interested, I can keep you in the loop. Sorry for the late notice! Huw

1 0

Wine staging 10.0 release
by Alistair Leslie-Hughes 21 Jan '25

21 Jan '25

Binary packages for various distributions will be available from: https://www.winehq.org/download Summary since last release * Rebased to current wine 10.0 (346 patches are applied to wine vanilla) Upstreamed (Either directly from staging or fixed with a similar patch). * None Removed (No longer required). * None Added: * None Updated: * vkd3d-latest Where can you help * Run Steam/Battle.net/GOG/UPlay/Epic * Test your favorite game. * Test your favorite applications. * Improve staging patches and get them accepted upstream. * Suggest patches to be included in staging. As always, if you find a bug, please report it via https://bugs.winehq.org Best Regards Alistair.

1 0