Message ID | 20180726081049.10527-1-tomasbortoli@gmail.com |
---|---|
State | Not Applicable, archived |
Delegated to: | David Miller |
Headers | show |
Series | 9p: fix NULL pointer dereferences | expand |
Tomas Bortoli wrote on Thu, Jul 26, 2018: > In p9_fd_create_tcp() and p9_fd_create_unix() it is possible to get > a NULL value in the addr parameter. Return -EINVAL in such cases. Let's refuse that at much higher level, like v9fs_mount() in fs/9p/vfs_super.c I can't think of any valid reason for dev_name to be NULL, it's the target IP or virtio handle.
On Thu, Jul 26, 2018 at 10:17 AM, Dominique Martinet <asmadeus@codewreck.org> wrote: > Tomas Bortoli wrote on Thu, Jul 26, 2018: >> In p9_fd_create_tcp() and p9_fd_create_unix() it is possible to get >> a NULL value in the addr parameter. Return -EINVAL in such cases. > > Let's refuse that at much higher level, like v9fs_mount() in > fs/9p/vfs_super.c > > I can't think of any valid reason for dev_name to be NULL, it's the > target IP or virtio handle. But I think trans=fd allows NULL addr today, no?
Dmitry Vyukov wrote on Thu, Jul 26, 2018: > > Let's refuse that at much higher level, like v9fs_mount() in > > fs/9p/vfs_super.c > > > > I can't think of any valid reason for dev_name to be NULL, it's the > > target IP or virtio handle. > > But I think trans=fd allows NULL addr today, no? Ah, right, I read the patch too fast and read unix_create as fd_create, I never realized there was a unix_create variant... fd legitimately doesn't need a name, you are correct. I'm really curious if anyone ever uses the unix/fd variants for "real" stuff though! (not meaning syzbot isn't real, but I have yet to see anything take advantage of this, even if I could imagine some fun applications by piping the wmii libixp server socket.. and just crashed my laptop trying because of the (fixed) trans put bug.. I have yet to see anyone actually doing this) On the other hand, virtio, rdma and xen all have the same problem, so Thomas, please fix them instead :)
On Thu, Jul 26, 2018 at 11:48 AM, Dominique Martinet <asmadeus@codewreck.org> wrote: > Dmitry Vyukov wrote on Thu, Jul 26, 2018: >> > Let's refuse that at much higher level, like v9fs_mount() in >> > fs/9p/vfs_super.c >> > >> > I can't think of any valid reason for dev_name to be NULL, it's the >> > target IP or virtio handle. >> >> But I think trans=fd allows NULL addr today, no? > > Ah, right, I read the patch too fast and read unix_create as fd_create, > I never realized there was a unix_create variant... > > fd legitimately doesn't need a name, you are correct. > > I'm really curious if anyone ever uses the unix/fd variants for "real" > stuff though! (not meaning syzbot isn't real, but I have yet to see > anything take advantage of this, even if I could imagine some fun > applications by piping the wmii libixp server socket.. and just crashed > my laptop trying because of the (fixed) trans put bug.. I have yet to > see anyone actually doing this) I don't really know any real-world cases, but 9p over fd it looks like a kind of fuse, so perhaps somebody uses it this way. Also, fd allows to use something like sctp socket, and if I am not mistaken, trans=tcp can't even do ipv6 tcp (?). > On the other hand, virtio, rdma and xen all have the same problem, so > Thomas, please fix them instead :)
Dmitry Vyukov wrote on Thu, Jul 26, 2018: > On Thu, Jul 26, 2018 at 11:48 AM, Dominique Martinet > <asmadeus@codewreck.org> wrote: > > I'm really curious if anyone ever uses the unix/fd variants for "real" > > stuff though! (not meaning syzbot isn't real, but I have yet to see > > anything take advantage of this, even if I could imagine some fun > > applications by piping the wmii libixp server socket.. and just crashed > > my laptop trying because of the (fixed) trans put bug.. I have yet to > > see anyone actually doing this) > > I don't really know any real-world cases, but 9p over fd it looks like > a kind of fuse, so perhaps somebody uses it this way. I guess, who knows... > Also, fd allows to use something like sctp socket, and if I am not > mistaken, trans=tcp can't even do ipv6 tcp (?). Yeah... I have a server that supposedly supports it (nfs-ganesha) so it's been on my todo list forever, but frankly I don't know many more people using tcp either... I'd like to at least get this done for rdma eventually but we're stuck with ipv4 for Lustre anyway so this hasn't really been a priority, even if either of the transports are likely a few lines "fix" to make them support both protocols.
On 07/26/2018 11:48 AM, Dominique Martinet wrote: > Dmitry Vyukov wrote on Thu, Jul 26, 2018: >>> Let's refuse that at much higher level, like v9fs_mount() in >>> fs/9p/vfs_super.c >>> >>> I can't think of any valid reason for dev_name to be NULL, it's the >>> target IP or virtio handle. >> >> But I think trans=fd allows NULL addr today, no? > How ? > Ah, right, I read the patch too fast and read unix_create as fd_create, > I never realized there was a unix_create variant... > > fd legitimately doesn't need a name, you are correct. > > I'm really curious if anyone ever uses the unix/fd variants for "real" > stuff though! (not meaning syzbot isn't real, but I have yet to see > anything take advantage of this, even if I could imagine some fun > applications by piping the wmii libixp server socket.. and just crashed > my laptop trying because of the (fixed) trans put bug.. I have yet to > see anyone actually doing this) > > > On the other hand, virtio, rdma and xen all have the same problem, so > Thomas, please fix them instead :) > So just by patching v9fs_mount ?
Tomas Bortoli wrote on Thu, Jul 26, 2018: >> But I think trans=fd allows NULL addr today, no? > > How ? Just using the mount syscall with a NULL dev_name? I haven't checked this syzcaller reproducer but it's probably what it does. p9_fd_create doesn't use 'addr' at all so it's safe to create a 9p mount for trans=fd with no device name, as Dmitry pointed out > > On the other hand, virtio, rdma and xen all have the same problem, so > > Thomas, please fix them instead :) > > So just by patching v9fs_mount ? If we want to preserve the current behaviour for trans=fd (and I don't see why not) we just have to patch all the transports that use the device, that is all .create functions but p9_fd_create() Basically exactly what you did, just for a few more functions - I apparently was a little bit too optimistic thinking we could share this check.
On 07/26/2018 04:21 PM, Dominique Martinet wrote: > Tomas Bortoli wrote on Thu, Jul 26, 2018: >>> But I think trans=fd allows NULL addr today, no? >> >> How ? > > Just using the mount syscall with a NULL dev_name? I haven't checked > this syzcaller reproducer but it's probably what it does. > > p9_fd_create doesn't use 'addr' at all so it's safe to create a 9p mount > for trans=fd with no device name, as Dmitry pointed out > mmh, ok. > >>> On the other hand, virtio, rdma and xen all have the same problem, so >>> Thomas, please fix them instead :) >> >> So just by patching v9fs_mount ? > > If we want to preserve the current behaviour for trans=fd (and I don't > see why not) we just have to patch all the transports that use the > device, that is all .create functions but p9_fd_create() > > Basically exactly what you did, just for a few more functions - I > apparently was a little bit too optimistic thinking we could share > this check. > Does v9fs_mount() knows the transport ahead? Because in that case it'd be possible to check if addr!=NULL && trans!=fd then return error Otherwise, patching all the .create, ok.
On 26 July 2018 at 17:48, Dominique Martinet <asmadeus@codewreck.org> wrote: > Dmitry Vyukov wrote on Thu, Jul 26, 2018: > > > Let's refuse that at much higher level, like v9fs_mount() in > > > fs/9p/vfs_super.c > > > > > > I can't think of any valid reason for dev_name to be NULL, it's the > > > target IP or virtio handle. > > > > But I think trans=fd allows NULL addr today, no? > > Ah, right, I read the patch too fast and read unix_create as fd_create, > I never realized there was a unix_create variant... > > fd legitimately doesn't need a name, you are correct. > > I'm really curious if anyone ever uses the unix/fd variants for "real" > stuff though! I definitely used the unix variant for mounting plan9port servers (which all listen for 9p requests via unix sockets). A long time ago I also experimented with mounting p9p servers from remote machines and I think I might have combined socat with -o trans=fd at one point. But I gave up on it in the end because having a process blocked in read() was preventing my laptop from going to sleep for the duration. And since I was trying to read /event type files the Tread could block for quite some time if no events were posted. I believe 9pfuse had the same issue, so the problem be deeper than v9fs. But anyway I'm on quite a tangent already so I'll stop distracting you all from the work at hand. It's been nice to see 9p still kicking recently! :) > (not meaning syzbot isn't real, but I have yet to see > anything take advantage of this, even if I could imagine some fun > applications by piping the wmii libixp server socket.. > Wait woahhhhh, there's another wmii user left? I thought I was the last one! -sqweek <div dir="ltr">On 26 July 2018 at 17:48, Dominique Martinet <span dir="ltr"><<a href="mailto:asmadeus@codewreck.org" target="_blank">asmadeus@codewreck.org</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">Dmitry Vyukov wrote on Thu, Jul 26, 2018:<br> > > Let's refuse that at much higher level, like v9fs_mount() in<br> > > fs/9p/vfs_super.c<br> > ><br> > > I can't think of any valid reason for dev_name to be NULL, it's the<br> > > target IP or virtio handle.<br> > <br> > But I think trans=fd allows NULL addr today, no?<br> <br> </span>Ah, right, I read the patch too fast and read unix_create as fd_create,<br> I never realized there was a unix_create variant...<br> <br> fd legitimately doesn't need a name, you are correct.<br> <br> I'm really curious if anyone ever uses the unix/fd variants for "real"<br> stuff though! </blockquote><div><br></div><div> <div>I definitely used the unix variant for mounting plan9port servers (which all listen for 9p requests via unix sockets).</div><div><br></div><div>A long time ago I also experimented with mounting p9p servers from remote machines and I think I might have combined socat with -o trans=fd at one point. But I gave up on it in the end because having a process blocked in read() was preventing my laptop from going to sleep for the duration. And since I was trying to read /event type files the Tread could block for quite some time if no events were posted.</div><div><br></div><div>I believe 9pfuse had the same issue, so the problem be deeper than v9fs. But anyway I'm on quite a tangent already so I'll stop distracting you all from the work at hand. It's been nice to see 9p still kicking recently! :)<br></div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">(not meaning syzbot isn't real, but I have yet to see<br> anything take advantage of this, even if I could imagine some fun<br> applications by piping the wmii libixp server socket.. <br></blockquote><div><br></div><div>Wait woahhhhh, there's another wmii user left? I thought I was the last one!</div><div>-sqweek<br></div><div><br></div><br></div></div></div>
Tomas Bortoli wrote on Thu, Jul 26, 2018: > > If we want to preserve the current behaviour for trans=fd (and I don't > > see why not) we just have to patch all the transports that use the > > device, that is all .create functions but p9_fd_create() > > > > Basically exactly what you did, just for a few more functions - I > > apparently was a little bit too optimistic thinking we could share > > this check. > > Does v9fs_mount() knows the transport ahead? Because in that case it'd > be possible to check if addr!=NULL && trans!=fd then return error > > Otherwise, patching all the .create, ok. 9p option parsing is all over the place, split in fs/9p/v9fs, net/9p/client and net/9p/trans_*... Which is actually somewhat of a problem because that means we can't warn on unused option as each parser does not know the full subset of options used, so if someone makes a typo they're on their own to figure it out :/ If you want to factor it in, v9fs_mount does not know which transport is used, but p9_client_create does know - although the functions/structs are all static so you need to check clnt->trans_mod->name with strcmp and I'm honestly not sure that's better than checking in each function.. But, as usual I'm happy as long as it works, so pick your poison :)
On Fri, Jul 27, 2018 at 12:15 AM, Dominique Martinet <asmadeus@codewreck.org> wrote: > Tomas Bortoli wrote on Thu, Jul 26, 2018: >> > If we want to preserve the current behaviour for trans=fd (and I don't >> > see why not) we just have to patch all the transports that use the >> > device, that is all .create functions but p9_fd_create() >> > >> > Basically exactly what you did, just for a few more functions - I >> > apparently was a little bit too optimistic thinking we could share >> > this check. >> >> Does v9fs_mount() knows the transport ahead? Because in that case it'd >> be possible to check if addr!=NULL && trans!=fd then return error >> >> Otherwise, patching all the .create, ok. > > 9p option parsing is all over the place, split in fs/9p/v9fs, > net/9p/client and net/9p/trans_*... Which is actually somewhat of a > problem because that means we can't warn on unused option as each parser > does not know the full subset of options used, so if someone makes a > typo they're on their own to figure it out :/ > > > If you want to factor it in, v9fs_mount does not know which transport is > used, but p9_client_create does know - although the functions/structs > are all static so you need to check clnt->trans_mod->name with strcmp > and I'm honestly not sure that's better than checking in each function.. > > But, as usual I'm happy as long as it works, so pick your poison :) So let's proceed with checking in each transport function?
Dmitry Vyukov wrote on Wed, Aug 08, 2018: > > If you want to factor it in, v9fs_mount does not know which transport is > > used, but p9_client_create does know - although the functions/structs > > are all static so you need to check clnt->trans_mod->name with strcmp > > and I'm honestly not sure that's better than checking in each function.. > > > > But, as usual I'm happy as long as it works, so pick your poison :) > > So let's proceed with checking in each transport function? Yes, he's done it a while ago, just didn't se the in-reply-to field to keep part of the thread but the Reported-by was here: http://lkml.kernel.org/r/20180727110558.5479-1-tomasbortoli@gmail.com This is in linux-next right now as 631263a1b23c71
diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c index 964260265b13..ecfceb659d0c 100644 --- a/net/9p/trans_fd.c +++ b/net/9p/trans_fd.c @@ -941,6 +941,9 @@ p9_fd_create_tcp(struct p9_client *client, const char *addr, char *args) struct sockaddr_in sin_server; struct p9_fd_opts opts; + if (addr == NULL) + return -EINVAL; + err = parse_opts(args, &opts); if (err < 0) return err; @@ -995,6 +998,9 @@ p9_fd_create_unix(struct p9_client *client, const char *addr, char *args) csocket = NULL; + if (addr == NULL) + return -EINVAL; + if (strlen(addr) >= UNIX_PATH_MAX) { pr_err("%s (%d): address too long: %s\n", __func__, task_pid_nr(current), addr);
In p9_fd_create_tcp() and p9_fd_create_unix() it is possible to get a NULL value in the addr parameter. Return -EINVAL in such cases. Signed-off-by: Tomas Bortoli <tomasbortoli@gmail.com> Reported-by: syzbot+1a262da37d3bead15c39@syzkaller.appspotmail.com --- net/9p/trans_fd.c | 6 ++++++ 1 file changed, 6 insertions(+)