| Message ID | 20250214122759.2629-2-mike@fireburn.co.uk (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ntsync: Set the permissions to be 0666 | expand |
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?
thanks,
greg k-h
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.
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. --D
On Fri, 14 Feb 2025, 18:45 Darrick J. Wong, <djwong@kernel.org> 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. > > --D > I did create a MR for systemd first https://github.com/systemd/systemd/pull/36384 >
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.
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. > 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. <shrug> Well yes, I could turn it off in my bespoke kernels, but most distributors turn on a lot of modules to minimize friction for users. Chances are that'll be most of them if this enables better Steam gaming. --D
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
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. > > Ok, makes sense, can you give an acked-by or reviewed-by and I'll be glad to queue it up and get it merged. thanks, greg k-h
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@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@codeweavers.com> -- The comment seems rather redundant, but otherwise this is correct and prudent.
On Tue, Feb 18, 2025 at 05:57:31PM -0600, Elizabeth Figura wrote: > 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@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@codeweavers.com> > > -- > > The comment seems rather redundant, but otherwise this is correct and prudent. I agree, I'll drop the comment when I apply it, thanks. greg k-h
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);
This allows ntsync to be usuable by non-root processes out of the box Signed-off-by: Mike Lothian <mike@fireburn.co.uk> --- drivers/misc/ntsync.c | 1 + 1 file changed, 1 insertion(+)