2021-06-24 10:53:48

by lijian_8010a29

[permalink] [raw]
Subject: [PATCH] spi: spi-topcliff-pch: Fixed the possible null pointer exception issue

From: lijian <[email protected]>

The 'data->pkt_tx_buff' is used after called
‘kfree(data->pkt_tx_buff)’,it may be null when it is called,
and null pointer exception may occur,
so judgment is added when using 'data->pkt_tx_buff'.

Signed-off-by: lijian <[email protected]>
---
drivers/spi/spi-topcliff-pch.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi-topcliff-pch.c b/drivers/spi/spi-topcliff-pch.c
index b8870784fc6e..a5ac59f2eb80 100644
--- a/drivers/spi/spi-topcliff-pch.c
+++ b/drivers/spi/spi-topcliff-pch.c
@@ -599,7 +599,7 @@ static void pch_spi_set_tx(struct pch_spi_data *data, int *bpw)
}

/* copy Tx Data */
- if (data->cur_trans->tx_buf != NULL) {
+ if ((data->cur_trans->tx_buf != NULL) && (data->pkt_tx_buff != NULL)) {
if (*bpw == 8) {
tx_buf = data->cur_trans->tx_buf;
for (j = 0; j < data->bpw_len; j++)
@@ -621,8 +621,10 @@ static void pch_spi_set_tx(struct pch_spi_data *data, int *bpw)
__func__);
pch_spi_writereg(data->master, PCH_SSNXCR, SSN_LOW);

- for (j = 0; j < n_writes; j++)
- pch_spi_writereg(data->master, PCH_SPDWR, data->pkt_tx_buff[j]);
+ if (data->pkt_tx_buff != NULL) {
+ for (j = 0; j < n_writes; j++)
+ pch_spi_writereg(data->master, PCH_SPDWR, data->pkt_tx_buff[j]);
+ }

/* update tx_index */
data->tx_index = j;
--
2.25.1



2021-06-24 11:45:36

by Mark Brown

[permalink] [raw]
Subject: Re: [PATCH] spi: spi-topcliff-pch: Fixed the possible null pointer exception issue

On Thu, Jun 24, 2021 at 06:50:56PM +0800, [email protected] wrote:
> From: lijian <[email protected]>
>
> The 'data->pkt_tx_buff' is used after called
> ‘kfree(data->pkt_tx_buff)’,it may be null when it is called,
> and null pointer exception may occur,
> so judgment is added when using 'data->pkt_tx_buff'.

> - if (data->cur_trans->tx_buf != NULL) {
> + if ((data->cur_trans->tx_buf != NULL) && (data->pkt_tx_buff != NULL)) {
> if (*bpw == 8) {
> tx_buf = data->cur_trans->tx_buf;
> for (j = 0; j < data->bpw_len; j++)
> @@ -621,8 +621,10 @@ static void pch_spi_set_tx(struct pch_spi_data *data, int *bpw)
> __func__);
> pch_spi_writereg(data->master, PCH_SSNXCR, SSN_LOW);
>
> - for (j = 0; j < n_writes; j++)
> - pch_spi_writereg(data->master, PCH_SPDWR, data->pkt_tx_buff[j]);
> + if (data->pkt_tx_buff != NULL) {
> + for (j = 0; j < n_writes; j++)

I've not checked to make sure that the dereference can happen but if it
does and these checks prevent dereferences it seems that they do so by
simply skipping the writes that were happening which will result in the
driver just silently dropping data, probably creating serious problems
for whatever SPI device is attached. We should ideally carry on with
the transmit, or at the very least return an error if things fail.


Attachments:
(No filename) (1.31 kB)
signature.asc (499.00 B)
Download all attachments

2021-06-25 12:48:09

by Dan Carpenter

[permalink] [raw]
Subject: Re: [PATCH] spi: spi-topcliff-pch: Fixed the possible null pointer exception issue

Hi,

url: https://github.com/0day-ci/linux/commits/lijian_8010a29-163-com/spi-spi-topcliff-pch-Fixed-the-possible-null-pointer-exception-issue/20210624-185333
base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
config: i386-randconfig-m021-20210622 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
Reported-by: Dan Carpenter <[email protected]>

New smatch warnings:
drivers/spi/spi-topcliff-pch.c:632 pch_spi_set_tx() error: uninitialized symbol 'j'.

Old smatch warnings:
drivers/spi/spi-topcliff-pch.c:1253 pch_spi_process_messages() warn: variable dereferenced before check 'data->cur_trans' (see line 1201)

vim +/j +632 drivers/spi/spi-topcliff-pch.c

c37f3c2749b5322 drivers/spi/spi-topcliff-pch.c Tomoya MORINAGA 2011-06-07 544 static void pch_spi_set_tx(struct pch_spi_data *data, int *bpw)
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 545 {
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 546 int size;
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 547 u32 n_writes;
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 548 int j;
cd8d984f0def2a8 drivers/spi/spi-topcliff-pch.c Wei Yongjun 2013-04-27 549 struct spi_message *pmsg, *tmp;
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 550 const u8 *tx_buf;
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 551 const u16 *tx_sbuf;
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 552
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 553 /* set baud rate if needed */
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 554 if (data->cur_trans->speed_hz) {
65308c46b760bb2 drivers/spi/spi_topcliff_pch.c Grant Likely 2010-09-29 555 dev_dbg(&data->master->dev, "%s:setting baud rate\n", __func__);
65308c46b760bb2 drivers/spi/spi_topcliff_pch.c Grant Likely 2010-09-29 556 pch_spi_set_baud_rate(data->master, data->cur_trans->speed_hz);
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 557 }
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 558
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 559 /* set bits per word if needed */
65308c46b760bb2 drivers/spi/spi_topcliff_pch.c Grant Likely 2010-09-29 560 if (data->cur_trans->bits_per_word &&
65308c46b760bb2 drivers/spi/spi_topcliff_pch.c Grant Likely 2010-09-29 561 (data->current_msg->spi->bits_per_word != data->cur_trans->bits_per_word)) {
65308c46b760bb2 drivers/spi/spi_topcliff_pch.c Grant Likely 2010-09-29 562 dev_dbg(&data->master->dev, "%s:set bits per word\n", __func__);
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 563 pch_spi_set_bits_per_word(data->master,
65308c46b760bb2 drivers/spi/spi_topcliff_pch.c Grant Likely 2010-09-29 564 data->cur_trans->bits_per_word);
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 565 *bpw = data->cur_trans->bits_per_word;
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 566 } else {
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 567 *bpw = data->current_msg->spi->bits_per_word;
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 568 }
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 569
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 570 /* reset Tx/Rx index */
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 571 data->tx_index = 0;
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 572 data->rx_index = 0;
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 573
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 574 data->bpw_len = data->cur_trans->len / (*bpw / 8);
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 575
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 576 /* find alloc size */
65308c46b760bb2 drivers/spi/spi_topcliff_pch.c Grant Likely 2010-09-29 577 size = data->cur_trans->len * sizeof(*data->pkt_tx_buff);
65308c46b760bb2 drivers/spi/spi_topcliff_pch.c Grant Likely 2010-09-29 578
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 579 /* allocate memory for pkt_tx_buff & pkt_rx_buffer */
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 580 data->pkt_tx_buff = kzalloc(size, GFP_KERNEL);
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 581 if (data->pkt_tx_buff != NULL) {
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 582 data->pkt_rx_buff = kzalloc(size, GFP_KERNEL);
026a1dc1af52742 drivers/spi/spi-topcliff-pch.c Jay Fang 2021-05-06 583 if (!data->pkt_rx_buff) {
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 584 kfree(data->pkt_tx_buff);
026a1dc1af52742 drivers/spi/spi-topcliff-pch.c Jay Fang 2021-05-06 585 data->pkt_tx_buff = NULL;
026a1dc1af52742 drivers/spi/spi-topcliff-pch.c Jay Fang 2021-05-06 586 }
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 587 }
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 588
65308c46b760bb2 drivers/spi/spi_topcliff_pch.c Grant Likely 2010-09-29 589 if (!data->pkt_rx_buff) {
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 590 /* flush queue and set status of all transfers to -ENOMEM */
cd8d984f0def2a8 drivers/spi/spi-topcliff-pch.c Wei Yongjun 2013-04-27 591 list_for_each_entry_safe(pmsg, tmp, data->queue.next, queue) {
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 592 pmsg->status = -ENOMEM;
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 593
f5d8ee3f15dd718 drivers/spi/spi-topcliff-pch.c Sachin Kamat 2013-05-31 594 if (pmsg->complete)
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 595 pmsg->complete(pmsg->context);
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 596
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 597 /* delete from queue */
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 598 list_del_init(&pmsg->queue);
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 599 }
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 600 return;
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 601 }
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 602
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 603 /* copy Tx Data */
393b49835a2b7cf drivers/spi/spi-topcliff-pch.c lijian 2021-06-24 604 if ((data->cur_trans->tx_buf != NULL) && (data->pkt_tx_buff != NULL)) {
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 605 if (*bpw == 8) {
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 606 tx_buf = data->cur_trans->tx_buf;
65308c46b760bb2 drivers/spi/spi_topcliff_pch.c Grant Likely 2010-09-29 607 for (j = 0; j < data->bpw_len; j++)
65308c46b760bb2 drivers/spi/spi_topcliff_pch.c Grant Likely 2010-09-29 608 data->pkt_tx_buff[j] = *tx_buf++;
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 609 } else {
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 610 tx_sbuf = data->cur_trans->tx_buf;
65308c46b760bb2 drivers/spi/spi_topcliff_pch.c Grant Likely 2010-09-29 611 for (j = 0; j < data->bpw_len; j++)
65308c46b760bb2 drivers/spi/spi_topcliff_pch.c Grant Likely 2010-09-29 612 data->pkt_tx_buff[j] = *tx_sbuf++;
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 613 }
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 614 }
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 615
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 616 /* if len greater than PCH_MAX_FIFO_DEPTH, write 16,else len bytes */
65308c46b760bb2 drivers/spi/spi_topcliff_pch.c Grant Likely 2010-09-29 617 n_writes = data->bpw_len;
65308c46b760bb2 drivers/spi/spi_topcliff_pch.c Grant Likely 2010-09-29 618 if (n_writes > PCH_MAX_FIFO_DEPTH)
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 619 n_writes = PCH_MAX_FIFO_DEPTH;
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 620
b996356d305fd31 drivers/spi/spi-topcliff-pch.c Markus Elfring 2017-01-13 621 dev_dbg(&data->master->dev,
b996356d305fd31 drivers/spi/spi-topcliff-pch.c Markus Elfring 2017-01-13 622 "\n%s:Pulling down SSN low - writing 0x2 to SSNXCR\n",
b996356d305fd31 drivers/spi/spi-topcliff-pch.c Markus Elfring 2017-01-13 623 __func__);
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 624 pch_spi_writereg(data->master, PCH_SSNXCR, SSN_LOW);
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 625
393b49835a2b7cf drivers/spi/spi-topcliff-pch.c lijian 2021-06-24 626 if (data->pkt_tx_buff != NULL) {
65308c46b760bb2 drivers/spi/spi_topcliff_pch.c Grant Likely 2010-09-29 627 for (j = 0; j < n_writes; j++)
65308c46b760bb2 drivers/spi/spi_topcliff_pch.c Grant Likely 2010-09-29 628 pch_spi_writereg(data->master, PCH_SPDWR, data->pkt_tx_buff[j]);
393b49835a2b7cf drivers/spi/spi-topcliff-pch.c lijian 2021-06-24 629 }

"j" not initialized on else path.

e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 630
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 631 /* update tx_index */
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 @632 data->tx_index = j;
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 633
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 634 /* reset transfer complete flag */
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 635 data->transfer_complete = false;
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 636 data->transfer_active = true;
e8b17b5b3f30252 drivers/spi/spi_topcliff_pch.c Masayuki Ohtake 2010-10-08 637 }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]