| Message ID | 20220709200005.681861-1-tyhicks@linux.microsoft.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net/9p: Initialize the iounit field during fid creation | expand |
On 2022-07-09 15:00:05, Tyler Hicks wrote: > Ensure that the fid's iounit field is set to zero when a new fid is > created. Certain 9P operations, such as OPEN and CREATE, allow the > server to reply with an iounit size which the client code assigns to the > fid struct shortly after the fid is created in p9_fid_create(). Other > operations that follow a call to p9_fid_create(), such as an XATTRWALK, > don't include an iounit value in the reply message from the server. In > the latter case, the iounit field remained uninitialized. Depending on > allocation patterns, the iounit value could have been something > reasonable that was carried over from previously freed fids or, in the > worst case, could have been arbitrary values from non-fid related usages > of the memory location. > > The bug was detected in the Windows Subsystem for Linux 2 (WSL2) kernel > after the uninitialized iounit field resulted in the typical sequence of > two getxattr(2) syscalls, one to get the size of an xattr and another > after allocating a sufficiently sized buffer to fit the xattr value, to > hit an unexpected ERANGE error in the second call to getxattr(2). An > uninitialized iounit field would sometimes force rsize to be smaller > than the xattr value size in p9_client_read_once() and the 9P server in > WSL refused to chunk up the READ on the attr_fid and, instead, returned > ERANGE to the client. The virtfs server in QEMU seems happy to chunk up > the READ and this problem goes undetected there. However, there are > likely other non-xattr implications of this bug that could cause > inefficient communication between the client and server. > > Cc: stable@vger.kernel.org > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com> > --- > > Note that I haven't had a chance to identify when this bug was > introduced so I don't yet have a proper Fixes tag. The history looked a > little tricky to me but I'll have another look in the coming days. We > started hitting this bug after trying to move from linux-5.10.y to > linux-5.15.y but I didn't see any obvious changes between those two > series. I'm not confident of this theory but perhaps the fid refcounting > changes impacted the fid allocation patterns enough to uncover the > latent bug? From reading the source, I believe that this first showed up in commit ebf46264a004 ("fs/9p: Add support user. xattr") which landed in v2.6.36. Before that commit, p9_client_read(), p9_client_write(), and p9_client_readdir() were always passed a fid that came from a file's private_data and went through the open/create functions that initialized iounit. That commit was the first that passed a fid directly from p9_fid_create() to p9_client_read(). Tyler
On 2022-07-10 01:26:13, Tyler Hicks wrote: > On 2022-07-09 15:00:05, Tyler Hicks wrote: > > Ensure that the fid's iounit field is set to zero when a new fid is > > created. Certain 9P operations, such as OPEN and CREATE, allow the > > server to reply with an iounit size which the client code assigns to the > > fid struct shortly after the fid is created in p9_fid_create(). Other > > operations that follow a call to p9_fid_create(), such as an XATTRWALK, > > don't include an iounit value in the reply message from the server. In > > the latter case, the iounit field remained uninitialized. Depending on > > allocation patterns, the iounit value could have been something > > reasonable that was carried over from previously freed fids or, in the > > worst case, could have been arbitrary values from non-fid related usages > > of the memory location. > > > > The bug was detected in the Windows Subsystem for Linux 2 (WSL2) kernel > > after the uninitialized iounit field resulted in the typical sequence of > > two getxattr(2) syscalls, one to get the size of an xattr and another > > after allocating a sufficiently sized buffer to fit the xattr value, to > > hit an unexpected ERANGE error in the second call to getxattr(2). An > > uninitialized iounit field would sometimes force rsize to be smaller > > than the xattr value size in p9_client_read_once() and the 9P server in > > WSL refused to chunk up the READ on the attr_fid and, instead, returned > > ERANGE to the client. The virtfs server in QEMU seems happy to chunk up > > the READ and this problem goes undetected there. However, there are > > likely other non-xattr implications of this bug that could cause > > inefficient communication between the client and server. ^ I think this last sentence can be removed. I now believe that this only affects xattr get/set operations because nothing else calling the functions that honor iounit is getting the fid directly from a call to p9_fid_create(). > > Please add the following tag: Fixes: ebf46264a004 ("fs/9p: Add support user. xattr") I'm happy to do both of these things in a v2 if any changes/improvements are requested. Thanks! Tyler > > Cc: stable@vger.kernel.org > > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com> > > --- > > > > Note that I haven't had a chance to identify when this bug was > > introduced so I don't yet have a proper Fixes tag. The history looked a > > little tricky to me but I'll have another look in the coming days. We > > started hitting this bug after trying to move from linux-5.10.y to > > linux-5.15.y but I didn't see any obvious changes between those two > > series. I'm not confident of this theory but perhaps the fid refcounting > > changes impacted the fid allocation patterns enough to uncover the > > latent bug? > > From reading the source, I believe that this first showed up in commit > ebf46264a004 ("fs/9p: Add support user. xattr") which landed in v2.6.36. > Before that commit, p9_client_read(), p9_client_write(), and > p9_client_readdir() were always passed a fid that came from a file's > private_data and went through the open/create functions that initialized > iounit. That commit was the first that passed a fid directly from > p9_fid_create() to p9_client_read(). > > Tyler
Le 09/07/2022 à 22:00, Tyler Hicks a écrit : > Ensure that the fid's iounit field is set to zero when a new fid is > created. Certain 9P operations, such as OPEN and CREATE, allow the > server to reply with an iounit size which the client code assigns to the > fid struct shortly after the fid is created in p9_fid_create(). Other > operations that follow a call to p9_fid_create(), such as an XATTRWALK, > don't include an iounit value in the reply message from the server. In > the latter case, the iounit field remained uninitialized. Depending on > allocation patterns, the iounit value could have been something > reasonable that was carried over from previously freed fids or, in the > worst case, could have been arbitrary values from non-fid related usages > of the memory location. > > The bug was detected in the Windows Subsystem for Linux 2 (WSL2) kernel > after the uninitialized iounit field resulted in the typical sequence of > two getxattr(2) syscalls, one to get the size of an xattr and another > after allocating a sufficiently sized buffer to fit the xattr value, to > hit an unexpected ERANGE error in the second call to getxattr(2). An > uninitialized iounit field would sometimes force rsize to be smaller > than the xattr value size in p9_client_read_once() and the 9P server in > WSL refused to chunk up the READ on the attr_fid and, instead, returned > ERANGE to the client. The virtfs server in QEMU seems happy to chunk up > the READ and this problem goes undetected there. However, there are > likely other non-xattr implications of this bug that could cause > inefficient communication between the client and server. > > Cc: stable@vger.kernel.org > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com> > --- > > Note that I haven't had a chance to identify when this bug was > introduced so I don't yet have a proper Fixes tag. The history looked a > little tricky to me but I'll have another look in the coming days. We > started hitting this bug after trying to move from linux-5.10.y to > linux-5.15.y but I didn't see any obvious changes between those two > series. I'm not confident of this theory but perhaps the fid refcounting > changes impacted the fid allocation patterns enough to uncover the > latent bug? > > net/9p/client.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/net/9p/client.c b/net/9p/client.c > index 8bba0d9cf975..1dfceb9154f7 100644 > --- a/net/9p/client.c > +++ b/net/9p/client.c > @@ -899,6 +899,7 @@ static struct p9_fid *p9_fid_create(struct p9_client *clnt) > fid->clnt = clnt; > fid->rdir = NULL; > fid->fid = 0; > + fid->iounit = 0; > refcount_set(&fid->count, 1); > > idr_preload(GFP_KERNEL); Hi, you could also kzalloc 'fid' and remove the memset, "= NULL" and "= 0". This would be even more future proof and would save some LoC. Just my 2c, CJ
Tyler Hicks wrote on Sat, Jul 09, 2022 at 03:00:05PM -0500: > Ensure that the fid's iounit field is set to zero when a new fid is > created. Certain 9P operations, such as OPEN and CREATE, allow the > server to reply with an iounit size which the client code assigns to the > fid struct shortly after the fid is created in p9_fid_create(). Other > operations that follow a call to p9_fid_create(), such as an XATTRWALK, > don't include an iounit value in the reply message from the server. In > the latter case, the iounit field remained uninitialized. Depending on > allocation patterns, the iounit value could have been something > reasonable that was carried over from previously freed fids or, in the > worst case, could have been arbitrary values from non-fid related usages > of the memory location. > > The bug was detected in the Windows Subsystem for Linux 2 (WSL2) kernel > after the uninitialized iounit field resulted in the typical sequence of > two getxattr(2) syscalls, one to get the size of an xattr and another > after allocating a sufficiently sized buffer to fit the xattr value, to > hit an unexpected ERANGE error in the second call to getxattr(2). An > uninitialized iounit field would sometimes force rsize to be smaller > than the xattr value size in p9_client_read_once() and the 9P server in > WSL refused to chunk up the READ on the attr_fid and, instead, returned > ERANGE to the client. The virtfs server in QEMU seems happy to chunk up > the READ and this problem goes undetected there. However, there are > likely other non-xattr implications of this bug that could cause > inefficient communication between the client and server. > > Cc: stable@vger.kernel.org > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com> Thanks for the fix! > --- > > Note that I haven't had a chance to identify when this bug was > introduced so I don't yet have a proper Fixes tag. The history looked a > little tricky to me but I'll have another look in the coming days. We > started hitting this bug after trying to move from linux-5.10.y to > linux-5.15.y but I didn't see any obvious changes between those two > series. I'm not confident of this theory but perhaps the fid refcounting > changes impacted the fid allocation patterns enough to uncover the > latent bug? > > net/9p/client.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/net/9p/client.c b/net/9p/client.c > index 8bba0d9cf975..1dfceb9154f7 100644 > --- a/net/9p/client.c > +++ b/net/9p/client.c > @@ -899,6 +899,7 @@ static struct p9_fid *p9_fid_create(struct p9_client *clnt) > fid->clnt = clnt; > fid->rdir = NULL; > fid->fid = 0; > + fid->iounit = 0; ugh, this isn't the first we've missed so I'll be tempted to agree with Christophe -- let's make that a kzalloc and only set non-zero fields. -- Dominique
On 2022-07-10 22:21:33, Dominique Martinet wrote: > Tyler Hicks wrote on Sat, Jul 09, 2022 at 03:00:05PM -0500: > > Ensure that the fid's iounit field is set to zero when a new fid is > > created. Certain 9P operations, such as OPEN and CREATE, allow the > > server to reply with an iounit size which the client code assigns to the > > fid struct shortly after the fid is created in p9_fid_create(). Other > > operations that follow a call to p9_fid_create(), such as an XATTRWALK, > > don't include an iounit value in the reply message from the server. In > > the latter case, the iounit field remained uninitialized. Depending on > > allocation patterns, the iounit value could have been something > > reasonable that was carried over from previously freed fids or, in the > > worst case, could have been arbitrary values from non-fid related usages > > of the memory location. > > > > The bug was detected in the Windows Subsystem for Linux 2 (WSL2) kernel > > after the uninitialized iounit field resulted in the typical sequence of > > two getxattr(2) syscalls, one to get the size of an xattr and another > > after allocating a sufficiently sized buffer to fit the xattr value, to > > hit an unexpected ERANGE error in the second call to getxattr(2). An > > uninitialized iounit field would sometimes force rsize to be smaller > > than the xattr value size in p9_client_read_once() and the 9P server in > > WSL refused to chunk up the READ on the attr_fid and, instead, returned > > ERANGE to the client. The virtfs server in QEMU seems happy to chunk up > > the READ and this problem goes undetected there. However, there are > > likely other non-xattr implications of this bug that could cause > > inefficient communication between the client and server. > > > > Cc: stable@vger.kernel.org > > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com> > > Thanks for the fix! No problem! > > > --- > > > > Note that I haven't had a chance to identify when this bug was > > introduced so I don't yet have a proper Fixes tag. The history looked a > > little tricky to me but I'll have another look in the coming days. We > > started hitting this bug after trying to move from linux-5.10.y to > > linux-5.15.y but I didn't see any obvious changes between those two > > series. I'm not confident of this theory but perhaps the fid refcounting > > changes impacted the fid allocation patterns enough to uncover the > > latent bug? > > > > net/9p/client.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/net/9p/client.c b/net/9p/client.c > > index 8bba0d9cf975..1dfceb9154f7 100644 > > --- a/net/9p/client.c > > +++ b/net/9p/client.c > > @@ -899,6 +899,7 @@ static struct p9_fid *p9_fid_create(struct p9_client *clnt) > > fid->clnt = clnt; > > fid->rdir = NULL; > > fid->fid = 0; > > + fid->iounit = 0; > > ugh, this isn't the first we've missed so I'll be tempted to agree with > Christophe -- let's make that a kzalloc and only set non-zero fields. Agreed - This is the better approach. V2 will be sent out shortly. Tyler > > -- > Dominique >
diff --git a/net/9p/client.c b/net/9p/client.c index 8bba0d9cf975..1dfceb9154f7 100644 --- a/net/9p/client.c +++ b/net/9p/client.c @@ -899,6 +899,7 @@ static struct p9_fid *p9_fid_create(struct p9_client *clnt) fid->clnt = clnt; fid->rdir = NULL; fid->fid = 0; + fid->iounit = 0; refcount_set(&fid->count, 1); idr_preload(GFP_KERNEL);
Ensure that the fid's iounit field is set to zero when a new fid is created. Certain 9P operations, such as OPEN and CREATE, allow the server to reply with an iounit size which the client code assigns to the fid struct shortly after the fid is created in p9_fid_create(). Other operations that follow a call to p9_fid_create(), such as an XATTRWALK, don't include an iounit value in the reply message from the server. In the latter case, the iounit field remained uninitialized. Depending on allocation patterns, the iounit value could have been something reasonable that was carried over from previously freed fids or, in the worst case, could have been arbitrary values from non-fid related usages of the memory location. The bug was detected in the Windows Subsystem for Linux 2 (WSL2) kernel after the uninitialized iounit field resulted in the typical sequence of two getxattr(2) syscalls, one to get the size of an xattr and another after allocating a sufficiently sized buffer to fit the xattr value, to hit an unexpected ERANGE error in the second call to getxattr(2). An uninitialized iounit field would sometimes force rsize to be smaller than the xattr value size in p9_client_read_once() and the 9P server in WSL refused to chunk up the READ on the attr_fid and, instead, returned ERANGE to the client. The virtfs server in QEMU seems happy to chunk up the READ and this problem goes undetected there. However, there are likely other non-xattr implications of this bug that could cause inefficient communication between the client and server. Cc: stable@vger.kernel.org Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com> --- Note that I haven't had a chance to identify when this bug was introduced so I don't yet have a proper Fixes tag. The history looked a little tricky to me but I'll have another look in the coming days. We started hitting this bug after trying to move from linux-5.10.y to linux-5.15.y but I didn't see any obvious changes between those two series. I'm not confident of this theory but perhaps the fid refcounting changes impacted the fid allocation patterns enough to uncover the latent bug? net/9p/client.c | 1 + 1 file changed, 1 insertion(+)