* array underflow in receive_SyncParam()?
@ 2012-03-27 7:10 Dan Carpenter
2012-03-27 11:32 ` Philipp Reisner
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2012-03-27 7:10 UTC (permalink / raw)
To: Philipp Reisner; +Cc: drbd-user, linux-kernel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: array underflow in receive_SyncParam()?
2012-03-27 7:10 array underflow in receive_SyncParam()? Dan Carpenter
@ 2012-03-27 11:32 ` Philipp Reisner
2012-03-27 11:43 ` Dan Carpenter
0 siblings, 1 reply; 4+ messages in thread
From: Philipp Reisner @ 2012-03-27 11:32 UTC (permalink / raw)
To: Dan Carpenter, linux-kernel
Am Dienstag, 27. März 2012, 10:10:36 schrieb Dan Carpenter:
> 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.
>
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:
diff --git a/drbd/drbd_receiver.c b/drbd/drbd_receiver.c
index 3ef6130..317d307 100644
--- a/drbd/drbd_receiver.c
+++ b/drbd/drbd_receiver.c
@@ -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
--
: 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.
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: array underflow in receive_SyncParam()?
2012-03-27 11:32 ` Philipp Reisner
@ 2012-03-27 11:43 ` Dan Carpenter
2012-03-28 8:26 ` Philipp Reisner
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2012-03-27 11:43 UTC (permalink / raw)
To: Philipp Reisner; +Cc: linux-kernel
On Tue, Mar 27, 2012 at 01:32:35PM +0200, Philipp Reisner wrote:
> 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: array underflow in receive_SyncParam()?
2012-03-27 11:43 ` Dan Carpenter
@ 2012-03-28 8:26 ` Philipp Reisner
0 siblings, 0 replies; 4+ messages in thread
From: Philipp Reisner @ 2012-03-28 8:26 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-kernel
Am Dienstag, 27. März 2012, 14:43:43 schrieb Dan Carpenter:
> On Tue, Mar 27, 2012 at 01:32:35PM +0200, Philipp Reisner wrote:
> > 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
--
: 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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-03-28 8:26 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-03-27 7:10 array underflow in receive_SyncParam()? Dan Carpenter
2012-03-27 11:32 ` Philipp Reisner
2012-03-27 11:43 ` Dan Carpenter
2012-03-28 8:26 ` Philipp Reisner
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.