* [PATCH] scsi: ses: fix some risks of out of bound access @ 2019-03-25 7:40 Jianchao Wang 2019-03-26 1:19 ` jianchao.wang 2019-03-26 13:06 ` Ewan D. Milne 0 siblings, 2 replies; 4+ messages in thread From: Jianchao Wang @ 2019-03-25 7:40 UTC (permalink / raw) To: jejb, martin.petersen; +Cc: linux-scsi, linux-kernel We have some places with risk of accessing out of bound of the buffer allocated from slab, even it could corrupt the memory. Signed-off-by: Jianchao Wang <jianchao.w.wang@oracle.com> --- drivers/scsi/ses.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c index 0fc3922..42e6a1f 100644 --- a/drivers/scsi/ses.c +++ b/drivers/scsi/ses.c @@ -520,6 +520,7 @@ static void ses_enclosure_data_process(struct enclosure_device *edev, struct ses_device *ses_dev = edev->scratch; int types = ses_dev->page1_num_types; unsigned char *hdr_buf = kzalloc(INIT_ALLOC_SIZE, GFP_KERNEL); + unsigned char *page1_end = ses_dev->page1 + ses_dev->page1_len; if (!hdr_buf) goto simple_populate; @@ -556,6 +557,11 @@ static void ses_enclosure_data_process(struct enclosure_device *edev, type_ptr = ses_dev->page1_types; components = 0; for (i = 0; i < types; i++, type_ptr += 4) { + if (type_ptr > page1_end - 2) { + sdev_printk(KERN_ERR, sdev, "Access out of bound of page1" + "%p page1_end %p\n", page1_end, type_ptr); + break; + } for (j = 0; j < type_ptr[1]; j++) { char *name = NULL; struct enclosure_component *ecomp; @@ -566,10 +572,15 @@ static void ses_enclosure_data_process(struct enclosure_device *edev, } else { len = (desc_ptr[2] << 8) + desc_ptr[3]; desc_ptr += 4; - /* Add trailing zero - pushes into - * reserved space */ - desc_ptr[len] = '\0'; - name = desc_ptr; + if (desc_ptr + len >= buf + page7_len) { + desc_ptr = NULL; + } else { + + /* Add trailing zero - pushes into + * reserved space */ + desc_ptr[len] = '\0'; + name = desc_ptr; + } } } if (type_ptr[0] == ENCLOSURE_COMPONENT_DEVICE || @@ -693,7 +704,13 @@ static int ses_intf_add(struct device *cdev, /* begin at the enclosure descriptor */ type_ptr = buf + 8; /* skip all the enclosure descriptors */ - for (i = 0; i < num_enclosures && type_ptr < buf + len; i++) { + for (i = 0; i < num_enclosures; i++) { + if (type_ptr >= buf + len) { + sdev_printk(KERN_ERR, sdev, "Overflow the buf len = %d\n", len); + err = -EINVAL; + goto err_free; + } + types += type_ptr[2]; type_ptr += type_ptr[3] + 4; } -- 2.7.4 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] scsi: ses: fix some risks of out of bound access 2019-03-25 7:40 [PATCH] scsi: ses: fix some risks of out of bound access Jianchao Wang @ 2019-03-26 1:19 ` jianchao.wang 2019-03-26 13:06 ` Ewan D. Milne 1 sibling, 0 replies; 4+ messages in thread From: jianchao.wang @ 2019-03-26 1:19 UTC (permalink / raw) To: jejb, martin.petersen; +Cc: linux-scsi, linux-kernel Would anyone please take a look at this. Our customer encounter terrible memory corruption and panic due to this. Thanks Jianchao On 3/25/19 3:40 PM, Jianchao Wang wrote: > We have some places with risk of accessing out of bound of the > buffer allocated from slab, even it could corrupt the memory. > > Signed-off-by: Jianchao Wang <jianchao.w.wang@oracle.com> > --- > drivers/scsi/ses.c | 27 ++++++++++++++++++++++----- > 1 file changed, 22 insertions(+), 5 deletions(-) > > diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c > index 0fc3922..42e6a1f 100644 > --- a/drivers/scsi/ses.c > +++ b/drivers/scsi/ses.c > @@ -520,6 +520,7 @@ static void ses_enclosure_data_process(struct enclosure_device *edev, > struct ses_device *ses_dev = edev->scratch; > int types = ses_dev->page1_num_types; > unsigned char *hdr_buf = kzalloc(INIT_ALLOC_SIZE, GFP_KERNEL); > + unsigned char *page1_end = ses_dev->page1 + ses_dev->page1_len; > > if (!hdr_buf) > goto simple_populate; > @@ -556,6 +557,11 @@ static void ses_enclosure_data_process(struct enclosure_device *edev, > type_ptr = ses_dev->page1_types; > components = 0; > for (i = 0; i < types; i++, type_ptr += 4) { > + if (type_ptr > page1_end - 2) { > + sdev_printk(KERN_ERR, sdev, "Access out of bound of page1" > + "%p page1_end %p\n", page1_end, type_ptr); > + break; > + } > for (j = 0; j < type_ptr[1]; j++) { > char *name = NULL; > struct enclosure_component *ecomp; > @@ -566,10 +572,15 @@ static void ses_enclosure_data_process(struct enclosure_device *edev, > } else { > len = (desc_ptr[2] << 8) + desc_ptr[3]; > desc_ptr += 4; > - /* Add trailing zero - pushes into > - * reserved space */ > - desc_ptr[len] = '\0'; > - name = desc_ptr; > + if (desc_ptr + len >= buf + page7_len) { > + desc_ptr = NULL; > + } else { > + > + /* Add trailing zero - pushes into > + * reserved space */ > + desc_ptr[len] = '\0'; > + name = desc_ptr; > + } > } > } > if (type_ptr[0] == ENCLOSURE_COMPONENT_DEVICE || > @@ -693,7 +704,13 @@ static int ses_intf_add(struct device *cdev, > /* begin at the enclosure descriptor */ > type_ptr = buf + 8; > /* skip all the enclosure descriptors */ > - for (i = 0; i < num_enclosures && type_ptr < buf + len; i++) { > + for (i = 0; i < num_enclosures; i++) { > + if (type_ptr >= buf + len) { > + sdev_printk(KERN_ERR, sdev, "Overflow the buf len = %d\n", len); > + err = -EINVAL; > + goto err_free; > + } > + > types += type_ptr[2]; > type_ptr += type_ptr[3] + 4; > } > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] scsi: ses: fix some risks of out of bound access 2019-03-25 7:40 [PATCH] scsi: ses: fix some risks of out of bound access Jianchao Wang 2019-03-26 1:19 ` jianchao.wang @ 2019-03-26 13:06 ` Ewan D. Milne 2019-03-27 2:21 ` jianchao.wang 1 sibling, 1 reply; 4+ messages in thread From: Ewan D. Milne @ 2019-03-26 13:06 UTC (permalink / raw) To: Jianchao Wang, jejb, martin.petersen; +Cc: linux-scsi, linux-kernel See below. On Mon, 2019-03-25 at 15:40 +0800, Jianchao Wang wrote: > We have some places with risk of accessing out of bound of the > buffer allocated from slab, even it could corrupt the memory. > > Signed-off-by: Jianchao Wang <jianchao.w.wang@oracle.com> > --- > drivers/scsi/ses.c | 27 ++++++++++++++++++++++----- > 1 file changed, 22 insertions(+), 5 deletions(-) > > diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c > index 0fc3922..42e6a1f 100644 > --- a/drivers/scsi/ses.c > +++ b/drivers/scsi/ses.c > @@ -520,6 +520,7 @@ static void ses_enclosure_data_process(struct enclosure_device *edev, > struct ses_device *ses_dev = edev->scratch; > int types = ses_dev->page1_num_types; > unsigned char *hdr_buf = kzalloc(INIT_ALLOC_SIZE, GFP_KERNEL); > + unsigned char *page1_end = ses_dev->page1 + ses_dev->page1_len; > > if (!hdr_buf) > goto simple_populate; > @@ -556,6 +557,11 @@ static void ses_enclosure_data_process(struct enclosure_device *edev, > type_ptr = ses_dev->page1_types; > components = 0; > for (i = 0; i < types; i++, type_ptr += 4) { > + if (type_ptr > page1_end - 2) { I think "if (type_ptr + 1 >= page1_end)" would be more consistent. > + sdev_printk(KERN_ERR, sdev, "Access out of bound of page1" > + "%p page1_end %p\n", page1_end, type_ptr); This message is not helpful for someone with a device reporting invalid enclosure data. It should be more generic, like "Enclosure data too short" or "invalid" or something. And, the actual pointer values are irrelevant, it all depends upon the contents of the buffer. > + break; > + } > for (j = 0; j < type_ptr[1]; j++) { > char *name = NULL; > struct enclosure_component *ecomp; > @@ -566,10 +572,15 @@ static void ses_enclosure_data_process(struct enclosure_device *edev, > } else { > len = (desc_ptr[2] << 8) + desc_ptr[3]; > desc_ptr += 4; > - /* Add trailing zero - pushes into > - * reserved space */ > - desc_ptr[len] = '\0'; > - name = desc_ptr; > + if (desc_ptr + len >= buf + page7_len) { > + desc_ptr = NULL; > + } else { > + > + /* Add trailing zero - pushes into > + * reserved space */ > + desc_ptr[len] = '\0'; > + name = desc_ptr; > + } > } > } > if (type_ptr[0] == ENCLOSURE_COMPONENT_DEVICE || > @@ -693,7 +704,13 @@ static int ses_intf_add(struct device *cdev, > /* begin at the enclosure descriptor */ > type_ptr = buf + 8; > /* skip all the enclosure descriptors */ > - for (i = 0; i < num_enclosures && type_ptr < buf + len; i++) { > + for (i = 0; i < num_enclosures; i++) { > + if (type_ptr >= buf + len) { > + sdev_printk(KERN_ERR, sdev, "Overflow the buf len = %d\n", len); See above, the message is unhelpful. The actual problem is that the Enclosure data is too short or invalid. > + err = -EINVAL; > + goto err_free; > + } > + > types += type_ptr[2]; > type_ptr += type_ptr[3] + 4; > } This will still potentially leave type_ptr past the end of the buffer in the subsequent code, though, right? This might fix the problem for your malfunctioning device, but does not look like it would handle the general case. -Ewan ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] scsi: ses: fix some risks of out of bound access 2019-03-26 13:06 ` Ewan D. Milne @ 2019-03-27 2:21 ` jianchao.wang 0 siblings, 0 replies; 4+ messages in thread From: jianchao.wang @ 2019-03-27 2:21 UTC (permalink / raw) To: Ewan D. Milne, jejb, martin.petersen; +Cc: linux-scsi, linux-kernel Hi Ewan Thanks for your kindly response. On 3/26/19 9:06 PM, Ewan D. Milne wrote: > See below. > > On Mon, 2019-03-25 at 15:40 +0800, Jianchao Wang wrote: >> We have some places with risk of accessing out of bound of the >> buffer allocated from slab, even it could corrupt the memory. >> >> Signed-off-by: Jianchao Wang <jianchao.w.wang@oracle.com> >> --- >> drivers/scsi/ses.c | 27 ++++++++++++++++++++++----- >> 1 file changed, 22 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c >> index 0fc3922..42e6a1f 100644 >> --- a/drivers/scsi/ses.c >> +++ b/drivers/scsi/ses.c >> @@ -520,6 +520,7 @@ static void ses_enclosure_data_process(struct enclosure_device *edev, >> struct ses_device *ses_dev = edev->scratch; >> int types = ses_dev->page1_num_types; >> unsigned char *hdr_buf = kzalloc(INIT_ALLOC_SIZE, GFP_KERNEL); >> + unsigned char *page1_end = ses_dev->page1 + ses_dev->page1_len; >> >> if (!hdr_buf) >> goto simple_populate; >> @@ -556,6 +557,11 @@ static void ses_enclosure_data_process(struct enclosure_device *edev, >> type_ptr = ses_dev->page1_types; >> components = 0; >> for (i = 0; i < types; i++, type_ptr += 4) { >> + if (type_ptr > page1_end - 2) { > > I think "if (type_ptr + 1 >= page1_end)" would be more consistent. Yes, indeed. > >> + sdev_printk(KERN_ERR, sdev, "Access out of bound of page1" >> + "%p page1_end %p\n", page1_end, type_ptr); > > This message is not helpful for someone with a device reporting > invalid enclosure data. It should be more generic, like > "Enclosure data too short" or "invalid" or something. And, the > actual pointer values are irrelevant, it all depends upon the > contents of the buffer. > >> + break; >> + } >> for (j = 0; j < type_ptr[1]; j++) { >> char *name = NULL; >> struct enclosure_component *ecomp; >> @@ -566,10 +572,15 @@ static void ses_enclosure_data_process(struct enclosure_device *edev, >> } else { >> len = (desc_ptr[2] << 8) + desc_ptr[3]; >> desc_ptr += 4; >> - /* Add trailing zero - pushes into >> - * reserved space */ >> - desc_ptr[len] = '\0'; >> - name = desc_ptr; >> + if (desc_ptr + len >= buf + page7_len) { >> + desc_ptr = NULL; >> + } else { >> + >> + /* Add trailing zero - pushes into >> + * reserved space */ >> + desc_ptr[len] = '\0'; >> + name = desc_ptr; >> + } >> } >> } >> if (type_ptr[0] == ENCLOSURE_COMPONENT_DEVICE || >> @@ -693,7 +704,13 @@ static int ses_intf_add(struct device *cdev, >> /* begin at the enclosure descriptor */ >> type_ptr = buf + 8; >> /* skip all the enclosure descriptors */ >> - for (i = 0; i < num_enclosures && type_ptr < buf + len; i++) { >> + for (i = 0; i < num_enclosures; i++) { >> + if (type_ptr >= buf + len) { >> + sdev_printk(KERN_ERR, sdev, "Overflow the buf len = %d\n", len); > > See above, the message is unhelpful. The actual problem is that > the Enclosure data is too short or invalid. Yes, maybe we should dump all of the information of the page1 here. > >> + err = -EINVAL; >> + goto err_free; >> + } >> + >> types += type_ptr[2]; >> type_ptr += type_ptr[3] + 4; >> } > > This will still potentially leave type_ptr past the end of the > buffer in the subsequent code, though, right? Yes, type_ptr[3] is accessing type_ptr + 3 which has be beyond our checking point. > > This might fix the problem for your malfunctioning device, but > does not look like it would handle the general case. > Actually, I really don't know much about this ses driver. I sent out this bad patch because I want to push things forward as there is a serious issue on our customer side. Thanks Jianchao ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-03-27 2:21 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-03-25 7:40 [PATCH] scsi: ses: fix some risks of out of bound access Jianchao Wang 2019-03-26 1:19 ` jianchao.wang 2019-03-26 13:06 ` Ewan D. Milne 2019-03-27 2:21 ` jianchao.wang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).