[PATCH] char: pcmcia: fix possible array index out of bounds in set_protocol()

[PATCH] char: pcmcia: fix possible array index out of bounds in set_protocol()

[Yu Kuai]

The length of array 'pts_reply' is 4, and the loop in set_protocol()
will access array element from 0 to num_bytes_read - 1. Thus if
io_read_num_rec_bytes() gets 'num_bytes_read' more than 4, it will
cause index out of bounds errors.

Fixes: c1986ee9bea3 ("[PATCH] New Omnikey Cardman 4000 driver")
Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 drivers/char/pcmcia/cm4000_cs.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/char/pcmcia/cm4000_cs.c b/drivers/char/pcmcia/cm4000_cs.c
index 89681f07bc78..86b7c8e44198 100644
--- a/drivers/char/pcmcia/cm4000_cs.c
+++ b/drivers/char/pcmcia/cm4000_cs.c
@@ -564,16 +564,15 @@ static int set_protocol(struct cm4000_dev *dev, struct ptsreq *ptsreq)
 
 	/* Read PPS reply */
 	DEBUGP(5, dev, "Read PPS reply\n");
-	for (i = 0; i < num_bytes_read; i++) {
+	for (i = 0; i < 4; i++) {
 		xoutb(i, REG_BUF_ADDR(iobase));
 		pts_reply[i] = inb(REG_BUF_DATA(iobase));
 	}
 
 #ifdef CM4000_DEBUG
 	DEBUGP(2, dev, "PTSreply: ");
-	for (i = 0; i < num_bytes_read; i++) {
+	for (i = 0; i < 4; i++)
 		pr_debug("0x%.2x ", pts_reply[i]);
-	}
 	pr_debug("\n");
 #endif	/* CM4000_DEBUG */
 
-- 
2.25.4

Re: [PATCH] char: pcmcia: fix possible array index out of bounds in set_protocol()

[Greg KH]

On Fri, May 21, 2021 at 06:07:05PM +0800, Yu Kuai wrote:
> The length of array 'pts_reply' is 4, and the loop in set_protocol()
> will access array element from 0 to num_bytes_read - 1. Thus if
> io_read_num_rec_bytes() gets 'num_bytes_read' more than 4, it will
> cause index out of bounds errors.

And how can num_bytes_read be greater than 4?

Ah, it is tested, but you might want to error out if that happens, as
obviously something went wrong.

Do you have this hardware to test these changes?

thanks,

greg k-h

Re: [PATCH] char: pcmcia: fix possible array index out of bounds in set_protocol()

[yukuai (C)]

On 2021/05/21 18:59, Greg KH wrote:
> On Fri, May 21, 2021 at 06:07:05PM +0800, Yu Kuai wrote:
>> The length of array 'pts_reply' is 4, and the loop in set_protocol()
>> will access array element from 0 to num_bytes_read - 1. Thus if
>> io_read_num_rec_bytes() gets 'num_bytes_read' more than 4, it will
>> cause index out of bounds errors.
> 
> And how can num_bytes_read be greater than 4?

Hi

Do you mean num_bytes_read here should never be greater than 4?

544                 io_read_num_rec_bytes(iobase, &num_bytes_read);
545                 if (num_bytes_read >= 4) {
546                         DEBUGP(2, dev, "NumRecBytes = %i\n", 
num_bytes_read);
547                         break;


> 
> Ah, it is tested, but you might want to error out if that happens, as
> obviously something went wrong.
> 
> Do you have this hardware to test these changes?

Sorry we don't have this hardware...

Thanks,
Yu Kuai
> 
> thanks,
> 
> greg k-h
> .
> 

Re: [PATCH] char: pcmcia: fix possible array index out of bounds in set_protocol()

[Greg KH]

On Fri, May 21, 2021 at 07:34:52PM +0800, yukuai (C) wrote:
> On 2021/05/21 18:59, Greg KH wrote:
> > On Fri, May 21, 2021 at 06:07:05PM +0800, Yu Kuai wrote:
> > > The length of array 'pts_reply' is 4, and the loop in set_protocol()
> > > will access array element from 0 to num_bytes_read - 1. Thus if
> > > io_read_num_rec_bytes() gets 'num_bytes_read' more than 4, it will
> > > cause index out of bounds errors.
> > 
> > And how can num_bytes_read be greater than 4?
> 
> Hi
> 
> Do you mean num_bytes_read here should never be greater than 4?
> 
> 544                 io_read_num_rec_bytes(iobase, &num_bytes_read);
> 545                 if (num_bytes_read >= 4) {
> 546                         DEBUGP(2, dev, "NumRecBytes = %i\n",
> num_bytes_read);
> 547                         break;


Like I said, it is tested if it is bigger, but then nothing happens if
that is true.  So try to error out here.

> > Ah, it is tested, but you might want to error out if that happens, as
> > obviously something went wrong.
> > 
> > Do you have this hardware to test these changes?
> 
> Sorry we don't have this hardware...

Ah, then I wouldn't worry about it, as odds are, no one else does either :)

thanks,

greg k-h

[PATCH V2] char: pcmcia: error out if 'num_bytes_read' is greater than 4 in set_protocol()

[Yu Kuai]

Theoretically, it will cause index out of bounds error if
'num_bytes_read' is greater than 4. As we expect it(and was tested)
never to be greater than 4, error out if it happens.

Fixes: c1986ee9bea3 ("[PATCH] New Omnikey Cardman 4000 driver")
Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 drivers/char/pcmcia/cm4000_cs.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/char/pcmcia/cm4000_cs.c b/drivers/char/pcmcia/cm4000_cs.c
index 89681f07bc78..9468e9520cee 100644
--- a/drivers/char/pcmcia/cm4000_cs.c
+++ b/drivers/char/pcmcia/cm4000_cs.c
@@ -544,6 +544,10 @@ static int set_protocol(struct cm4000_dev *dev, struct ptsreq *ptsreq)
 		io_read_num_rec_bytes(iobase, &num_bytes_read);
 		if (num_bytes_read >= 4) {
 			DEBUGP(2, dev, "NumRecBytes = %i\n", num_bytes_read);
+			if (num_bytes_read > 4) {
+				rc = -EIO;
+				goto exit_setprotocol;
+			}
 			break;
 		}
 		usleep_range(10000, 11000);
-- 
2.25.4