array underflow in receive_SyncParam()?
I had a question about the following code:
drivers/block/drbd/drbd_receiver.c
2808 if (apv == 88) {
2809 if (data_size > SHARED_SECRET_MAX) {
2810 dev_err(DEV, "verify-alg too long, "
2811 "peer wants %u, accepting only %u byte\n",
2812 data_size, SHARED_SECRET_MAX);
2813 return false;
2814 }
2815
2816 if (drbd_recv(mdev, p->verify_alg, data_size) != data_size)
2817 return false;
2818
2819 /* we expect NUL terminated string */
2820 /* but just in case someone tries to be evil */
2821 D_ASSERT(p->verify_alg[data_size-1] == 0);
2822 p->verify_alg[data_size-1] = 0;
^^^^^^^^^Is it possible for data_size to be zero here leading to an array underflow? We test for overflows, but I don't see any place where we test for zero.
regards,
dan carpenter
Am Dienstag, 27. März 2012, 10:10:36 schrieb Dan Carpenter:
Quoted 18 lines
I had a question about the following code:
drivers/block/drbd/drbd_receiver.c
2808 if (apv == 88) {
2809 if (data_size>SHARED_SECRET_MAX) {
2810 dev_err(DEV, "verify-alg too long, "
2811 "peer wants %u, accepting only %u
byte\n", 2812data_size,SHARED_SECRET_MAX); 2813 return false;
2814 }
2815
2816 if (drbd_recv(mdev,p->verify_alg,data_size) !=data_size) 2817 return
false;
2818
2819 /* we expect NUL terminated string */
2820 /* but just in case someone tries to be evil
*/ 2821D_ASSERT(p->verify_alg[data_size-1] == 0);
Quoted 6 lines
2822 p->verify_alg[data_size-1] = 0; ^^^^^^^^^Is it possible for
data_sizeto be zero here leading to an array underflow? We test for overflows, but I don't see any place where we test for zero.
Hi Dan,
You are right, we are relying on the fact that DRBD peers that use the protocol 88 send a positive data_size (what they do).
But if we consider a modified peer, then this is a bug. Suggesting the following patch:
drbd/drbd_receiver.c +0 -0
@@ -3086,9 +3086,9 @@ STATIC int receive_SyncParam(struct drbd_conf *mdev,
enum drbd_packets cmd, unsi
if (apv >= 88) {
if (apv == 88) {- if (data_size > SHARED_SECRET_MAX) {
- dev_err(DEV, "verify-alg too long, "
- "peer wants %u, accepting only %u byte\n",
+ if (data_size > SHARED_SECRET_MAX || data_size == 0) {
+ dev_err(DEV, "verify-alg of wrong size, "
+ "peer wants %u, accepting only %u
byte\n",
data_size, SHARED_SECRET_MAX);
return false;
}Best,
Phil
Signature · 6 lines
: Dipl-Ing Philipp Reisner : LINBIT | Your Way to High Availability : Tel: +43-1-8178292-50, Fax: +43-1-8178292-82 : http://www.linbit.com DRBD(R) and LINBIT(R) are registered trademarks of LINBIT, Austria.
Quoted 7 lines · Philipp Reisner
Hi Dan,
You are right, we are relying on the fact that DRBD peers that use the protocol 88 send a positive
data_size(what they do).But if we consider a modified peer, then this is a bug. Suggesting the following patch:
Looks good to me. Could you give me a Reported-by tag when you send this?
regards,
dan carpenter
Am Dienstag, 27. März 2012, 14:43:43 schrieb Dan Carpenter:
Quoted 12 lines · Philipp Reisner
Quoted 8 lines
Hi Dan,
You are right, we are relying on the fact that DRBD peers that use the protocol 88 send a positive
data_size(what they do).But if we consider a modified peer, then this is a bug. Suggesting
the following patch:
Looks good to me. Could you give me a Reported-by tag when you send this?
Sure. Right now it is here, in out out-of-tree repo.
http://git.drbd.org/gitweb.cgi?p=drbd-8.3.git;a=commit;h=02d3c4dd5da22e8a2e8c953fe20f32de1124fedb
Best,
Phil
Signature · 6 lines
: Dipl-Ing Philipp Reisner : LINBIT | Your Way to High Availability : Tel: +43-1-8178292-50, Fax: +43-1-8178292-82 : http://www.linbit.com DRBD(R) and LINBIT(R) are registered trademarks of LINBIT, Austria.