Skip to contentSkip to conversation

dlm: some checks can underflow

PATCH New Dan Carpenter 2 participants 3 messages Jul 31, 2013

Subsystems (2)
Kernel janitors Distributed lock manager (DLM)
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 dlm: some checks can underflow

This is a static checker fix. We have several places here that check the upper limit without checking for negative numbers. One example of this is in find_rsb().

My static checker marks endian data as user controled so. The "ms->m_header.h_length" variable is tagged as user data because it starts as little endian and we convert it at the start of dlm_receive_buffer(). That means that receive_extralen() returns user controlled data which could be negative. The call tree here is:

-> dlm_receive_buffer()
-> dlm_receive_message()

      -> _receive_message()
         -> receive_request()

            We get "namelen" from receive_extralen(ms);

            -> find_rsb()

It's never perfectly clear how invasive to make a fix like this. Many of the changes in the patch are not needed but I wanted to make things consistent.

Signed-off-by Dan Carpenter <[email protected]>

---
Compile tested only.

fs/dlm/lock.h +5 -4
index 5e0c72e..22c630a 100644
@@ -14,7 +14,7 @@
 #define __LOCK_DOT_H__
 
 void dlm_dump_rsb(struct dlm_rsb *r);
-void dlm_dump_rsb_name(struct dlm_ls *ls, char *name, int len);
+void dlm_dump_rsb_name(struct dlm_ls *ls, char *name, unsigned int len);
 void dlm_print_lkb(struct dlm_lkb *lkb);
 void dlm_receive_message_saved(struct dlm_ls *ls, struct dlm_message *ms,
 			       uint32_t saved_seq);

@@ -29,10 +29,11 @@ void dlm_unlock_recovery(struct dlm_ls *ls);
 void dlm_scan_waiters(struct dlm_ls *ls);
 void dlm_scan_timeout(struct dlm_ls *ls);
 void dlm_adjust_timeouts(struct dlm_ls *ls);
-int dlm_master_lookup(struct dlm_ls *ls, int nodeid, char *name, int len,
-		      unsigned int flags, int *r_nodeid, int *result);
+int dlm_master_lookup(struct dlm_ls *ls, int nodeid, char *name,
+		      unsigned int len, unsigned int flags, int *r_nodeid,
+		      int *result);
 
-int dlm_search_rsb_tree(struct rb_root *tree, char *name, int len,
+int dlm_search_rsb_tree(struct rb_root *tree, char *name, unsigned int len,
 			struct dlm_rsb **r_ret);
 
 void dlm_recover_purge(struct dlm_ls *ls);
fs/dlm/lock.c +30 -23

173 lines not on this page · raw message

maintainer
#1 lore raw

PATCH 1/1 dlm: some checks can underflow

Quoted 19 lines · Dan Carpenter

This is a static checker fix. We have several places here that check the upper limit without checking for negative numbers. One example of this is in find_rsb().

My static checker marks endian data as user controled so. The "ms->m_header.h_length" variable is tagged as user data because it starts as little endian and we convert it at the start of dlm_receive_buffer(). That means that receive_extralen() returns user controlled data which could be negative. The call tree here is:

-> dlm_receive_buffer()
-> dlm_receive_message()

      -> _receive_message()
         -> receive_request()

            We get "namelen" from receive_extralen(ms);

            -> find_rsb()
Quoted 3 lines

It's never perfectly clear how invasive to make a fix like this. Many of the changes in the patch are not needed but I wanted to make things consistent.

If it's negative, I don't think it would pass the h_length validation in dlm_process_incoming_buffer(), but I'm not certain...

fs/dlm/lock.c static int receive_rcom_lock_args(struct dlm_ls *ls, struct dlm_lkb *lkb, · line 5603

Quoted 6 lines
fs/dlm/lock.c +2 -2
-		int lvblen = rc->rc_header.h_length - sizeof(struct dlm_rcom) -
-			 sizeof(struct rcom_lock);
+		unsigned int lvblen = rc->rc_header.h_length -
+			sizeof(struct dlm_rcom) - sizeof(struct rcom_lock);
 		if (lvblen > ls->ls_lvblen)
 			return -EINVAL;

Easier to just change that check to

if (lvblen != ls->ls_lvblen)

	return -EINVAL;

Dave

author
#2 lore raw

PATCH 1/1 dlm: some checks can underflow

Quoted 18 lines · David Teigland
Quoted 17 lines · Dan Carpenter

This is a static checker fix. We have several places here that check the upper limit without checking for negative numbers. One example of this is in find_rsb().

My static checker marks endian data as user controled so. The "ms->m_header.h_length" variable is tagged as user data because it starts as little endian and we convert it at the start of dlm_receive_buffer(). That means that receive_extralen() returns user controlled data which could be negative. The call tree here is:

-> dlm_receive_buffer()
-> dlm_receive_message()

      -> _receive_message()
         -> receive_request()

            We get "namelen" from receive_extralen(ms);
Quoted 8 lines
Quoted 5 lines
            -> find_rsb()

It's never perfectly clear how invasive to make a fix like this. Many of the changes in the patch are not needed but I wanted to make things consistent.

If it's negative, I don't think it would pass the h_length validation in dlm_process_incoming_buffer(), but I'm not certain...

Gar, yeah. We check that:

	if (p->header.h_cmd == DLM_MSG) {
		if (msglen < sizeof(struct dlm_message))
			break;

Which means receive_extralen() can't return negative.

We can drop this patch.

regards,
dan carpenter