array underflow in receive_SyncParam()?

array underflow in receive_SyncParam()?

[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.

regards,
dan carpenter

Re: array underflow in receive_SyncParam()?

[Philipp Reisner]

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.

Re: array underflow in receive_SyncParam()?

[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?

regards,
dan carpenter

Re: array underflow in receive_SyncParam()?

[Philipp Reisner]

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.