fs/ceph/mds_client: give up on paths longer than PATH_MAX
Subsystems (2)
PATCH 1/1 fs/ceph/mds_client: give up on paths longer than PATH_MAX
If the full path to be built by ceph_mdsc_build_path() happens to be longer than PATH_MAX, then this function will enter an endless (retry) loop, effectively blocking the whole task. Most of the machine becomes unusable, making this a very simple and effective DoS vulnerability.
I cannot imagine why this retry was ever implemented, but it seems rather useless and harmful to me. Let's remove it and fail with ENAMETOOLONG instead.
--- fs/ceph/mds_client.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
fs/ceph/mds_client.c +4 -5
@@ -2808,12 +2808,11 @@ char *ceph_mdsc_build_path(struct ceph_mds_client *mdsc, struct dentry *dentry,if (pos <0 ) {/* -* A rename didn't occur, but somehow we didn't end up where -* we thought we would. Throw a warning and try again. +* The path is longer than PATH_MAX and this function +* cannot ever succeed. Creating paths that long is +* possible with Ceph, but Linux cannot use them. */ -pr_warn_client (cl,"did not end path lookup where expected (pos = %d)\n" , - pos); - goto retry; +return ERR_PTR (-ENAMETOOLONG ); } *pbase = base;
Signature · 1 line
2.45.2
Ilya Dryomov
maintainerQuoted 10 lines · Max Kellermann
If the full path to be built by
ceph_mdsc_build_path()happens to be longer thanPATH_MAX, then this function will enter an endless (retry) loop, effectively blocking the whole task. Most of the machine becomes unusable, making this a very simple and effective DoS vulnerability.I cannot imagine why this retry was ever implemented, but it seems rather useless and harmful to me. Let's remove it and fail with ENAMETOOLONG instead.
Hi Max,
When this was put in place in 2009, I think the idea of a retry was copied from CIFS. Jeff preserved the retry when he massaged this code to not warn in case a rename race is detected [1]. CIFS got rid of it only a couple of years ago [2][3].
Adding Patrick and Venky as well, please chime in.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f5946bcc5e79038f9f7cb66ec25bd3b2d39b2775
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f6a9bc336b600e1266e6eebb0972d75d5b93aea9
[3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=991e72eb0e99764219865b9a3a07328695148e14
Thanks,
Ilya
Quoted 32 lines
Reported-by Dario Weißer <[email protected]>Signed-off-by Max Kellermann <[email protected]>--- fs/ceph/mds_client.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)fs/ceph/mds_client.c +4
-6@@ -2808,12 +2808,11 @@ char *ceph_mdsc_build_path(struct ceph_mds_client *mdsc, struct dentry *dentry,if (pos <0 ) {/* -* A rename didn't occur, but somehow we didn't end up where -* we thought we would. Throw a warning and try again. +* The path is longer than PATH_MAX and this function +* cannot ever succeed. Creating paths that long is +* possible with Ceph, but Linux cannot use them. */ -pr_warn_client (cl,"did not end path lookup where expected (pos = %d)\n" , - pos); - goto retry; +return ERR_PTR (-ENAMETOOLONG ); } *pbase = base; --2.45.2
Quoted 58 lines · Ilya Dryomov
Quoted 10 lines · Max Kellermann
If the full path to be built by
ceph_mdsc_build_path()happens to be longer thanPATH_MAX, then this function will enter an endless (retry) loop, effectively blocking the whole task. Most of the machine becomes unusable, making this a very simple and effective DoS vulnerability.I cannot imagine why this retry was ever implemented, but it seems rather useless and harmful to me. Let's remove it and fail with ENAMETOOLONG instead.
Hi Max,
When this was put in place in 2009, I think the idea of a retry was copied from CIFS. Jeff preserved the retry when he massaged this code to not warn in case a rename race is detected [1]. CIFS got rid of it only a couple of years ago [2][3].
Adding Patrick and Venky as well, please chime in.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f5946bcc5e79038f9f7cb66ec25bd3b2d39b2775
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f6a9bc336b600e1266e6eebb0972d75d5b93aea9
[3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=991e72eb0e99764219865b9a3a07328695148e14Thanks,
Ilya
Quoted 28 lines
Reported-by Dario Weißer <[email protected]>Signed-off-by Max Kellermann <[email protected]>--- fs/ceph/mds_client.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)fs/ceph/mds_client.c +4
-5@@ -2808,12 +2808,11 @@ char *ceph_mdsc_build_path(struct ceph_mds_client *mdsc, struct dentry *dentry,if (pos <0 ) {/* -* A rename didn't occur, but somehow we didn't end up where -* we thought we would. Throw a warning and try again. +* The path is longer than PATH_MAX and this function +* cannot ever succeed. Creating paths that long is +* possible with Ceph, but Linux cannot use them. */ -pr_warn_client (cl,"did not end path lookup where expected (pos = %d)\n" , - pos); - goto retry; +return ERR_PTR (-ENAMETOOLONG ); }
Quoted 4 lines
Quoted 4 lines
fs/ceph/mds_client.c +0
-1*pbase = base; --2.45.2
I think the idea was that if we ended up with a path longer than PATH_MAX that something must have changed in the middle of the reverse pathwalk to make it too long a path, and we should just go and try it again. At the very least, this code should cap the retries to a certain number if you don't just return an error here.
-ENAMETOOLONG could be problematic there. This function is often called when we have a dentry and need to build a path to it to send to the MDS in a call. The system call that caused us to generate this path probably doesn't involve a pathname itself, so the caller may be confused by an -ENAMETOOLONG return.
You may want to go with a more generic error code here -- -EIO or something. It might also be worthwhile to leave in a pr_warn_once or something since there may be users confused by this error return.
Signature · 1 line
Jeff Layton <[email protected]>
Quoted 5 lines · Jeff Layton
-ENAMETOOLONGcould be problematic there. This function is often called when we have a dentry and need to build a path to it to send to the MDS in a call. The system call that caused us to generate this path probably doesn't involve a pathname itself, so the caller may be confused by an-ENAMETOOLONGreturn.
It is unfortunate that the Ceph-MDS protocol requires having to convert a file descriptor back to a path name - but do you really believe EIO would cause less confusion? ENAMETOOLONG is exactly what happens, even if it's an internal error. But there are many error codes that describe internal errors, so there's some prior art.
EIO just doesn't fit, returning EIO would be confusing - even more so because EIO isn't a documented error code for open().
If this is about building path names for sending to the MDS, and not for the userspace ABI, maybe the PATH_MAX limitation is wrong here. If Ceph doesn't have such a limitation, the Ceph code shouldn't use the userspace ABI limit for protocol use.
Quoted 3 lines
You may want to go with a more generic error code here --
-EIOor something. It might also be worthwhile to leave in apr_warn_onceor something since there may be users confused by this error return.
Users cannot read the kernel log, and this allows users to flood the kernel log. So we get all the disadvantages of the kernel log while our users get none of the advantages.
Quoted 7 lines · Max Kellermann
Quoted 5 lines · Jeff Layton
-ENAMETOOLONGcould be problematic there. This function is often called when we have a dentry and need to build a path to it to send to the MDS in a call. The system call that caused us to generate this path probably doesn't involve a pathname itself, so the caller may be confused by an-ENAMETOOLONGreturn.
Quoted 9 lines
It is unfortunate that the Ceph-MDS protocol requires having to convert a file descriptor back to a path name - but do you really believe EIO would cause less confusion? ENAMETOOLONG is exactly what happens, even if it's an internal error. But there are many error codes that describe internal errors, so there's some prior art.
EIO just doesn't fit, returning EIO would be confusing - even more so because EIO isn't a documented error code for
open().
Fair enough. EIO is just my goto when I don't have a better idea.
Probably you want some weird error code that will make people stand up and take notice. Maybe?
#define ENOSR 63 /* Out of streams resources */
Quoted 5 lines
If this is about building path names for sending to the MDS, and not for the userspace ABI, maybe the
PATH_MAXlimitation is wrong here. If Ceph doesn't have such a limitation, the Ceph code shouldn't use the userspace ABI limit for protocol use.
It is possible to build a dir hierarchy that is deeper than can be represented by a PATH_MAX-length string. No reason you can't allocate a bigger buffer there. Does the MDS have a limit on the size of a path string that it'll accept?
Quoted 7 lines
Quoted 3 lines
You may want to go with a more generic error code here --
-EIOor something. It might also be worthwhile to leave in apr_warn_onceor something since there may be users confused by this error return.Users cannot read the kernel log, and this allows users to flood the kernel log. So we get all the disadvantages of the kernel log while our users get none of the advantages.
Sure. I'd make this a pr_warn_once or heavily ratelimit it or something. No need for a lot of these messages, but eventually users are going to wonder why they're getting these weird errors and bug the admins. Giving them this hint might be helpful.
Alternately you could add a conditional tracepoint or something, but those are more obscure.
Signature · 1 line
Jeff Layton <[email protected]>
Quoted 8 lines · Max Kellermann
Quoted 5 lines · Jeff Layton
-ENAMETOOLONGcould be problematic there. This function is often called when we have a dentry and need to build a path to it to send to the MDS in a call. The system call that caused us to generate this path probably doesn't involve a pathname itself, so the caller may be confused by an-ENAMETOOLONGreturn.
Quoted 5 lines
It is unfortunate that the Ceph-MDS protocol requires having to convert a file descriptor back to a path name - but do you really believe EIO would cause less confusion? ENAMETOOLONG is exactly what happens, even if it's an internal error. But there are many error codes that describe internal errors, so there's some prior art.
The protocol does **not** require building the full path for most operations unless it involves a snapshot. For snapshots we have to climb the directory tree until we find the directory with the snapshot. e.g.:
$ tree -a foo/ foo/.snap/ foo/bar/baz/.snap/
foo/
└── bar
└── baz
└── file
foo/.snap/
└── 1
└── bar
└── baz
└── file
foo/bar/baz/.snap/
├── _1_1099511627779
│ └── file
└── 2
└── fileIf you read "file" via foo/.snap/1 you get:
2024-11-19T13:47:23.523+0000 7f9b3b79b640 1 -- [v2:172.21.10.4:6874/192645635,v1:172.21.10.4:6875/192645635] <== client.4417 172.21.10.4:0/1322260999 43506 ==== client_request(client.4417:121 open #0x10000000003//1/bar/baz/file 2024-11-19T13:47:23.524518+0000 caller_uid=1141,
caller_gid=1141{1000,1141,}) ==== 199+0+0 (crc 0 0 0) 0x55acb2d55180 con 0x55acb2b3cc00
and for foo/bar/baz/.snap/2/
2024-11-19T13:47:56.796+0000 7f9b3b79b640 1 -- [v2:172.21.10.4:6874/192645635,v1:172.21.10.4:6875/192645635] <== client.4417 172.21.10.4:0/1322260999 43578 ==== client_request(client.4417:155 open #0x10000000005//2/file 2024-11-19T13:47:56.798370+0000 caller_uid=1141,
caller_gid=1141{1000,1141,}) ==== 191+0+0 (crc 0 0 0) 0x55acb2d56000 con 0x55acb2b3cc00
(Note: the MDS protocol indicates a snapshot in the relative file path via double forward slash.)
If you create "file":
2024-11-19T13:56:56.895+0000 7f9b34f8e640 7 mds.0.server reply_client_request 0 ((0) Success) client_request(client.4467:7 create owner_uid=1141, owner_gid=1141 #0x10000000005/file 2024-11-19T13:56:56.890430+0000 caller_uid=1141,
caller_gid=1141{1000,1141,})
(During path lookups when planning to read the file, the client will usually get read caps so it doesn't need to formally open the file. So this last example uses a create.)
Signature · 5 lines
Patrick Donnelly, Ph.D. He / Him / His Red Hat Partner Engineer IBM, Inc. GPG: 19F28A586F808C2402351B93C3301A3E258DD79D
Quoted 2 lines · Patrick Donnelly
The protocol does **not** require building the full path for most operations unless it involves a snapshot.
We don't use Ceph snapshots, but before today's emergency update, we could shoot down an arbitrary server with a single (unprivileged) system call using this vulnerability.
I'm not sure what your point is, but this vulnerability exists, it works without snapshots and we think it's serious.
Quoted 11 lines · Max Kellermann
Quoted 2 lines · Patrick Donnelly
The protocol does **not** require building the full path for most operations unless it involves a snapshot.
We don't use Ceph snapshots, but before today's emergency update, we could shoot down an arbitrary server with a single (unprivileged) system call using this vulnerability.
I'm not sure what your point is, but this vulnerability exists, it works without snapshots and we think it's serious.
I'm not suggesting there isn't a bug. I'm correcting a misunderstanding.
Signature · 5 lines
Patrick Donnelly, Ph.D. He / Him / His Red Hat Partner Engineer IBM, Inc. GPG: 19F28A586F808C2402351B93C3301A3E258DD79D
Alex Markuze
maintainerIMHO, we should first have a solution for the immediate problem, remove infinite retries and fail early, and cap it at 3 retries in case there is a temporary issue here.
I would use ENAMETOOLONG as the primary error code, as it is the most informative, and couple it with a rate-limited kernel log (pr_warn_once) for debugging without flooding.
I would also open a bug/feature request for a dynamic buffer allocation that bypasses PATH_MAX for protocol-specific paths.
Quoted 24 lines · Patrick Donnelly
Quoted 11 lines · Max Kellermann
Quoted 2 lines · Patrick Donnelly
The protocol does **not** require building the full path for most operations unless it involves a snapshot.
We don't use Ceph snapshots, but before today's emergency update, we could shoot down an arbitrary server with a single (unprivileged) system call using this vulnerability.
I'm not sure what your point is, but this vulnerability exists, it works without snapshots and we think it's serious.
I'm not suggesting there isn't a bug. I'm correcting a misunderstanding.
--
Patrick Donnelly, Ph.D.
He / Him / His
Red Hat Partner Engineer
IBM, Inc.
GPG: 19F28A586F808C2402351B93C3301A3E258DD79D
Alex Markuze
maintainerHey, Folks.
This seems important, so I'm bumping this thread. Max has a valid concern, and this issue must be addressed.
Jeff seems to think keeping at least a few retries might be a good idea.
Max, could you add a cap on the retry count to your original patch? I will discuss the PATH_MAX with Patrick and open a feature request for myself to alleviate the limitation.
Quoted 35 lines · Alex Markuze
IMHO, we should first have a solution for the immediate problem, remove infinite retries and fail early, and cap it at 3 retries in case there is a temporary issue here.
I would use ENAMETOOLONG as the primary error code, as it is the most informative, and couple it with a rate-limited kernel log (pr_warn_once) for debugging without flooding.
I would also open a bug/feature request for a dynamic buffer allocation that bypassesPATH_MAXfor protocol-specific paths.Quoted 24 lines · Patrick Donnelly
Quoted 11 lines · Max Kellermann
Quoted 2 lines · Patrick Donnelly
The protocol does **not** require building the full path for most operations unless it involves a snapshot.
We don't use Ceph snapshots, but before today's emergency update, we could shoot down an arbitrary server with a single (unprivileged) system call using this vulnerability.
I'm not sure what your point is, but this vulnerability exists, it works without snapshots and we think it's serious.
I'm not suggesting there isn't a bug. I'm correcting a misunderstanding.
--
Patrick Donnelly, Ph.D.
He / Him / His
Red Hat Partner Engineer
IBM, Inc.
GPG: 19F28A586F808C2402351B93C3301A3E258DD79D
Quoted 1 line · Alex Markuze
Max, could you add a cap on the retry count to your original patch?
Before I wrote code that's not useful at all: I don't quite get why retry on buffer overflow is necessary at all. It looks like it once seemed to be a useful kludge, but then 1b71fe2efa31 ("ceph analog of cifs build_path_from_dentry() race fix") added the read_seqretry() check which, to my limited understanding, is a more robust implementation of rename detection.
Max
Alex Markuze
maintainerYou and Illia agree on this point. I'll wait for replies and take your original patch into the testing branch unless any concerns are raised.
Quoted 13 lines · Max Kellermann
Quoted 1 line · Alex Markuze
Max, could you add a cap on the retry count to your original patch?
Before I wrote code that's not useful at all: I don't quite get why retry on buffer overflow is necessary at all. It looks like it once seemed to be a useful kludge, but then
1b71fe2efa31("ceph analog of cifsbuild_path_from_dentry()race fix") added theread_seqretry()check which, to my limited understanding, is a more robust implementation of rename detection.Max
Quoted 2 lines · Alex Markuze
You and Illia agree on this point. I'll wait for replies and take your original patch into the testing branch unless any concerns are raised.
How long will you wait? It's been more than two weeks since I reported this DoS vulnerability.
Alex Markuze
maintainerIt's already in a testing branch; what branch are you working on?
Quoted 8 lines · Max Kellermann
Quoted 2 lines · Alex Markuze
You and Illia agree on this point. I'll wait for replies and take your original patch into the testing branch unless any concerns are raised.
How long will you wait? It's been more than two weeks since I reported this DoS vulnerability.
Quoted 1 line · Alex Markuze
It's already in a testing branch; what branch are you working on?
It is? Which one? I checked
https://github.com/ceph/ceph-client/commits/testing/ and it's not there. This is the repository mentioned on
https://docs.ceph.com/en/latest/dev/developer_guide/testing_integration_tests/tests-integration-testing-teuthology-kernel/
Quoted 1 line · Alex Markuze
It's already in a testing branch; what branch are you working on?
I found this on branch "wip-shirnk-crash":
https://github.com/ceph/ceph-client/commit/6cdec9f931e38980eb007d9704c5a24535fb5ec5
- did you mean this branch?
This is my patch; but you removed the commit message, removed the explanation I wrote from the code comment, left the (useless and confusing) log message in, and then claimed authorship for my work.
Alex Markuze
maintainerIllya, this patch is tested and it has my review by.
Quoted 12 lines · Max Kellermann
Quoted 1 line · Alex Markuze
It's already in a testing branch; what branch are you working on?
I found this on branch "wip-shirnk-crash":
https://github.com/ceph/ceph-client/commit/6cdec9f931e38980eb007d9704c5a24535fb5ec5
- did you mean this branch?This is my patch; but you removed the commit message, removed the explanation I wrote from the code comment, left the (useless and confusing) log message in, and then claimed authorship for my work.
Ilya Dryomov
maintainerQuoted 2 lines · Alex Markuze
Illya, this patch is tested and it has my review by.
Max's original patch has been applied (with the authorship, commit message, etc preserved).
Thanks,
Ilya
Quoted 15 lines · Max Kellermann
Quoted 12 lines · Alex Markuze
Quoted 1 line
It's already in a testing branch; what branch are you working on?
I found this on branch "wip-shirnk-crash":
https://github.com/ceph/ceph-client/commit/6cdec9f931e38980eb007d9704c5a24535fb5ec5
- did you mean this branch?This is my patch; but you removed the commit message, removed the explanation I wrote from the code comment, left the (useless and confusing) log message in, and then claimed authorship for my work.