| Message ID | 20240823031059.32579-4-lihuisong@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add some features and bugfix for kunpeng_hccs | expand |
On Fri, 23 Aug 2024 11:10:56 +0800 Huisong Li <lihuisong@huawei.com> wrote: > If the shmem_base_addr from PCCT is zero, hccs_register_pcc_channel will > return success. And then driver will access to illegal address when send > PCC command. In addition, the size of shared memory used for communication > between driver and platform is fixed, namely 64 Bytes which is > unchangeable. So add the verification for them. > As with previous, make it clear if this hardening or fix fix to catch a problem on shipping hardware (I assume not, but you never know!) A comment on existing code inline. Not a suggestion for a change in this series, but maybe for the future. There are a lot of goto err_mbx_channel_Free in here already and this patch adds another. The cleanup there is trivial so DEFINE_FREE() and __free usage will make this code quite a bit nicer to read. > Signed-off-by: Huisong Li <lihuisong@huawei.com> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/soc/hisilicon/kunpeng_hccs.c | 24 +++++++++++++++--------- > 1 file changed, 15 insertions(+), 9 deletions(-) > > diff --git a/drivers/soc/hisilicon/kunpeng_hccs.c b/drivers/soc/hisilicon/kunpeng_hccs.c > index 6e88f597f267..6055e5091cbd 100644 > --- a/drivers/soc/hisilicon/kunpeng_hccs.c > +++ b/drivers/soc/hisilicon/kunpeng_hccs.c > @@ -170,15 +170,21 @@ static int hccs_register_pcc_channel(struct hccs_dev *hdev) > goto err_mbx_channel_free; > } > > - if (pcc_chan->shmem_base_addr) { > - cl_info->pcc_comm_addr = ioremap(pcc_chan->shmem_base_addr, > - pcc_chan->shmem_size); > - if (!cl_info->pcc_comm_addr) { > - dev_err(dev, "Failed to ioremap PCC communication region for channel-%u.\n", > - hdev->chan_id); > - rc = -ENOMEM; > - goto err_mbx_channel_free; > - } > + if (!pcc_chan->shmem_base_addr || > + pcc_chan->shmem_size != HCCS_PCC_SHARE_MEM_BYTES) { > + dev_err(dev, "The base address or size (%llu) of PCC communication region is invalid.\n", > + pcc_chan->shmem_size); > + rc = -EINVAL; > + goto err_mbx_channel_free; Worth considering for the future: Maybe a DEFINE_FREE for pcc_mbox_free_channel) makes sense, though if you do you should only assign cl_info->pcc_chan after all possible error paths. > + } > + > + cl_info->pcc_comm_addr = ioremap(pcc_chan->shmem_base_addr, > + pcc_chan->shmem_size); > + if (!cl_info->pcc_comm_addr) { > + dev_err(dev, "Failed to ioremap PCC communication region for channel-%u.\n", > + hdev->chan_id); > + rc = -ENOMEM; > + goto err_mbx_channel_free; > } > > return 0;
在 2024/8/23 16:38, Jonathan Cameron 写道: > On Fri, 23 Aug 2024 11:10:56 +0800 > Huisong Li <lihuisong@huawei.com> wrote: > >> If the shmem_base_addr from PCCT is zero, hccs_register_pcc_channel will >> return success. And then driver will access to illegal address when send >> PCC command. In addition, the size of shared memory used for communication >> between driver and platform is fixed, namely 64 Bytes which is >> unchangeable. So add the verification for them. >> > As with previous, make it clear if this hardening or fix > fix to catch a problem on shipping hardware (I assume not, but you never > know!) Ack > > A comment on existing code inline. Not a suggestion for a change > in this series, but maybe for the future. There are a lot > of goto err_mbx_channel_Free in here already and this patch adds > another. The cleanup there is trivial so DEFINE_FREE() and __free > usage will make this code quite a bit nicer to read. Yeah, it's a good way to simplify code on error path. I will take into account it. thanks for your good suggestion. > >> Signed-off-by: Huisong Li <lihuisong@huawei.com> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > >> --- >> drivers/soc/hisilicon/kunpeng_hccs.c | 24 +++++++++++++++--------- >> 1 file changed, 15 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/soc/hisilicon/kunpeng_hccs.c b/drivers/soc/hisilicon/kunpeng_hccs.c >> index 6e88f597f267..6055e5091cbd 100644 >> --- a/drivers/soc/hisilicon/kunpeng_hccs.c >> +++ b/drivers/soc/hisilicon/kunpeng_hccs.c >> @@ -170,15 +170,21 @@ static int hccs_register_pcc_channel(struct hccs_dev *hdev) >> goto err_mbx_channel_free; >> } >> >> - if (pcc_chan->shmem_base_addr) { >> - cl_info->pcc_comm_addr = ioremap(pcc_chan->shmem_base_addr, >> - pcc_chan->shmem_size); >> - if (!cl_info->pcc_comm_addr) { >> - dev_err(dev, "Failed to ioremap PCC communication region for channel-%u.\n", >> - hdev->chan_id); >> - rc = -ENOMEM; >> - goto err_mbx_channel_free; >> - } >> + if (!pcc_chan->shmem_base_addr || >> + pcc_chan->shmem_size != HCCS_PCC_SHARE_MEM_BYTES) { >> + dev_err(dev, "The base address or size (%llu) of PCC communication region is invalid.\n", >> + pcc_chan->shmem_size); >> + rc = -EINVAL; >> + goto err_mbx_channel_free; > Worth considering for the future: Maybe a DEFINE_FREE for pcc_mbox_free_channel) makes sense, > though if you do you should only assign cl_info->pcc_chan after all possible error paths. Ack > >> + } >> + >> + cl_info->pcc_comm_addr = ioremap(pcc_chan->shmem_base_addr, >> + pcc_chan->shmem_size); >> + if (!cl_info->pcc_comm_addr) { >> + dev_err(dev, "Failed to ioremap PCC communication region for channel-%u.\n", >> + hdev->chan_id); >> + rc = -ENOMEM; >> + goto err_mbx_channel_free; >> } >> >> return 0; > .
diff --git a/drivers/soc/hisilicon/kunpeng_hccs.c b/drivers/soc/hisilicon/kunpeng_hccs.c index 6e88f597f267..6055e5091cbd 100644 --- a/drivers/soc/hisilicon/kunpeng_hccs.c +++ b/drivers/soc/hisilicon/kunpeng_hccs.c @@ -170,15 +170,21 @@ static int hccs_register_pcc_channel(struct hccs_dev *hdev) goto err_mbx_channel_free; } - if (pcc_chan->shmem_base_addr) { - cl_info->pcc_comm_addr = ioremap(pcc_chan->shmem_base_addr, - pcc_chan->shmem_size); - if (!cl_info->pcc_comm_addr) { - dev_err(dev, "Failed to ioremap PCC communication region for channel-%u.\n", - hdev->chan_id); - rc = -ENOMEM; - goto err_mbx_channel_free; - } + if (!pcc_chan->shmem_base_addr || + pcc_chan->shmem_size != HCCS_PCC_SHARE_MEM_BYTES) { + dev_err(dev, "The base address or size (%llu) of PCC communication region is invalid.\n", + pcc_chan->shmem_size); + rc = -EINVAL; + goto err_mbx_channel_free; + } + + cl_info->pcc_comm_addr = ioremap(pcc_chan->shmem_base_addr, + pcc_chan->shmem_size); + if (!cl_info->pcc_comm_addr) { + dev_err(dev, "Failed to ioremap PCC communication region for channel-%u.\n", + hdev->chan_id); + rc = -ENOMEM; + goto err_mbx_channel_free; } return 0;
If the shmem_base_addr from PCCT is zero, hccs_register_pcc_channel will return success. And then driver will access to illegal address when send PCC command. In addition, the size of shared memory used for communication between driver and platform is fixed, namely 64 Bytes which is unchangeable. So add the verification for them. Signed-off-by: Huisong Li <lihuisong@huawei.com> --- drivers/soc/hisilicon/kunpeng_hccs.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-)