staging/lustre/ptlrpc: Removes potential null dereference

PATCH New Lidza Louina 2 participants 3 messages May 12, 2016 – May 16, 2016 Staging

Share & tools

Lidza Louina

author

2016-05-12 22:33 UTC #0 lore raw

PATCH 1/1 staging/lustre/ptlrpc: Removes potential null dereference

The lustre_msg_buf method could return NULL. Subsequent code didn't check if it's null before using it. This patch adds two checks. ????

Signed-off-by Lidza Louina <[email protected]>

drivers/staging/lustre/lustre/ptlrpc/sec.c +0 -0

index 187fd1d..e6fedc3 100644
@@ -2195,6 +2195,8 @@ int sptlrpc_pack_user_desc(struct lustre_msg *msg, int offset)

? struct ptlrpc_user_desc *pud;
?
? pud = lustre_msg_buf(msg, offset, 0);
+ if (!pud)
+ return -EINVAL;
?
? pud->pud_uid = from_kuid(&init_user_ns, current_uid());
? pud->pud_gid = from_kgid(&init_user_ns, current_gid());

drivers/staging/lustre/lustre/ptlrpc/sec_plain.c +0 -0

index 37c9f4c..7736aa9 100644
@@ -542,6 +542,7 @@ int plain_alloc_reqbuf(struct ptlrpc_sec *sec,

?{
? __u32 buflens[PLAIN_PACK_SEGMENTS] = { 0, };
? int alloc_len;
+ int desc;
?
? buflens[PLAIN_PACK_HDR_OFF] = sizeof(struct plain_header);
? buflens[PLAIN_PACK_MSG_OFF] = msgsize;
@@ -575,7 +576,10 @@ int plain_alloc_reqbuf(struct ptlrpc_sec *sec,
? req->rq_reqmsg = lustre_msg_buf(req->rq_reqbuf, PLAIN_PACK_MSG_OFF, 0);
?
? if (req->rq_pack_udesc)
- sptlrpc_pack_user_desc(req->rq_reqbuf, PLAIN_PACK_USER_OFF);
+ desc = sptlrpc_pack_user_desc(req->rq_reqbuf, PLAIN_PACK_USER_OFF);
+ if (!desc){
+ return desc;
+ }
?
? return 0;
?}

Drokin

2016-05-13 16:43 UTC #1 lore raw

PATCH 1/1 staging/lustre/ptlrpc: Removes potential null dereference

Quoted 2 lines · Lidza Louina

The lustre_msg_buf method could return NULL. Subsequent code didn't check if it's null before using it. This patch adds two checks.

Thank you for the patch, it looks good code-wise, but there are some style issues.

Quoted 29 lines

Signed-off-by Lidza Louina <[email protected]>

drivers/staging/lustre/lustre/ptlrpc/sec.c +2 -0

index 187fd1d..e6fedc3 100644
@@ -2195,6 +2195,8 @@ int sptlrpc_pack_user_desc(struct lustre_msg *msg, int offset)
 	struct ptlrpc_user_desc *pud;
 
 	pud = lustre_msg_buf(msg, offset, 0);
+	if (!pud)
+		return -EINVAL;
 
 	pud->pud_uid = from_kuid(&init_user_ns, current_uid());
 	pud->pud_gid = from_kgid(&init_user_ns, current_gid());

drivers/staging/lustre/lustre/ptlrpc/sec_plain.c +1 -0

index 37c9f4c..7736aa9 100644
@@ -542,6 +542,7 @@ int plain_alloc_reqbuf(struct ptlrpc_sec *sec,
 {
 	__u32 buflens[PLAIN_PACK_SEGMENTS] = { 0, };
 	int alloc_len;
+	int desc;
 
 	buflens[PLAIN_PACK_HDR_OFF] = sizeof(struct plain_header);
 	buflens[PLAIN_PACK_MSG_OFF] = msgsize;

drivers/staging/lustre/lustre/ptlrpc/sec_plain.c int plain_alloc_reqbuf(struct ptlrpc_sec *sec, · line 579

Quoted 5 lines

drivers/staging/lustre/lustre/ptlrpc/sec_plain.c +1 -1

@@ -575,7 +576,10 @@ int plain_alloc_reqbuf(struct ptlrpc_sec *sec,
 	req->rq_reqmsg = lustre_msg_buf(req->rq_reqbuf, PLAIN_PACK_MSG_OFF, 0);
 
 	if (req->rq_pack_udesc)
-		sptlrpc_pack_user_desc(req->rq_reqbuf, PLAIN_PACK_USER_OFF);
+		desc = sptlrpc_pack_user_desc(req->rq_reqbuf, PLAIN_PACK_USER_OFF);

This makes the line longer than 80 chars.
Also why not declare desc right here as opposed to the start of the function like you did?

drivers/staging/lustre/lustre/ptlrpc/sec_plain.c int plain_alloc_reqbuf(struct ptlrpc_sec *sec, · line 582

Quoted 3 lines

drivers/staging/lustre/lustre/ptlrpc/sec_plain.c +3 -0
+		if (!desc){
+			return desc;
+		}

We don't really need these curvy braces here.

Thanks!

Lidza Louina

author

2016-05-16 14:11 UTC #2 lore raw

PATCH 1/1 staging/lustre/ptlrpc: Removes potential null dereference

Quoted 43 lines · Oleg

Quoted 2 lines · Lidza Louina
The lustre_msg_buf method could return NULL. Subsequent code didn't check if it's null before using it. This patch adds two checks.
Thank you for the patch, it looks good code-wise, but there are some style issues.
Quoted 29 lines
Signed-off-by Lidza Louina <[email protected]>
drivers/staging/lustre/lustre/ptlrpc/sec.c +2 -0
index 187fd1d..e6fedc3 100644
@@ -2195,6 +2195,8 @@ int sptlrpc_pack_user_desc(struct lustre_msg *msg, int offset)
  	struct ptlrpc_user_desc *pud;
 
  	pud = lustre_msg_buf(msg, offset, 0);
+	if (!pud)
+		return -EINVAL;
 
  	pud->pud_uid = from_kuid(&init_user_ns, current_uid());
  	pud->pud_gid = from_kgid(&init_user_ns, current_gid());
drivers/staging/lustre/lustre/ptlrpc/sec_plain.c +1 -0
index 37c9f4c..7736aa9 100644
@@ -542,6 +542,7 @@ int plain_alloc_reqbuf(struct ptlrpc_sec *sec,
  {
  	__u32 buflens[PLAIN_PACK_SEGMENTS] = { 0, };
  	int alloc_len;
+	int desc;
 
  	buflens[PLAIN_PACK_HDR_OFF] = sizeof(struct plain_header);
  	buflens[PLAIN_PACK_MSG_OFF] = msgsize;
drivers/staging/lustre/lustre/ptlrpc/sec_plain.c int plain_alloc_reqbuf(struct ptlrpc_sec *sec, · line 579
Quoted 5 lines
drivers/staging/lustre/lustre/ptlrpc/sec_plain.c +1 -1
@@ -575,7 +576,10 @@ int plain_alloc_reqbuf(struct ptlrpc_sec *sec,
  	req->rq_reqmsg = lustre_msg_buf(req->rq_reqbuf, PLAIN_PACK_MSG_OFF, 0);
 
  	if (req->rq_pack_udesc)
-		sptlrpc_pack_user_desc(req->rq_reqbuf, PLAIN_PACK_USER_OFF);
+		desc = sptlrpc_pack_user_desc(req->rq_reqbuf, PLAIN_PACK_USER_OFF);
This makes the line longer than 80 chars.
Also why not declare desc right here as opposed to the start of the function like you did?

Quoted 8 lines

drivers/staging/lustre/lustre/ptlrpc/sec_plain.c int plain_alloc_reqbuf(struct ptlrpc_sec *sec, · line 582
Quoted 3 lines
drivers/staging/lustre/lustre/ptlrpc/sec_plain.c +3 -0
+		if (!desc){
+			return desc;
+		}
We don't really need these curvy braces here.
Thanks!

Thanks for the feedback. I'm sending a new patch in now.

Lidza