[Cluster-devel] [PATCH] checking NULL pointer in device_write of dlm-control

[Cluster-devel] [PATCH] checking NULL pointer in device_write of dlm-control

[Masatake YAMATO]

Hi,

I found a way to let linux dereference NULL pointer
in gfs2-2.6-nmw/fs/dlm/user.c. 

If `device_write' method is called via "dlm-control", 
file->private_data is NULL. (See ctl_device_open() in 
user.c. ) Through proc->flags is read:

	if ((kbuf->cmd == DLM_USER_LOCK || kbuf->cmd == DLM_USER_UNLOCK) &&
	    test_bit(DLM_PROC_FLAGS_CLOSING, &proc->flags))
		return -EINVAL;

It causes following message on my Fedora 9 PC:

    BUG: unable to handle kernel NULL pointer dereference at 00000004
    IP: [<f8f555df>] :dlm:device_write+0xa5/0x432
    *pde = 7f11b067 
    Oops: 0000 [#2] SMP 
    Modules linked in: ...<snipped>

    Pid: 26899, comm: a.out Tainted: G      D  (2.6.25-14.fc9.i686 #1)
    EIP: 0060:[<f8f555df>] EFLAGS: 00210297 CPU: 1
    EIP is at device_write+0xa5/0x432 [dlm]
    EAX: f66ad200 EBX: f66ad280 ECX: 00000000 EDX: 00000006
    ESI: 00000064 EDI: 00000000 EBP: c8a45f70 ESP: c8a45f44
     DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
    Process a.out (pid: 26899, ti=c8a45000 task=c8a72000 task.ti=c8a45000)
    Stack: bfe90b34 f66ad280 00000000 c8a45f58 c04cc41c c8a45f70 c0482bd5 00000001 
	   def7a0c0 f8f5553a 00000064 c8a45f90 c04832bb c8a45f9c bfe90b34 c04817e7 
	   def7a0c0 fffffff7 080483a0 c8a45fb0 c04833f8 c8a45f9c 00000000 00000000 
    Call Trace:
     [<c04cc41c>] ? security_file_permission+0xf/0x11
     [<c0482bd5>] ? rw_verify_area+0x76/0x97
     [<f8f5553a>] ? device_write+0x0/0x432 [dlm]
     [<c04832bb>] ? vfs_write+0x8a/0x12e
     [<c04817e7>] ? do_sys_open+0xab/0xb5
     [<c04833f8>] ? sys_write+0x3b/0x60
     [<c0405bf2>] ? syscall_call+0x7/0xb
     [<c0620000>] ? acpi_pci_root_add+0x22f/0x2a0
     =======================
    Code: <snipped>
    EIP: [<f8f555df>] device_write+0xa5/0x432 [dlm] SS:ESP 0068:c8a45f44
    ---[ end trace 74c3a9c3bd1a789d ]---
    [yamato at localhost dlm-crash]$ 



Here is a patch.

Signed-off-by: Masatake YAMATO <yamato@redhat.com>

diff --git a/fs/dlm/user.c b/fs/dlm/user.c
index ebbcf38..1aa76b3 100644
--- a/fs/dlm/user.c
+++ b/fs/dlm/user.c
@@ -538,7 +538,7 @@ static ssize_t device_write(struct file *file, const char __user *buf,
 
 	/* do we really need this? can a write happen after a close? */
 	if ((kbuf->cmd == DLM_USER_LOCK || kbuf->cmd == DLM_USER_UNLOCK) &&
-	    test_bit(DLM_PROC_FLAGS_CLOSING, &proc->flags))
+	    (proc && test_bit(DLM_PROC_FLAGS_CLOSING, &proc->flags)))
 		return -EINVAL;
 
 	sigfillset(&allsigs);

[Cluster-devel] [PATCH] checking NULL pointer in device_write of dlm-control

[David Teigland]

On Wed, May 28, 2008 at 02:45:10PM +0900, Masatake YAMATO wrote:
> Hi,
> 
> I found a way to let linux dereference NULL pointer
> in gfs2-2.6-nmw/fs/dlm/user.c. 
> 
> If `device_write' method is called via "dlm-control", 
> file->private_data is NULL. (See ctl_device_open() in 
> user.c. ) Through proc->flags is read:
> 
> 	if ((kbuf->cmd == DLM_USER_LOCK || kbuf->cmd == DLM_USER_UNLOCK) &&
> 	    test_bit(DLM_PROC_FLAGS_CLOSING, &proc->flags))
> 		return -EINVAL;

Thanks for the patch, I'll push it out shortly.

Dave

[Cluster-devel] [PATCH] set NULL to sock field of connection if listen failed

[Masatake YAMATO]

Hi,


I found lines where a `sock' is released twice in dlm 
unexpectedly.

See tcp_create_listen_sock() of gfs2-2.6-nmw/fs/dlm/lowcomms.c:


	result = sock->ops->listen(sock, 5);
	if (result < 0) {
		log_print("Can't listen on port %d", dlm_config.ci_tcp_port);
		sock_release(sock);
		sock = NULL;
		goto create_out;
	}

When listen is failed, sock is released by calling sock_release.
This is ok. However, the sock still occupies con->sock.


See dlm_lowcomms_start() of the file:


	    if (dlm_config.ci_protocol == 0)
		    error = tcp_listen_for_all();
	    else
		    error = sctp_listen_for_all();
	    if (error)
		    goto fail_unlisten;

	    error = work_start();
	    if (error)
		    goto fail_unlisten;

	    return 0;

    fail_unlisten:
	    con = nodeid2con(0,0);
	    if (con) {
		    close_connection(con, false);
		    kmem_cache_free(con_cache, con);
	    }

When tcp_listen_for_all returns an error, close_connection is
called. The failure of listen call causes tcp_listen_for_all returns
an error.

See close_connection:


    static void close_connection(struct connection *con, bool and_other)
    {
	    mutex_lock(&con->sock_mutex);

	    if (con->sock) {
		    sock_release(con->sock);
		    con->sock = NULL;
	    }

If con->sock is not NULL, sock_release is called again on con->sock.
Attached patch fixes this.


Signed-off-by: Masatake YAMATO <yamato@redhat.com>

diff --git a/fs/dlm/lowcomms.c b/fs/dlm/lowcomms.c
index 637018c..9ff9c5c 100644
--- a/fs/dlm/lowcomms.c
+++ b/fs/dlm/lowcomms.c
@@ -994,6 +994,7 @@ static struct socket *tcp_create_listen_sock(struct connection *con,
 		log_print("Can't listen on port %d", dlm_config.ci_tcp_port);
 		sock_release(sock);
 		sock = NULL;
+		con->sock = NULL;
 		goto create_out;
 	}
 

[Cluster-devel] Re: [PATCH] set NULL to sock field of connection if listen failed

[Masatake YAMATO]

Please ignore the patch I sent with this Subject.
I found dlm is maintained in its own git repository.
I looked at gfs2 git repository.

Masatake YAMATO

[Cluster-devel] [PATCH] release sock in tcp_connect_to_sock of dlm if dlm_nodeid_to_addr returns error

[Masatake YAMATO]

Hi,

It seems that `sock' allocated by sock_create_kern
in tcp_connect_to_sock() of dlm/fs/lowcomms.c is not released if
dlm_nodeid_to_addr an error.



    static void tcp_connect_to_sock(struct connection *con)
    {
    ...

	result = sock_create_kern(dlm_local_addr[0]->ss_family, SOCK_STREAM,
				  IPPROTO_TCP, &sock);
	if (result < 0)
		goto out_err;

	memset(&saddr, 0, sizeof(saddr));
	if (dlm_nodeid_to_addr(con->nodeid, &saddr)) {
		sock_release(sock);
		goto out_err;
	}

	...

    out_err:
	    if (con->sock) {
		    sock_release(con->sock);
		    con->sock = NULL;
	    }


Signed-off-by: Masatake YAMATO <yamato@redhat.com>

diff --git a/fs/dlm/lowcomms.c b/fs/dlm/lowcomms.c
index 637018c..3962262 100644
--- a/fs/dlm/lowcomms.c
+++ b/fs/dlm/lowcomms.c
@@ -891,8 +891,10 @@ static void tcp_connect_to_sock(struct connection *con)
 		goto out_err;
 
 	memset(&saddr, 0, sizeof(saddr));
-	if (dlm_nodeid_to_addr(con->nodeid, &saddr))
+	if (dlm_nodeid_to_addr(con->nodeid, &saddr)) {
+		sock_release(sock);
 		goto out_err;
+	}
 
 	sock->sk->sk_user_data = con;
 	con->rx_action = receive_from_sock;