| Message ID | 20200809203406.751971-1-christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: amdgpu: Use the correct size when allocating memory | expand |
Am 09.08.20 um 22:34 schrieb Christophe JAILLET: > When '*sgt' is allocated, we must allocated 'sizeof(**sgt)' bytes instead > of 'sizeof(*sg)'. 'sg' (i.e. struct scatterlist) is smaller than > 'sgt' (i.e struct sg_table), so this could lead to memory corruption. > > Fixes: f44ffd677fb3 ("drm/amdgpu: add support for exporting VRAM using DMA-buf v3") > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> Good catch, Reviewed-by: Christian König <christian.koenig@amd.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c > index 134cc36e30c5..0739e259bf91 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c > @@ -462,7 +462,7 @@ int amdgpu_vram_mgr_alloc_sgt(struct amdgpu_device *adev, > unsigned int pages; > int i, r; > > - *sgt = kmalloc(sizeof(*sg), GFP_KERNEL); > + *sgt = kmalloc(sizeof(**sgt), GFP_KERNEL); > if (!*sgt) > return -ENOMEM; >
On Sun, Aug 09, 2020 at 10:34:06PM +0200, Christophe JAILLET wrote: > When '*sgt' is allocated, we must allocated 'sizeof(**sgt)' bytes instead > of 'sizeof(*sg)'. 'sg' (i.e. struct scatterlist) is smaller than > 'sgt' (i.e struct sg_table), so this could lead to memory corruption. The sizeof(*sg) is bigger than sizeof(**sgt) so this wastes memory but it won't lead to corruption. 11 struct scatterlist { 12 unsigned long page_link; 13 unsigned int offset; 14 unsigned int length; 15 dma_addr_t dma_address; 16 #ifdef CONFIG_NEED_SG_DMA_LENGTH 17 unsigned int dma_length; 18 #endif 19 }; 42 struct sg_table { 43 struct scatterlist *sgl; /* the list */ 44 unsigned int nents; /* number of mapped entries */ 45 unsigned int orig_nents; /* original size of list */ 46 }; regards, dan carpenter
Le 10/08/2020 à 17:42, Dan Carpenter a écrit : > On Sun, Aug 09, 2020 at 10:34:06PM +0200, Christophe JAILLET wrote: >> When '*sgt' is allocated, we must allocated 'sizeof(**sgt)' bytes instead >> of 'sizeof(*sg)'. 'sg' (i.e. struct scatterlist) is smaller than >> 'sgt' (i.e struct sg_table), so this could lead to memory corruption. > The sizeof(*sg) is bigger than sizeof(**sgt) so this wastes memory but > it won't lead to corruption. > > 11 struct scatterlist { > 12 unsigned long page_link; > 13 unsigned int offset; > 14 unsigned int length; > 15 dma_addr_t dma_address; > 16 #ifdef CONFIG_NEED_SG_DMA_LENGTH > 17 unsigned int dma_length; > 18 #endif > 19 }; > > 42 struct sg_table { > 43 struct scatterlist *sgl; /* the list */ > 44 unsigned int nents; /* number of mapped entries */ > 45 unsigned int orig_nents; /* original size of list */ > 46 }; > > regards, > dan carpenter My bad. I read 'struct scatterlist sgl' (without the *) Thanks for the follow-up, Dan. Doesn't smatch catch such mismatch? (I've not run smatch for a while, so it is maybe reported) Well, the proposal is still valid, even if it has less impact as initially thought. Thx for the review. CJ
Applied and updated the commit message to reflect the sizes. Thanks! Alex On Mon, Aug 10, 2020 at 3:07 PM Marion & Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote: > > > Le 10/08/2020 à 17:42, Dan Carpenter a écrit : > > On Sun, Aug 09, 2020 at 10:34:06PM +0200, Christophe JAILLET wrote: > >> When '*sgt' is allocated, we must allocated 'sizeof(**sgt)' bytes instead > >> of 'sizeof(*sg)'. 'sg' (i.e. struct scatterlist) is smaller than > >> 'sgt' (i.e struct sg_table), so this could lead to memory corruption. > > The sizeof(*sg) is bigger than sizeof(**sgt) so this wastes memory but > > it won't lead to corruption. > > > > 11 struct scatterlist { > > 12 unsigned long page_link; > > 13 unsigned int offset; > > 14 unsigned int length; > > 15 dma_addr_t dma_address; > > 16 #ifdef CONFIG_NEED_SG_DMA_LENGTH > > 17 unsigned int dma_length; > > 18 #endif > > 19 }; > > > > 42 struct sg_table { > > 43 struct scatterlist *sgl; /* the list */ > > 44 unsigned int nents; /* number of mapped entries */ > > 45 unsigned int orig_nents; /* original size of list */ > > 46 }; > > > > regards, > > dan carpenter > > > My bad. I read 'struct scatterlist sgl' (without the *) > Thanks for the follow-up, Dan. > > Doesn't smatch catch such mismatch? > (I've not run smatch for a while, so it is maybe reported) > > Well, the proposal is still valid, even if it has less impact as > initially thought. > > Thx for the review. > > CJ > > _______________________________________________ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
On Mon, Aug 10, 2020 at 08:41:14PM +0200, Marion & Christophe JAILLET wrote: > > Le 10/08/2020 à 17:42, Dan Carpenter a écrit : > > On Sun, Aug 09, 2020 at 10:34:06PM +0200, Christophe JAILLET wrote: > > > When '*sgt' is allocated, we must allocated 'sizeof(**sgt)' bytes instead > > > of 'sizeof(*sg)'. 'sg' (i.e. struct scatterlist) is smaller than > > > 'sgt' (i.e struct sg_table), so this could lead to memory corruption. > > The sizeof(*sg) is bigger than sizeof(**sgt) so this wastes memory but > > it won't lead to corruption. > > > > 11 struct scatterlist { > > 12 unsigned long page_link; > > 13 unsigned int offset; > > 14 unsigned int length; > > 15 dma_addr_t dma_address; > > 16 #ifdef CONFIG_NEED_SG_DMA_LENGTH > > 17 unsigned int dma_length; > > 18 #endif > > 19 }; > > > > 42 struct sg_table { > > 43 struct scatterlist *sgl; /* the list */ > > 44 unsigned int nents; /* number of mapped entries */ > > 45 unsigned int orig_nents; /* original size of list */ > > 46 }; > > > > regards, > > dan carpenter > > > My bad. I read 'struct scatterlist sgl' (without the *) > Thanks for the follow-up, Dan. > > Doesn't smatch catch such mismatch? > (I've not run smatch for a while, so it is maybe reported) That's why I was investigating it, because Smatch didn't catch it. Smatch would have warned if it led to memory corruption. Smatch also tries to detect struct mismatches as a separate check but for some reason it missed it. I'm not totally sure why yet. I suspect that it's a complicated internal reason where Sparse is the sizeof to a normal number... It's a known issue and hard to fix. > > Well, the proposal is still valid, even if it has less impact as initially > thought. Yep. regards, dan carpenter
On Tue, Aug 11, 2020 at 10:57:02AM +0300, Dan Carpenter wrote: > On Mon, Aug 10, 2020 at 08:41:14PM +0200, Marion & Christophe JAILLET wrote: > > > > Le 10/08/2020 à 17:42, Dan Carpenter a écrit : > > > On Sun, Aug 09, 2020 at 10:34:06PM +0200, Christophe JAILLET wrote: > > > > When '*sgt' is allocated, we must allocated 'sizeof(**sgt)' bytes instead > > > > of 'sizeof(*sg)'. 'sg' (i.e. struct scatterlist) is smaller than > > > > 'sgt' (i.e struct sg_table), so this could lead to memory corruption. > > > The sizeof(*sg) is bigger than sizeof(**sgt) so this wastes memory but > > > it won't lead to corruption. > > > > > > 11 struct scatterlist { > > > 12 unsigned long page_link; > > > 13 unsigned int offset; > > > 14 unsigned int length; > > > 15 dma_addr_t dma_address; > > > 16 #ifdef CONFIG_NEED_SG_DMA_LENGTH > > > 17 unsigned int dma_length; > > > 18 #endif > > > 19 }; > > > > > > 42 struct sg_table { > > > 43 struct scatterlist *sgl; /* the list */ > > > 44 unsigned int nents; /* number of mapped entries */ > > > 45 unsigned int orig_nents; /* original size of list */ > > > 46 }; > > > > > > regards, > > > dan carpenter > > > > > > My bad. I read 'struct scatterlist sgl' (without the *) > > Thanks for the follow-up, Dan. > > > > Doesn't smatch catch such mismatch? > > (I've not run smatch for a while, so it is maybe reported) > > That's why I was investigating it, because Smatch didn't catch it. > > Smatch would have warned if it led to memory corruption. Smatch also > tries to detect struct mismatches as a separate check but for some > reason it missed it. I'm not totally sure why yet. I suspect that it's > a complicated internal reason where Sparse is the sizeof to a normal s/is/changes/ > number... It's a known issue and hard to fix. regards, dan carpenter
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c index 134cc36e30c5..0739e259bf91 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c @@ -462,7 +462,7 @@ int amdgpu_vram_mgr_alloc_sgt(struct amdgpu_device *adev, unsigned int pages; int i, r; - *sgt = kmalloc(sizeof(*sg), GFP_KERNEL); + *sgt = kmalloc(sizeof(**sgt), GFP_KERNEL); if (!*sgt) return -ENOMEM;
When '*sgt' is allocated, we must allocated 'sizeof(**sgt)' bytes instead of 'sizeof(*sg)'. 'sg' (i.e. struct scatterlist) is smaller than 'sgt' (i.e struct sg_table), so this could lead to memory corruption. Fixes: f44ffd677fb3 ("drm/amdgpu: add support for exporting VRAM using DMA-buf v3") Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)