[PATCH] 9p: fix NULL pointer dereferences

[PATCH] 9p: fix NULL pointer dereferences

[Tomas Bortoli]

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(+)

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);
-- 
2.11.0

Re: [PATCH] 9p: fix NULL pointer dereferences

[Dominique Martinet]

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.

-- 
Dominique

Re: [PATCH] 9p: fix NULL pointer dereferences

[Dmitry Vyukov]

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?

Re: [PATCH] 9p: fix NULL pointer dereferences

[Dominique Martinet]

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 :)

-- 
Dominique Martinet

Re: [PATCH] 9p: fix NULL pointer dereferences

[Dmitry Vyukov]

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 :)

Re: [PATCH] 9p: fix NULL pointer dereferences

[Dominique Martinet]

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.

-- 
Dominique Martinet

Re: [PATCH] 9p: fix NULL pointer dereferences

[Tomas Bortoli]

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 ?

Re: [PATCH] 9p: fix NULL pointer dereferences

[Dominique Martinet]

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.

-- 
Dominique

Re: [PATCH] 9p: fix NULL pointer dereferences

[Tomas Bortoli]

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.

Re: [V9fs-developer] [PATCH] 9p: fix NULL pointer dereferences

[sqweek]

[-- Attachment #1: Type: text/plain, Size: 1688 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 2410 bytes --]

Re: [PATCH] 9p: fix NULL pointer dereferences

[Dominique Martinet]

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 :)

-- 
Dominique Martinet

Re: [PATCH] 9p: fix NULL pointer dereferences

[Dmitry Vyukov]

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?

Re: [PATCH] 9p: fix NULL pointer dereferences

[Dominique Martinet]

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

-- 
Dominique